Skip to content

Commit

Permalink
Remove scope variance exceptions (#76296)
Browse files Browse the repository at this point in the history 
* Remove scope variance exceptions

* Add UnscopedRef example

* Report new scoped mismatches as warnings in older lang versions

* Fixup a test

* Fixup another test
  • Loading branch information
@jjonescz
jjonescz authored Jan 6, 2025
1 parent 642ee01 commit b86d831
Show file tree
Hide file tree
Showing 7 changed files with 524 additions and 127 deletions.
34 changes: 34 additions & 0 deletions docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,40 @@ struct S
}
```

## Variance of `scoped` and `[UnscopedRef]` is more strict

***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.
Note that the error is downgraded to a warning in `unsafe` contexts and also (in scenarios where it would be a breaking change) with LangVersion 12 or lower.

```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);
```

```cs
using System.Diagnostics.CodeAnalysis;

D1 d1 = ([UnscopedRef] ref int i) => { }; // previously no mismatch error reported, now:
// error CS8986: The 'scoped' modifier of parameter 'i' doesn't match target 'D1'.
D2 d2 = ([UnscopedRef] 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(ref int x);
delegate ref int D2(ref int x);
```

## `Microsoft.CodeAnalysis.EmbeddedAttribute` is validated on declaration

***Introduced in Visual Studio 2022 version 17.13***
Expand Down
37 changes: 17 additions & 20 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
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
126 changes: 59 additions & 67 deletions ...pilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs
View file
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,61 +1366,56 @@ 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>
internal static bool ReportInvalidScopedOverrideAsError(MethodSymbol baseMethod, MethodSymbol overrideMethod)
{
// https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/low-level-struct-improvements.md#scoped-mismatch
// The diagnostic is reported as an error if the mismatched signatures are both using C#11 ref safety rules; otherwise, the diagnostic is a warning.
return baseMethod.UseUpdatedEscapeRules && overrideMethod.UseUpdatedEscapeRules;
return baseMethod.UseUpdatedEscapeRules && overrideMethod.UseUpdatedEscapeRules &&
// We have removed exceptions to the scoped mismatch error reporting, but to avoid breaks
// we report the new scenarios (previously exempted) as warnings in C# 12 and earlier.
// https://github.com/dotnet/roslyn/issues/76100
(overrideMethod.DeclaringCompilation.LanguageVersion > LanguageVersion.CSharp12 || usedToBeReported(baseMethod));

static bool usedToBeReported(MethodSymbol method)
{
var parameters = method.Parameters;

// https://github.com/dotnet/csharplang/blob/1f7f23f/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>
Expand Down
36 changes: 17 additions & 19 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
View file
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
Original file line number Diff line number Diff line change
Expand Up @@ -21358,7 +21358,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 @@ -21372,7 +21372,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 @@ -21381,7 +21381,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

0 comments on commit b86d831

Please sign in to comment.