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.

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

Support UnscopedRefAttribute #62783

Merged
merged 14 commits into from
Aug 3, 2022
25 changes: 17 additions & 8 deletions docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,24 @@ static ref T ReturnOutParamByRef<T>(out T t)
}
```

A possible workaround is to change the method signature to pass the parameter by `ref` instead.
Possible workarounds are:
1. Use `System.Diagnostics.CodeAnalysis.UnscopedRefAttribute` to mark the reference as unscoped.
```csharp
Copy link
Member

@jcouv jcouv Jul 28, 2022

Choose a reason for hiding this comment

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

nit: other csharp blocks are left-indented. Not sure whether it's material (does that affect markup rendering on GitHub or docs.microsoft.com), but I'd stick with that to be safe. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Without indenting, subsequent bullets start at 1.

static ref T ReturnOutParamByRef<T>([UnscopedRef] out T t)
{
t = default;
return ref t; // ok
}
```

```csharp
static ref T ReturnRefParamByRef<T>(ref T t)
{
t = default;
return ref t; // ok
}
```
1. Change the method signature to pass the parameter by `ref`.
```csharp
static ref T ReturnRefParamByRef<T>(ref T t)
{
t = default;
return ref t; // ok
}
```

## Method ref struct return escape analysis depends on ref escape of ref arguments

Expand Down
30 changes: 26 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ private uint GetParameterValEscape(ParameterSymbol parameter)
{
if (UseUpdatedEscapeRules)
{
return parameter.Scope == DeclarationScope.ValueScoped ?
return parameter.EffectiveScope == DeclarationScope.ValueScoped ?
Binder.TopLevelScope :
Binder.ExternalScope;
}
Expand All @@ -796,7 +796,7 @@ private uint GetParameterRefEscape(ParameterSymbol parameter)
{
if (UseUpdatedEscapeRules)
{
return parameter.RefKind is RefKind.None || parameter.Scope != DeclarationScope.Unscoped ? Binder.TopLevelScope : Binder.ExternalScope;
return parameter.RefKind is RefKind.None || parameter.EffectiveScope != DeclarationScope.Unscoped ? Binder.TopLevelScope : Binder.ExternalScope;
}
else
{
Expand Down Expand Up @@ -1822,7 +1822,7 @@ bool considerParameter(ParameterSymbol? parameter)
// SPEC: For a given argument `a` that is passed to parameter `p`:
// SPEC: 1. ...
// SPEC: 2. If `p` is `scoped` then `a` does not contribute *safe-to-escape* when considering arguments.
if (parameter?.Scope == DeclarationScope.ValueScoped)
if (parameter?.EffectiveScope == DeclarationScope.ValueScoped)
{
return false;
}
Expand Down Expand Up @@ -1909,7 +1909,7 @@ private static RefKind GetEffectiveRefKind(
}
}

scope = paramIndex >= 0 ? parameters[paramIndex].Scope : DeclarationScope.Unscoped;
scope = paramIndex >= 0 ? parameters[paramIndex].EffectiveScope : DeclarationScope.Unscoped;
return effectiveRefKind;
}

Expand Down Expand Up @@ -2227,6 +2227,8 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres
return ((BoundLocal)expr).LocalSymbol.RefEscapeScope;

case BoundKind.ThisReference:
Debug.Assert(this.ContainingMember() is MethodSymbol { ThisParameter: not null });

var thisref = (BoundThisReference)expr;

// "this" is an RValue, unless in a struct.
Expand All @@ -2235,6 +2237,15 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres
break;
}

if (UseUpdatedEscapeRules)
{
if (this.ContainingMember() is MethodSymbol { ThisParameter: var thisParameter } &&
thisParameter.EffectiveScope == DeclarationScope.Unscoped)
{
return Binder.ExternalScope;
}
}

//"this" is not returnable by reference in a struct.
// can ref escape to any other level
return Binder.TopLevelScope;
Expand Down Expand Up @@ -2481,6 +2492,8 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF
return CheckLocalRefEscape(node, local, escapeTo, checkingReceiver, diagnostics);

case BoundKind.ThisReference:
Debug.Assert(this.ContainingMember() is MethodSymbol { ThisParameter: not null });

var thisref = (BoundThisReference)expr;

// "this" is an RValue, unless in a struct.
Expand All @@ -2492,6 +2505,15 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF
//"this" is not returnable by reference in a struct.
if (escapeTo == Binder.ExternalScope)
{
if (UseUpdatedEscapeRules)
{
if (this.ContainingMember() is MethodSymbol { ThisParameter: var thisParameter } &&
thisParameter.EffectiveScope == DeclarationScope.Unscoped)
{
// can ref escape to any other level
return true;
}
}
Error(diagnostics, ErrorCode.ERR_RefReturnStructThis, node);
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8846,8 +8846,8 @@ private MethodGroupResolution ResolveDefaultMethodGroup(
}

var parameters = method.Parameters;
var parameterScopes = parameters.Any(p => p.Scope != DeclarationScope.Unscoped) ?
parameters.SelectAsArray(p => p.Scope) :
var parameterScopes = parameters.Any(p => p.EffectiveScope != DeclarationScope.Unscoped) ?
parameters.SelectAsArray(p => p.EffectiveScope) :
default;
return GetMethodGroupOrLambdaDelegateType(node.Syntax, method.RefKind, method.ReturnTypeWithAnnotations, method.ParameterRefKinds, parameterScopes, method.ParameterTypesWithAnnotations);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ private ImmutableArray<ParameterSymbol> MakeParameters()
ordinal++,
p.RefKind,
p.Name,
p.Scope,
p.DeclaredScope,
// the synthesized parameter doesn't need to have the same ref custom modifiers as the base
refCustomModifiers: default,
inheritAttributes ? p as SourceComplexParameterSymbolBase : null));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,7 @@ void visitFunctionPointerSignature(IMethodSymbol symbol)
foreach (var param in symbol.Parameters)
{
// https://github.com/dotnet/roslyn/issues/61647: Use public API.
Debug.Assert((param as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().Scope switch
Debug.Assert((param as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().EffectiveScope switch
{
null => true,
DeclarationScope.Unscoped => param.RefKind != RefKind.Out,
Expand Down Expand Up @@ -780,7 +780,7 @@ public override void VisitParameter(IParameterSymbol symbol)
AddCustomModifiersIfNeeded(symbol.RefCustomModifiers, leadingSpace: false, trailingSpace: true);

// https://github.com/dotnet/roslyn/issues/61647: Use public API.
if ((symbol as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().Scope == DeclarationScope.ValueScoped &&
if ((symbol as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().EffectiveScope == DeclarationScope.ValueScoped &&
format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.IncludeScoped))
{
AddKeyword(SyntaxKind.ScopedKeyword);
Expand Down Expand Up @@ -1079,7 +1079,7 @@ private void AddParameterRefKindIfNeeded(IParameterSymbol symbol)
if (format.ParameterOptions.IncludesOption(SymbolDisplayParameterOptions.IncludeParamsRefOut))
{
// https://github.com/dotnet/roslyn/issues/61647: Use public API.
if ((symbol as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().Scope == DeclarationScope.RefScoped &&
if ((symbol as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().EffectiveScope == DeclarationScope.RefScoped &&
symbol.RefKind != RefKind.Out &&
format.CompilerInternalOptions.IncludesOption(SymbolDisplayCompilerInternalOptions.IncludeScoped))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,22 @@ public bool HasSkipLocalsInitAttribute
}
}

private bool _hasUnscopedRefAttribute;
public bool HasUnscopedRefAttribute
{
get
{
VerifySealed(expected: true);
return _hasUnscopedRefAttribute;
}
set
{
VerifySealed(expected: false);
_hasUnscopedRefAttribute = value;
SetDataStored();
}
}

private ImmutableArray<string> _memberNotNullAttributeData = ImmutableArray<string>.Empty;

public void AddNotNullMember(string memberName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,20 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
/// </summary>
internal sealed class ParameterEarlyWellKnownAttributeData : CommonParameterEarlyWellKnownAttributeData
{
private bool _hasUnscopedRefAttribute;
public bool HasUnscopedRefAttribute
{
get
{
VerifySealed(expected: true);
return _hasUnscopedRefAttribute;
}
set
{
VerifySealed(expected: false);
_hasUnscopedRefAttribute = value;
SetDataStored();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,22 @@ public bool HasSkipLocalsInitAttribute
}
}

private bool _hasUnscopedRefAttribute;
public bool HasUnscopedRefAttribute
{
get
{
VerifySealed(expected: true);
return _hasUnscopedRefAttribute;
}
set
{
VerifySealed(expected: false);
_hasUnscopedRefAttribute = value;
SetDataStored();
}
}

private ImmutableArray<string> _memberNotNullAttributeData = ImmutableArray<string>.Empty;

public void AddNotNullMember(string memberName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public FunctionPointerParameterSymbol(TypeWithAnnotations typeWithAnnotations, R
public override int Ordinal { get; }
public override Symbol ContainingSymbol => _containingSymbol;
public override ImmutableArray<CustomModifier> RefCustomModifiers { get; }
internal override DeclarationScope Scope => RefKind == RefKind.Out ? DeclarationScope.RefScoped : DeclarationScope.Unscoped;
internal override DeclarationScope DeclaredScope => RefKind == RefKind.Out ? DeclarationScope.RefScoped : DeclarationScope.Unscoped;
internal override DeclarationScope EffectiveScope => DeclaredScope;

public override bool Equals(Symbol other, TypeCompareKind compareKind)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,11 @@ private PEParameterSymbol(
typeWithAnnotations = NullableTypeDecoder.TransformType(typeWithAnnotations, handle, moduleSymbol, accessSymbol: accessSymbol, nullableContext: nullableContext);
typeWithAnnotations = TupleTypeDecoder.DecodeTupleTypesIfApplicable(typeWithAnnotations, handle, moduleSymbol);

if (_moduleSymbol.Module.HasLifetimeAnnotationAttribute(_handle, out var pair))
if (_moduleSymbol.Module.HasUnscopedRefAttribute(_handle))
{
scope = DeclarationScope.Unscoped;
Copy link
Member

@jcouv jcouv Jul 28, 2022

Choose a reason for hiding this comment

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

Should we mark the parameter as bad if there's both attributes? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

}
else if (_moduleSymbol.Module.HasLifetimeAnnotationAttribute(_handle, out var pair))
{
var scopeOpt = GetScope(refKind, typeWithAnnotations.Type, pair.IsRefScoped, pair.IsValueScoped);
if (scopeOpt is null)
Expand Down Expand Up @@ -978,7 +982,9 @@ public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences
}
}

internal sealed override DeclarationScope Scope => _packedFlags.Scope;
internal sealed override DeclarationScope DeclaredScope => _packedFlags.Scope;
Copy link
Member

@jcouv jcouv Jul 28, 2022

Choose a reason for hiding this comment

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

nit: Is PEParameterSymbol.DeclaredScope reachable (aside from the use on next line)? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like PEParameterSymbol.DeclaredScope is only used below, so perhaps we could limit DeclaredScope to source symbols only. That change may be a little involved though, so perhaps that could be done later if necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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


internal sealed override DeclarationScope EffectiveScope => DeclaredScope;

private static DeclarationScope? GetScope(RefKind refKind, TypeSymbol type, bool isRefScoped, bool isValueScoped)
{
Expand Down
12 changes: 11 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/ParameterSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,17 @@ internal sealed override ObsoleteAttributeData? ObsoleteAttributeData
/// </summary>
internal abstract bool HasInterpolatedStringHandlerArgumentError { get; }

internal abstract DeclarationScope Scope { get; }
/// <summary>
/// The declared scope. From source, this is from the <c>scope</c> keyword
/// and any implicit scope, ignoring any <c>UnscopedRefAttribute</c>.
/// </summary>
internal abstract DeclarationScope DeclaredScope { get; }

/// <summary>
/// The effective scope. This is from the declared scope and any
/// <c>UnscopedRefAttribute</c>.
/// </summary>
internal abstract DeclarationScope EffectiveScope { get; }

protected sealed override bool IsHighestPriorityUseSiteErrorCode(int code) => code is (int)ErrorCode.ERR_UnsupportedCompilerFeature or (int)ErrorCode.ERR_BogusType;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ public SignatureOnlyParameterSymbol(

public override bool IsDiscard { get { return false; } }

internal override DeclarationScope Scope => DeclarationScope.Unscoped;
internal override DeclarationScope DeclaredScope => DeclarationScope.Unscoped;
internal override DeclarationScope EffectiveScope => DeclaredScope;

#region Not used by MethodSignatureComparer

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ internal static bool RequiresLifetimeAnnotationAttribute(ParameterSymbol paramet
{
Debug.Assert(!parameter.IsThis);

var scope = parameter.Scope;
var scope = parameter.DeclaredScope;
if (scope == DeclarationScope.Unscoped)
{
return false;
Expand Down Expand Up @@ -648,7 +648,7 @@ private static void ReportParameterErrors(
diagnostics.Add(ErrorCode.ERR_MethodArgCantBeRefAny, parameterSyntax.Location, parameter.Type);
}

if (parameter.Scope == DeclarationScope.ValueScoped && !parameter.Type.IsErrorTypeOrRefLikeType())
if (parameter.DeclaredScope == DeclarationScope.ValueScoped && !parameter.Type.IsErrorTypeOrRefLikeType())
{
diagnostics.Add(ErrorCode.ERR_ScopedRefAndRefStructOnly, parameterSyntax.Location);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ internal override bool IsMetadataOptional
}
}

internal sealed override DeclarationScope Scope => _originalParam.Scope;
internal sealed override DeclarationScope DeclaredScope => _originalParam.DeclaredScope;
internal sealed override DeclarationScope EffectiveScope => _originalParam.EffectiveScope;

internal override ConstantValue ExplicitDefaultConstantValue
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,22 @@ internal bool HasEnumeratorCancellationAttribute

#nullable enable

internal sealed override DeclarationScope EffectiveScope
{
get
{
var scope = DeclaredScope;
if (scope != DeclarationScope.Unscoped &&
HasUnscopedRefAttribute)
{
return DeclarationScope.Unscoped;
}
return scope;
}
}

private bool HasUnscopedRefAttribute => GetEarlyDecodedWellKnownAttributeData()?.HasUnscopedRefAttribute == true;

internal static SyntaxNode? GetDefaultValueSyntaxForIsNullableAnalysisEnabled(ParameterSyntax? parameterSyntax) =>
parameterSyntax?.Default?.Value;

Expand Down Expand Up @@ -593,6 +609,13 @@ internal override (CSharpAttributeData?, BoundAttribute?) EarlyDecodeWellKnownAt
{
return EarlyDecodeAttributeForDefaultParameterValue(AttributeDescription.DateTimeConstantAttribute, ref arguments);
}
else if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.UnscopedRefAttribute))
{
// We can't bind the attribute here because that might lead to a cycle.
// Instead, simply record that the attribute exists and bind later.
arguments.GetOrCreateData<ParameterEarlyWellKnownAttributeData>().HasUnscopedRefAttribute = true;
return (null, null);
}
else if (!IsOnPartialImplementation(arguments.AttributeSyntax))
{
if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.CallerLineNumberAttribute))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1392,7 +1392,7 @@ internal static bool CheckValidScopedOverride<TArg>(
{
var baseParameter = baseParameters[i];
var overrideParameter = overrideParameters[i + overrideParameterOffset];
if (baseParameter.Scope != overrideParameter.Scope)
if (baseParameter.EffectiveScope != overrideParameter.EffectiveScope)
{
reportMismatchInParameterType(diagnostics, baseMethod, overrideMethod, overrideParameter, topLevel: true, extraArgument);
hasErrors = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,10 @@ private void DecodeWellKnownAttributeAppliedToMethod(ref DecodeWellKnownAttribut
{
DecodeUnmanagedCallersOnlyAttribute(ref arguments);
}
else if (attribute.IsTargetAttribute(this, AttributeDescription.UnscopedRefAttribute))
{
arguments.GetOrCreateData<MethodWellKnownAttributeData>().HasUnscopedRefAttribute = true;
}
else
{
var compilation = this.DeclaringCompilation;
Expand Down Expand Up @@ -600,6 +604,8 @@ public override FlowAnalysisAnnotations FlowAnalysisAnnotations
private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(MethodWellKnownAttributeData attributeData)
=> attributeData?.HasDoesNotReturnAttribute == true ? FlowAnalysisAnnotations.DoesNotReturn : FlowAnalysisAnnotations.None;

internal bool HasUnscopedRefAttribute => GetDecodedWellKnownAttributeData()?.HasUnscopedRefAttribute == true;

private bool VerifyObsoleteAttributeAppliedToMethod(
ref DecodeWellKnownAttributeArguments<AttributeSyntax, CSharpAttributeData, AttributeLocation> arguments,
AttributeDescription description)
Expand Down
Loading