Unsafe evolution: check overrides and implementations#82172
Conversation
| // (7,1): error CS9502: 'I2.M2()' must be used in an unsafe context because it is marked as 'unsafe' or 'extern' | ||
| // i2.M2(); | ||
| Diagnostic(ErrorCode.ERR_UnsafeMemberOperation, "i2.M2()").WithArguments("I2.M2()").WithLocation(7, 1), | ||
| // (12,24): error CS9504: Unsafe member 'C3.M2()' overrides safe member 'I1.M2()' |
There was a problem hiding this comment.
Consider using a dedicated error message for implementation scenarios, since it's not an override
FWIW, nullability check uses CS8767 #Closed
| // (39,19): error CS9504: Unsafe member 'C6.I.M2()' overrides safe member 'I.M2()' | ||
| // extern void I.M2(); | ||
| Diagnostic(ErrorCode.ERR_CallerUnsafeOverridingSafe, "M2").WithArguments("C6.I.M2()", "I.M2()").WithLocation(39, 19), | ||
| ]); |
There was a problem hiding this comment.
Consider testing:
- interceptor scenario (we do nullability checks there, but intentionally not caller-unsafe checks)
- test effect of LeastOverridenMember with interesting hierarchy from metadata
- test hiding scenarios (the caller-unsafe bit should not count as part of the signature for hiding)
There was a problem hiding this comment.
test effect of LeastOverridenMember with interesting hierarchy from metadata
The initially added tests (e.g., Member_Method_Override) should already test interesting hierarchy from both metadata and source.
There was a problem hiding this comment.
The initially added tests (e.g., Member_Method_Override) should already test interesting hierarchy from both metadata and source.
Do we test metadata that cannot be produced from C#? For instance, metadata has base method is safe and derived method that is requires-unsafe; then source overrides
There was a problem hiding this comment.
Added Member_Method_Override_Metadata, thanks.
There was a problem hiding this comment.
Should probably add a virtual impl of an interface method too.
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (commit 2)
| .VerifyDiagnostics(); | ||
| var libRef = AsReference(lib, useCompilationReference); | ||
|
|
||
| // PROTOTYPE: Should a method like `I<T>.M(T)` be considered caller-unsafe under compat rules when substituted for `T = int*[]`? |
There was a problem hiding this comment.
Removed this prototype comment as I've realized it doesn't make sense to handle this. Corresponding speclet update: dotnet/csharplang#9962
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (commit 5). One test suggestion remains
| <value>'{0}' must be used in an unsafe context because it has pointers in its signature</value> | ||
| </data> | ||
| <data name="ERR_CallerUnsafeOverridingSafe" xml:space="preserve"> | ||
| <value>Unsafe member '{0}' overrides safe member '{1}'</value> |
There was a problem hiding this comment.
Think these would be phrased better as "cannot". IE:
| <value>Unsafe member '{0}' overrides safe member '{1}'</value> | |
| <value>Unsafe member '{0}' cannot override safe member '{1}'</value> |
| @@ -8,6 +8,7 @@ | |||
| using System.Reflection.Metadata.Ecma335; | |||
| using Microsoft.CodeAnalysis.CSharp.Symbols; | |||
| using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE; | |||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | |||
There was a problem hiding this comment.
Yes in Member_Interceptor for InvocationExpressionSyntax.
| // (39,19): error CS9504: Unsafe member 'C6.I.M2()' overrides safe member 'I.M2()' | ||
| // extern void I.M2(); | ||
| Diagnostic(ErrorCode.ERR_CallerUnsafeOverridingSafe, "M2").WithArguments("C6.I.M2()", "I.M2()").WithLocation(39, 19), | ||
| ]); |
There was a problem hiding this comment.
Should probably add a virtual impl of an interface method too.
|
@333fred for another look, thanks |
Test plan: #81207
Implements this part of the speclet: Overriding, inheritance, and implementation
Note: the first commit can be reviewed separately.