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

Do not resolve diagnostics early during attribute binding #71140

Merged
merged 10 commits into from
Dec 15, 2023
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 @@ -6846,7 +6846,7 @@ private BoundExpression BindLeftIdentifierOfPotentialColorColorMemberAccess(Iden
// NOTE: ReplaceTypeOrValueReceiver will call CheckValue explicitly.
boundValue = BindToNaturalType(boundValue, valueDiagnostics);
return new BoundTypeOrValueExpression(left,
new BoundTypeOrValueData(leftSymbol, boundValue, valueDiagnostics.ToReadOnlyAndFree(), boundType, typeDiagnostics.ToReadOnlyAndFree()), leftType);
Copy link
Member Author

Choose a reason for hiding this comment

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

This was recursion source 1. ToReadOnlyAndFree forces resolution of lazy diagnostics. We would attempt to resolve whether to report obsolete on the value, which then forced us to bind assembly attributes, bringing us right back here.

333fred marked this conversation as resolved.
Show resolved Hide resolved
new BoundTypeOrValueData(leftSymbol, boundValue, valueDiagnostics.ToReadOnlyAndFree(forceDiagnosticResolution: false), boundType, typeDiagnostics.ToReadOnlyAndFree(forceDiagnosticResolution: false)), leftType);
}

typeDiagnostics.Free();
Expand Down Expand Up @@ -9727,7 +9727,7 @@ private MethodGroupResolution ResolveDefaultMethodGroup(
}
}

