Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove scope variance exceptions #76296

Merged
merged 7 commits into from
Jan 6, 2025
+524 −127
Merged

Remove scope variance exceptions #76296

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f0d3f46
Remove scope variance exceptions
jjonescz Dec 4, 2024
62910ca
Add UnscopedRef example
jjonescz Dec 10, 2024
0539a70
Report new scoped mismatches as warnings in older lang versions
jjonescz Dec 12, 2024
810f1e3
Merge branch 'main' into 76100-ScopeVariance-NoExceptions
jjonescz Dec 13, 2024
1d15cfb
Fixup a test
jjonescz Dec 13, 2024
dc450fc
Fixup another test
jjonescz Dec 13, 2024
88b5b10
Merge branch 'main' into 76100-ScopeVariance-NoExceptions
jjonescz Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,22 @@ class C
}
```

## Variance of `scoped` and `[UnscopedRef]` is more strict
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I found these breaks when building the BCL with this change: dotnet/runtime@fae33d2e25a

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub FYI


***Introduced in Visual Studio 2022 version 17.13***

Scope can be changed when overriding a method, implementing an interface, or converting a lambda/method to a delegate under
[some conditions](https://github.com/dotnet/csharplang/blob/05064c2a9567b7a58a07e526dff403ece1866541/proposals/csharp-11.0/low-level-struct-improvements.md#scoped-mismatch)
(roughly, `scoped` can be added and `[UnscopedRef]` can be removed).
Previously, the compiler did not report an error/warning for such mismatch under some circumstances, but it is now always reported.

```cs
D1 d1 = (ref int i) => { }; // previously no mismatch error reported, now:
// error CS8986: The 'scoped' modifier of parameter 'i' doesn't match target 'D1'.

D2 d2 = (ref int i) => ref i; // an error was and continues to be reported:
// error CS8986: The 'scoped' modifier of parameter 'i' doesn't match target 'D2'.

