Forbid unsafe types in records #76588
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[The specification](https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/records.md#members-of-a-record-type) for records forbids the use of unsafe types as an instance field of the type. However, we didn't check that in all cases; we only checked when generating the `Equals` method for the record type. This meant that if the user provided their own definition of `Equals`, they could bypass this restriction. We also did not check for nested unsafe types, so `int*[]` was permitted in any scenario. We have 2 options for fixing this: 1. Treat it as a specification bug, and update the spec to follow the current behavior. We'd therefore fix dotnet#66312 by moving the diagnostic to SourceNamedTypeSymbol.AfterMembersChecks, and only error when the user didn't provide their own implementation of `Equals`. 2. Treat it as a compiler bug, and forbid use of unsafe types in all scenarios. I've opted for path 2 here, but we can discuss whether this is too aggressive and if we should opt for path 1 instead. Fixes dotnet#66312.
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
|
We also need to decide what to do with manually-implemented properties that have unsafe types. As pointed out by @hamarb123, even with this change we'll still generate invalid IL for |
AlekseyTs
reviewed
Jan 3, 2025
|
|
||
| ***Introduced in Visual Studio 2022 version 17.14*** | ||
|
|
||
| The specification for `record class` and `record struct` types indicated that any unsafe types are disallowed. However, this was not |
AlekseyTs
reviewed
Jan 3, 2025
| enforced correctly in 2 scenarios: | ||
|
|
||
| 1. When the `record class` or `record struct` type defined its own `Equals` implementation. | ||
| 2. When the field type only used the unsafe type in a nested context, such as `int*[]`. |
AlekseyTs
reviewed
Jan 3, 2025
| diagnostics.Add(ErrorCode.ERR_FieldAutoPropCantBeByRefLike, TypeSyntax?.Location ?? this.GetFirstLocation(), type); | ||
| diagnostics.Add(ErrorCode.ERR_FieldAutoPropCantBeByRefLike, getTypeErrorLocation(), type); | ||
| } | ||
| else if (!this.IsStatic && type.ContainsPointerOrFunctionPointer() && (ContainingType.IsRecord || ContainingType.IsRecordStruct)) |
AlekseyTs
reviewed
Jan 3, 2025
| { | ||
| diagnostics.Add(ErrorCode.ERR_FieldAutoPropCantBeByRefLike, TypeLocation, type); | ||
| if (!this.IsStatic && type.ContainsPointerOrFunctionPointer() && (ContainingType.IsRecord || ContainingType.IsRecordStruct)) |
AlekseyTs
reviewed
Jan 3, 2025
| comp.VerifyEmitDiagnostics( | ||
| // (12,22): error CS0723: Cannot declare a variable of static type 'C2' | ||
| var comp = CreateCompilation(new[] { src, IsExternalInitTypeDefinition }, options: TestOptions.UnsafeDebugDll, targetFramework: TargetFramework.Mscorlib461); | ||
| DiagnosticDescription[] expected = [// (12,22): error CS0723: Cannot declare a variable of static type 'C2' |
|
It looks like more test base-lines need an adjustment |
|
Done with review pass (commit 1) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The specification for records forbids the use of unsafe types as an instance field of the type. However, we didn't check that in all cases; we only checked when generating the
Equalsmethod for the record type. This meant that if the user provided their own definition ofEquals, they could bypass this restriction. We also did not check for nested unsafe types, soint*[]was permitted in any scenario. We have 2 options for fixing this:Equals.I've opted for path 2 here, but we can discuss whether this is too aggressive and if we should opt for path 1 instead. Fixes #66312.