Unsafe evolution: warn for using the attribute under legacy rules and some cleanup#82456
Conversation
It still makes sense to have unsafe blocks around all pointers, hence we don't hint to enable the updated memory safety rules in the legacy errors.
It's not relevant since we use an attribute now.
|
|
||
| | Warning ID | Description | | ||
| |------------|-------------| | ||
| | CS9508 | [RequiresUnsafeAttribute is only valid under the updated memory safety rules](https://github.com/dotnet/csharplang/issues/9704) | |
There was a problem hiding this comment.
Does the prototype checking infrastructure actually look at non-code files?
There was a problem hiding this comment.
Yes, it seems to filter out just binaries:
Line 10 in 1331de9
| return MessageID.IDS_FeatureUnsafeEvolution.GetFeatureAvailabilityDiagnosticInfo(this.Compilation); | ||
| } | ||
|
|
||
| // PROTOTYPE: Update the error message to hint that one can enable updated memory safety rules. |
There was a problem hiding this comment.
What was the resolution? #Closed
There was a problem hiding this comment.
It still makes sense to have unsafe blocks around all pointers even if that's not required by the updated rules, hence it doesn't seem appropriate to hint to enable the updated memory safety rules in the legacy errors.
There was a problem hiding this comment.
Agreed. I expect source generators may do just this.
| diagnostics.Add(ErrorCode.ERR_PartialMemberReadOnlyDifference, implementation.GetFirstLocation()); | ||
| } | ||
|
|
||
| // PROTOTYPE: Update and test this for unsafe evolution. |
There was a problem hiding this comment.
What was the resolution? #Closed
There was a problem hiding this comment.
We removed the possibility to mark property accessors as unsafe in a previous PR so this is no longer relevant for this feature.
| break; | ||
| case ErrorCode.WRN_RequiresUnsafeAttributeLegacyRules: | ||
| // These are the warnings introduced with the warning "wave" shipped with dotnet 11 and C# 15. | ||
| Assert.Equal(11, ErrorFacts.GetWarningLevel(errorCode)); |
There was a problem hiding this comment.
nit: not related to this PR, but it may be good to have a comment at the bottom of this test indicating that the warning wave doc should be updated #Closed
There was a problem hiding this comment.
There is already a comment to update that doc in GetWarningLevel method (which also needs to be updated). Are you suggesting it would be good to have such comment in both places?
There was a problem hiding this comment.
Doc in GetWarningLevel is good enough reminder. Thanks
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (commit 12). Only minor comments/questions
|
|
||
| | Warning ID | Description | | ||
| |------------|-------------| | ||
| | CS9508 | [RequiresUnsafeAttribute is only valid under the updated memory safety rules](https://github.com/dotnet/csharplang/issues/9704) | |
There was a problem hiding this comment.
Does the prototype checking infrastructure actually look at non-code files?
| return MessageID.IDS_FeatureUnsafeEvolution.GetFeatureAvailabilityDiagnosticInfo(this.Compilation); | ||
| } | ||
|
|
||
| // PROTOTYPE: Update the error message to hint that one can enable updated memory safety rules. |
There was a problem hiding this comment.
Agreed. I expect source generators may do just this.
60a23da
into
dotnet:features/UnsafeEvolution
Test plan: #81207
Related speclet update: dotnet/csharplang#9999
Commits should be mostly self-contained.