delegate void D1(scoped ref int x);
delegate ref int D2(scoped ref int x);
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
```
37 changes: 17 additions & 20 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2274,26 +2274,23 @@ private static void CheckParameterModifierMismatchMethodConversion(SyntaxNode sy
return;
}

if (SourceMemberContainerTypeSymbol.RequiresValidScopedOverrideForRefSafety(delegateMethod))
{
SourceMemberContainerTypeSymbol.CheckValidScopedOverride(
delegateMethod,
lambdaOrMethod,
diagnostics,
static (diagnostics, delegateMethod, lambdaOrMethod, parameter, _, typeAndSyntax) =>
{
diagnostics.Add(
SourceMemberContainerTypeSymbol.ReportInvalidScopedOverrideAsError(delegateMethod, lambdaOrMethod) ?
ErrorCode.ERR_ScopedMismatchInParameterOfTarget :
ErrorCode.WRN_ScopedMismatchInParameterOfTarget,
typeAndSyntax.Syntax.Location,
new FormattedSymbol(parameter, SymbolDisplayFormat.ShortFormat),
typeAndSyntax.Type);
},
(Type: targetType, Syntax: syntax),
allowVariance: true,
invokedAsExtensionMethod: invokedAsExtensionMethod);
}
SourceMemberContainerTypeSymbol.CheckValidScopedOverride(
delegateMethod,
lambdaOrMethod,
diagnostics,
static (diagnostics, delegateMethod, lambdaOrMethod, parameter, _, typeAndSyntax) =>
{
diagnostics.Add(
SourceMemberContainerTypeSymbol.ReportInvalidScopedOverrideAsError(delegateMethod, lambdaOrMethod) ?
ErrorCode.ERR_ScopedMismatchInParameterOfTarget :
ErrorCode.WRN_ScopedMismatchInParameterOfTarget,
typeAndSyntax.Syntax.Location,
new FormattedSymbol(parameter, SymbolDisplayFormat.ShortFormat),
typeAndSyntax.Type);
},
(Type: targetType, Syntax: syntax),
allowVariance: true,
invokedAsExtensionMethod: invokedAsExtensionMethod);

SourceMemberContainerTypeSymbol.CheckRefReadonlyInMismatch(
delegateMethod, lambdaOrMethod, diagnostics,
Expand Down
82 changes: 16 additions & 66 deletions ...pilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,25 +1149,22 @@ static void checkValidMethodOverride(
MethodSymbol overridingMethod,
BindingDiagnosticBag diagnostics)
{
if (RequiresValidScopedOverrideForRefSafety(overriddenMethod))
{
CheckValidScopedOverride(
overriddenMethod,
overridingMethod,
diagnostics,
static (diagnostics, overriddenMethod, overridingMethod, overridingParameter, _, location) =>
{
diagnostics.Add(
ReportInvalidScopedOverrideAsError(overriddenMethod, overridingMethod) ?
ErrorCode.ERR_ScopedMismatchInParameterOfOverrideOrImplementation :
ErrorCode.WRN_ScopedMismatchInParameterOfOverrideOrImplementation,
location,
new FormattedSymbol(overridingParameter, SymbolDisplayFormat.ShortFormat));
},
overridingMemberLocation,
allowVariance: true,
invokedAsExtensionMethod: false);
}
CheckValidScopedOverride(
overriddenMethod,
overridingMethod,
diagnostics,
static (diagnostics, overriddenMethod, overridingMethod, overridingParameter, _, location) =>
{
diagnostics.Add(
ReportInvalidScopedOverrideAsError(overriddenMethod, overridingMethod) ?
ErrorCode.ERR_ScopedMismatchInParameterOfOverrideOrImplementation :
ErrorCode.WRN_ScopedMismatchInParameterOfOverrideOrImplementation,
location,
new FormattedSymbol(overridingParameter, SymbolDisplayFormat.ShortFormat));
},
overridingMemberLocation,
allowVariance: true,
invokedAsExtensionMethod: false);

CheckValidNullableMethodOverride(overridingMethod.DeclaringCompilation, overriddenMethod, overridingMethod, diagnostics,
ReportBadReturn,
Expand Down Expand Up @@ -1369,53 +1366,6 @@ static bool isValidNullableConversion(
}

#nullable enable
/// <summary>
/// Returns true if the method signature must match, with respect to scoped for ref safety,
/// in overrides, interface implementations, or delegate conversions.
/// </summary>
internal static bool RequiresValidScopedOverrideForRefSafety(MethodSymbol? method)
{
if (method is null)
{
return false;
}

var parameters = method.Parameters;

// https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/low-level-struct-improvements.md#scoped-mismatch
// The compiler will report a diagnostic for _unsafe scoped mismatches_ across overrides, interface implementations, and delegate conversions when:
// - The method returns a `ref struct` or returns a `ref` or `ref readonly`, or the method has a `ref` or `out` parameter of `ref struct` type, and
// ...
int nRefParametersRequired;
if (method.ReturnType.IsRefLikeOrAllowsRefLikeType() ||
(method.RefKind is RefKind.Ref or RefKind.RefReadOnly))
{
nRefParametersRequired = 1;
}
else if (parameters.Any(p => (p.RefKind is RefKind.Ref or RefKind.Out) && p.Type.IsRefLikeOrAllowsRefLikeType()))
{
nRefParametersRequired = 2; // including the parameter found above
}
else
{
return false;
}

// ...
// - The method has at least one additional `ref`, `in`, `ref readonly`, or `out` parameter, or a parameter of `ref struct` type.
int nRefParameters = parameters.Count(p => p.RefKind is RefKind.Ref or RefKind.In or RefKind.RefReadOnlyParameter or RefKind.Out);
if (nRefParameters >= nRefParametersRequired)
{
return true;
}
else if (parameters.Any(p => p.RefKind == RefKind.None && p.Type.IsRefLikeOrAllowsRefLikeType()))
{
return true;
}

return false;
}

/// <summary>
/// Returns true if a scoped mismatch should be reported as an error rather than a warning.
/// </summary>
Expand Down
36 changes: 17 additions & 19 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,25 +1855,23 @@ static void checkMethodOverride(
reportMismatchInParameterType,
(implementingType, isExplicit));

if (SourceMemberContainerTypeSymbol.RequiresValidScopedOverrideForRefSafety(implementedMethod))
{
SourceMemberContainerTypeSymbol.CheckValidScopedOverride(
implementedMethod,
implementingMethod,
diagnostics,
static (diagnostics, implementedMethod, implementingMethod, implementingParameter, _, arg) =>
{
diagnostics.Add(
SourceMemberContainerTypeSymbol.ReportInvalidScopedOverrideAsError(implementedMethod, implementingMethod) ?
ErrorCode.ERR_ScopedMismatchInParameterOfOverrideOrImplementation :
ErrorCode.WRN_ScopedMismatchInParameterOfOverrideOrImplementation,
GetImplicitImplementationDiagnosticLocation(implementedMethod, arg.implementingType, implementingMethod),
new FormattedSymbol(implementingParameter, SymbolDisplayFormat.ShortFormat));
},
(implementingType, isExplicit),
allowVariance: true,
invokedAsExtensionMethod: false);
}
SourceMemberContainerTypeSymbol.CheckValidScopedOverride(
implementedMethod,
implementingMethod,
diagnostics,
static (diagnostics, implementedMethod, implementingMethod, implementingParameter, _, arg) =>
{
diagnostics.Add(
SourceMemberContainerTypeSymbol.ReportInvalidScopedOverrideAsError(implementedMethod, implementingMethod) ?
ErrorCode.ERR_ScopedMismatchInParameterOfOverrideOrImplementation :
ErrorCode.WRN_ScopedMismatchInParameterOfOverrideOrImplementation,
GetImplicitImplementationDiagnosticLocation(implementedMethod, arg.implementingType, implementingMethod),
new FormattedSymbol(implementingParameter, SymbolDisplayFormat.ShortFormat));
},
(implementingType, isExplicit),
allowVariance: true,
invokedAsExtensionMethod: false);

SourceMemberContainerTypeSymbol.CheckRefReadonlyInMismatch(
implementedMethod, implementingMethod, diagnostics,
static (diagnostics, implementedMethod, implementingMethod, implementingParameter, _, arg) =>
Expand Down
12 changes: 9 additions & 3 deletions src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21357,7 +21357,7 @@ class Helper<T>
delegate void D2(T x);

static D1 d11 = M1;
static D1 d12 = M2;
static D1 d12 = M2; // 1
static D2 d21 = M1;
static D2 d22 = M2;

Expand All @@ -21371,7 +21371,7 @@ class Helper
delegate void D2(Span<int> x);

static D1 d11 = M1;
static D1 d12 = M2;
static D1 d12 = M2; // 2
static D2 d21 = M1;
static D2 d22 = M2;

Expand All @@ -21380,7 +21380,13 @@ static void M2(Span<int> x) {}
}
";
var comp = CreateCompilation(src, targetFramework: s_targetFrameworkSupportingByRefLikeGenerics);
comp.VerifyEmitDiagnostics();
comp.VerifyEmitDiagnostics(
// (11,21): error CS8986: The 'scoped' modifier of parameter 'x' doesn't match target 'Helper<T>.D1'.
// static D1 d12 = M2; // 1
Diagnostic(ErrorCode.ERR_ScopedMismatchInParameterOfTarget, "M2").WithArguments("x", "Helper<T>.D1").WithLocation(11, 21),
// (25,21): error CS8986: The 'scoped' modifier of parameter 'x' doesn't match target 'Helper.D1'.
// static D1 d12 = M2; // 2
Diagnostic(ErrorCode.ERR_ScopedMismatchInParameterOfTarget, "M2").WithArguments("x", "Helper.D1").WithLocation(25, 21));
}

[Fact]
Expand Down
Loading