var sealedDiagnostics = ImmutableBindingDiagnostic<AssemblySymbol>.Empty;
var sealedDiagnostics = ReadOnlyBindingDiagnostic<AssemblySymbol>.Empty;
if (node.LookupError != null)
{
var diagnostics = BindingDiagnosticBag.GetInstance(withDiagnostics: true, withDependencies: false);
Expand Down
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1728,9 +1728,12 @@ private BoundExpression ReplaceTypeOrValueReceiver(BoundExpression receiver, boo

foreach (Diagnostic d in typeOrValue.Data.ValueDiagnostics.Diagnostics)
{
if (d.Code == (int)ErrorCode.WRN_PrimaryConstructorParameterIsShadowedAndNotPassedToBase &&
Copy link
Member Author

Choose a reason for hiding this comment

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

.Code was the second source of recursion, once the first was addressed. It forces resolution of the diagnostic info, throwing us right back into the same problem. To address this, I added an IsCode method on Diagnostic, which will not force resolution of lazy diagnostics. The only thing lazy about lazy diagnostics is whether or not they should be reported: we do know what code they are ahead of time. Arguably, we could avoid resolving lazy diagnostics with the Code property. I felt that I wasn't comfortable with that big a change here, but if we'd prefer that approach, I can do that.

// Avoid forcing resolution of lazy diagnostics to avoid cycles.
var code = d is DiagnosticWithInfo { HasLazyInfo: true, LazyInfo.Code: var lazyCode } ? lazyCode : d.Code;
if (code == (int)ErrorCode.WRN_PrimaryConstructorParameterIsShadowedAndNotPassedToBase &&
333fred marked this conversation as resolved.
Show resolved Hide resolved
!(d.Arguments is [ParameterSymbol shadowedParameter] && shadowedParameter.Type.Equals(typeOrValue.Data.ValueExpression.Type, TypeCompareKind.AllIgnoreOptions))) // If the type and the name match, we would resolve to the same type rather than a value at the end.
{
Debug.Assert(d is not DiagnosticWithInfo { HasLazyInfo: true }, "Adjust the Arguments access to handle lazy diagnostics to avoid cycles.");
diagnostics.Add(d);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/LockOrUsingBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ protected BoundExpression BindTargetExpression(BindingDiagnosticBag diagnostics,
private class ExpressionAndDiagnostics
{
public readonly BoundExpression Expression;
public readonly ImmutableBindingDiagnostic<AssemblySymbol> Diagnostics;
public readonly ReadOnlyBindingDiagnostic<AssemblySymbol> Diagnostics;

public ExpressionAndDiagnostics(BoundExpression expression, ImmutableBindingDiagnostic<AssemblySymbol> diagnostics)
public ExpressionAndDiagnostics(BoundExpression expression, ReadOnlyBindingDiagnostic<AssemblySymbol> diagnostics)
{
this.Expression = expression;
this.Diagnostics = diagnostics;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ internal readonly struct MethodGroupResolution
public readonly Symbol OtherSymbol;
public readonly OverloadResolutionResult<MethodSymbol> OverloadResolutionResult;
public readonly AnalyzedArguments AnalyzedArguments;
public readonly ImmutableBindingDiagnostic<AssemblySymbol> Diagnostics;
public readonly ReadOnlyBindingDiagnostic<AssemblySymbol> Diagnostics;
public readonly LookupResultKind ResultKind;

public MethodGroupResolution(MethodGroup methodGroup, ImmutableBindingDiagnostic<AssemblySymbol> diagnostics)
public MethodGroupResolution(MethodGroup methodGroup, ReadOnlyBindingDiagnostic<AssemblySymbol> diagnostics)
: this(methodGroup, otherSymbol: null, overloadResolutionResult: null, analyzedArguments: null, methodGroup.ResultKind, diagnostics)
{
}

public MethodGroupResolution(Symbol otherSymbol, LookupResultKind resultKind, ImmutableBindingDiagnostic<AssemblySymbol> diagnostics)
public MethodGroupResolution(Symbol otherSymbol, LookupResultKind resultKind, ReadOnlyBindingDiagnostic<AssemblySymbol> diagnostics)
: this(methodGroup: null, otherSymbol, overloadResolutionResult: null, analyzedArguments: null, resultKind, diagnostics)
{
}
Expand All @@ -40,7 +40,7 @@ public MethodGroupResolution(
OverloadResolutionResult<MethodSymbol> overloadResolutionResult,
AnalyzedArguments analyzedArguments,
LookupResultKind resultKind,
ImmutableBindingDiagnostic<AssemblySymbol> diagnostics)
ReadOnlyBindingDiagnostic<AssemblySymbol> diagnostics)
{
Debug.Assert((methodGroup == null) || (methodGroup.Methods.Count > 0));
Debug.Assert((methodGroup == null) || ((object)otherSymbol == null));
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/SwitchBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ protected BoundExpression SwitchGoverningExpression

protected TypeSymbol SwitchGoverningType => SwitchGoverningExpression.Type;

protected ImmutableBindingDiagnostic<AssemblySymbol> SwitchGoverningDiagnostics
protected ReadOnlyBindingDiagnostic<AssemblySymbol> SwitchGoverningDiagnostics
{
get
{
EnsureSwitchGoverningExpressionAndDiagnosticsBound();
return new ImmutableBindingDiagnostic<AssemblySymbol>(_switchGoverningDiagnostics, _switchGoverningDependencies);
return new ReadOnlyBindingDiagnostic<AssemblySymbol>(_switchGoverningDiagnostics, _switchGoverningDependencies);
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -640,11 +640,11 @@ public override bool SuppressVirtualCalls
{
public Symbol ValueSymbol { get; }
public BoundExpression ValueExpression { get; }
public ImmutableBindingDiagnostic<AssemblySymbol> ValueDiagnostics { get; }
public ReadOnlyBindingDiagnostic<AssemblySymbol> ValueDiagnostics { get; }
public BoundExpression TypeExpression { get; }
public ImmutableBindingDiagnostic<AssemblySymbol> TypeDiagnostics { get; }
public ReadOnlyBindingDiagnostic<AssemblySymbol> TypeDiagnostics { get; }

public BoundTypeOrValueData(Symbol valueSymbol, BoundExpression valueExpression, ImmutableBindingDiagnostic<AssemblySymbol> valueDiagnostics, BoundExpression typeExpression, ImmutableBindingDiagnostic<AssemblySymbol> typeDiagnostics)
public BoundTypeOrValueData(Symbol valueSymbol, BoundExpression valueExpression, ReadOnlyBindingDiagnostic<AssemblySymbol> valueDiagnostics, BoundExpression typeExpression, ReadOnlyBindingDiagnostic<AssemblySymbol> typeDiagnostics)
{
Debug.Assert(valueSymbol != null, "Field 'valueSymbol' cannot be null (use Null=\"allow\" in BoundNodes.xml to remove this check)");
Debug.Assert(valueExpression != null, "Field 'valueExpression' cannot be null (use Null=\"allow\" in BoundNodes.xml to remove this check)");
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
<ValueType Name="BoundLocalDeclarationKind"/>
<ValueType Name="NullableAnnotation"/>
<ValueType Name="ErrorCode"/>
<ValueType Name="ImmutableBindingDiagnostic"/>
<ValueType Name="ReadOnlyBindingDiagnostic"/>
<ValueType Name="InterpolatedStringHandlerData"/>
<ValueType Name="WellKnownMember"/>
<ValueType Name="CollectionExpressionTypeKind"/>
Expand Down Expand Up @@ -2253,7 +2253,7 @@
<Field Name="Symbol" Type="LambdaSymbol" Null="disallow"/>
<Field Name="Type" Type="TypeSymbol?" Override="true"/>
<Field Name="Body" Type="BoundBlock"/>
<Field Name="Diagnostics" Type="ImmutableBindingDiagnostic&lt;AssemblySymbol&gt;"/>
<Field Name="Diagnostics" Type="ReadOnlyBindingDiagnostic&lt;AssemblySymbol&gt;"/>
<Field Name="Binder" Type="Binder" Null="disallow" />
</Node>

Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ internal sealed partial class BoundLambda : IBoundLambdaOrFunction

SyntaxNode IBoundLambdaOrFunction.Syntax { get { return Syntax; } }

public BoundLambda(SyntaxNode syntax, UnboundLambda unboundLambda, BoundBlock body, ImmutableBindingDiagnostic<AssemblySymbol> diagnostics, Binder binder, TypeSymbol? delegateType, InferredLambdaReturnType inferredReturnType)
public BoundLambda(SyntaxNode syntax, UnboundLambda unboundLambda, BoundBlock body, ReadOnlyBindingDiagnostic<AssemblySymbol> diagnostics, Binder binder, TypeSymbol? delegateType, InferredLambdaReturnType inferredReturnType)
: this(syntax, unboundLambda.WithNoCache(), (LambdaSymbol)binder.ContainingMemberOrLambda!, body, diagnostics, binder, delegateType)
{
InferredReturnType = inferredReturnType;
Expand Down Expand Up @@ -1295,7 +1295,7 @@ public bool GenerateSummaryErrors(BindingDiagnosticBag diagnostics)
var allBags = convBags.Concat(retBags);

FirstAmongEqualsSet<Diagnostic>? intersection = null;
foreach (ImmutableBindingDiagnostic<AssemblySymbol> bag in allBags)
foreach (ReadOnlyBindingDiagnostic<AssemblySymbol> bag in allBags)
{
if (intersection == null)
{
Expand All @@ -1318,7 +1318,7 @@ public bool GenerateSummaryErrors(BindingDiagnosticBag diagnostics)

FirstAmongEqualsSet<Diagnostic>? union = null;

foreach (ImmutableBindingDiagnostic<AssemblySymbol> bag in allBags)
foreach (ReadOnlyBindingDiagnostic<AssemblySymbol> bag in allBags)
{
if (union == null)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2402,7 +2402,7 @@ internal override bool IsCompilerGenerated
get { return true; }
}

internal override ImmutableBindingDiagnostic<AssemblySymbol> GetConstantValueDiagnostics(BoundExpression boundInitValue)
internal override ReadOnlyBindingDiagnostic<AssemblySymbol> GetConstantValueDiagnostics(BoundExpression boundInitValue)
{
throw new NotImplementedException();
}
Expand Down
22 changes: 11 additions & 11 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1812,7 +1812,7 @@ internal EntryPoint GetEntryPointAndDiagnostics(CancellationToken cancellationTo

if (entryPoint is null)
{
ImmutableBindingDiagnostic<AssemblySymbol> diagnostics;
ReadOnlyBindingDiagnostic<AssemblySymbol> diagnostics;
var entryPointMethod = FindEntryPoint(simpleProgramEntryPointSymbol, cancellationToken, out diagnostics);
entryPoint = new EntryPoint(entryPointMethod, diagnostics);
}
Expand All @@ -1822,7 +1822,7 @@ internal EntryPoint GetEntryPointAndDiagnostics(CancellationToken cancellationTo
var diagnostics = DiagnosticBag.GetInstance();
diagnostics.Add(ErrorCode.ERR_SimpleProgramDisallowsMainType, NoLocation.Singleton);
entryPoint = new EntryPoint(entryPoint.MethodSymbol,
new ImmutableBindingDiagnostic<AssemblySymbol>(
new ReadOnlyBindingDiagnostic<AssemblySymbol>(
entryPoint.Diagnostics.Diagnostics.Concat(diagnostics.ToReadOnlyAndFree()), entryPoint.Diagnostics.Dependencies));
}
}
Expand All @@ -1833,7 +1833,7 @@ internal EntryPoint GetEntryPointAndDiagnostics(CancellationToken cancellationTo
return _lazyEntryPoint;
}

private MethodSymbol? FindEntryPoint(MethodSymbol? simpleProgramEntryPointSymbol, CancellationToken cancellationToken, out ImmutableBindingDiagnostic<AssemblySymbol> sealedDiagnostics)
private MethodSymbol? FindEntryPoint(MethodSymbol? simpleProgramEntryPointSymbol, CancellationToken cancellationToken, out ReadOnlyBindingDiagnostic<AssemblySymbol> sealedDiagnostics)
{
var diagnostics = BindingDiagnosticBag.GetInstance();
RoslynDebug.Assert(diagnostics.DiagnosticBag is object);
Expand Down Expand Up @@ -2176,11 +2176,11 @@ internal override bool IsUnreferencedAssemblyIdentityDiagnosticCode(int code)
internal class EntryPoint
{
public readonly MethodSymbol? MethodSymbol;
public readonly ImmutableBindingDiagnostic<AssemblySymbol> Diagnostics;
public readonly ReadOnlyBindingDiagnostic<AssemblySymbol> Diagnostics;

public static readonly EntryPoint None = new EntryPoint(null, ImmutableBindingDiagnostic<AssemblySymbol>.Empty);
public static readonly EntryPoint None = new EntryPoint(null, ReadOnlyBindingDiagnostic<AssemblySymbol>.Empty);

public EntryPoint(MethodSymbol? methodSymbol, ImmutableBindingDiagnostic<AssemblySymbol> diagnostics)
public EntryPoint(MethodSymbol? methodSymbol, ReadOnlyBindingDiagnostic<AssemblySymbol> diagnostics)
{
this.MethodSymbol = methodSymbol;
this.Diagnostics = diagnostics;
Expand Down Expand Up @@ -3103,7 +3103,7 @@ void registeredUsageOfUsingsInTree(SyntaxTree tree)
}
}

private ImmutableBindingDiagnostic<AssemblySymbol> GetSourceDeclarationDiagnostics(SyntaxTree? syntaxTree = null, TextSpan? filterSpanWithinTree = null, Func<IEnumerable<Diagnostic>, SyntaxTree, TextSpan?, IEnumerable<Diagnostic>>? locationFilterOpt = null, CancellationToken cancellationToken = default)
private ReadOnlyBindingDiagnostic<AssemblySymbol> GetSourceDeclarationDiagnostics(SyntaxTree? syntaxTree = null, TextSpan? filterSpanWithinTree = null, Func<IEnumerable<Diagnostic>, SyntaxTree, TextSpan?, IEnumerable<Diagnostic>>? locationFilterOpt = null, CancellationToken cancellationToken = default)
{
UsingsFromOptions.Complete(this, cancellationToken);

Expand Down Expand Up @@ -3139,12 +3139,12 @@ private ImmutableBindingDiagnostic<AssemblySymbol> GetSourceDeclarationDiagnosti
}

// NOTE: Concatenate the CLS diagnostics *after* filtering by tree/span, because they're already filtered.
ImmutableBindingDiagnostic<AssemblySymbol> clsDiagnostics = GetClsComplianceDiagnostics(syntaxTree, filterSpanWithinTree, cancellationToken);
ReadOnlyBindingDiagnostic<AssemblySymbol> clsDiagnostics = GetClsComplianceDiagnostics(syntaxTree, filterSpanWithinTree, cancellationToken);

return new ImmutableBindingDiagnostic<AssemblySymbol>(result.AsImmutable().Concat(clsDiagnostics.Diagnostics), clsDiagnostics.Dependencies);
return new ReadOnlyBindingDiagnostic<AssemblySymbol>(result.AsImmutable().Concat(clsDiagnostics.Diagnostics), clsDiagnostics.Dependencies);
}

private ImmutableBindingDiagnostic<AssemblySymbol> GetClsComplianceDiagnostics(SyntaxTree? syntaxTree, TextSpan? filterSpanWithinTree, CancellationToken cancellationToken)
private ReadOnlyBindingDiagnostic<AssemblySymbol> GetClsComplianceDiagnostics(SyntaxTree? syntaxTree, TextSpan? filterSpanWithinTree, CancellationToken cancellationToken)
{
if (syntaxTree != null)
{
Expand All @@ -3164,7 +3164,7 @@ private ImmutableBindingDiagnostic<AssemblySymbol> GetClsComplianceDiagnostics(S

Debug.Assert(!_lazyClsComplianceDependencies.IsDefault);
Debug.Assert(!_lazyClsComplianceDiagnostics.IsDefault);
return new ImmutableBindingDiagnostic<AssemblySymbol>(_lazyClsComplianceDiagnostics, _lazyClsComplianceDependencies);
return new ReadOnlyBindingDiagnostic<AssemblySymbol>(_lazyClsComplianceDiagnostics, _lazyClsComplianceDependencies);
}

private static IEnumerable<Diagnostic> FilterDiagnosticsByLocation(IEnumerable<Diagnostic> diagnostics, SyntaxTree tree, TextSpan? filterSpanWithinTree)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1088,7 +1088,7 @@ methodSymbol is SynthesizedPrimaryConstructor ||
try
{
bool diagsWritten;
actualDiagnostics = new ImmutableBindingDiagnostic<AssemblySymbol>(sourceMethod.SetDiagnostics(actualDiagnostics.Diagnostics, out diagsWritten), actualDiagnostics.Dependencies);
actualDiagnostics = new ReadOnlyBindingDiagnostic<AssemblySymbol>(sourceMethod.SetDiagnostics(actualDiagnostics.Diagnostics, out diagsWritten), actualDiagnostics.Dependencies);

if (diagsWritten && !methodSymbol.IsImplicitlyDeclared && _compilation.EventQueue != null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public override bool Equals(Symbol obj, TypeCompareKind compareKind)
public override RefKind RefKind => RefKind.None;
internal override SynthesizedLocalKind SynthesizedKind => throw ExceptionUtilities.Unreachable();
internal override ConstantValue GetConstantValue(SyntaxNode node, LocalSymbol inProgress, BindingDiagnosticBag diagnostics = null) => null;
internal override ImmutableBindingDiagnostic<AssemblySymbol> GetConstantValueDiagnostics(BoundExpression boundInitValue) => ImmutableBindingDiagnostic<AssemblySymbol>.Empty;
internal override ReadOnlyBindingDiagnostic<AssemblySymbol> GetConstantValueDiagnostics(BoundExpression boundInitValue) => ReadOnlyBindingDiagnostic<AssemblySymbol>.Empty;
internal override SyntaxNode GetDeclaratorSyntax() => throw ExceptionUtilities.Unreachable();
internal override bool HasSourceLocation => false;
internal override LocalSymbol WithSynthesizedLocalKindAndSyntax(
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading