Do not resolve diagnostics early during attribute binding (#71140)

We were attempting to resolve diagnostics in ColorColor attribute argument scenarios while we were in the middle of attribute argument binding. This then caused us to need to bind attributes to resolve the diagnostic, entering an infinite loop that eventually overflows. To solve this, I've allowed `ImmutableBindingDiagnostics` to avoid eagerly resolving diagnostics during construction. To better convey this new contract, I've also renamed the type to `ReadOnlyBindingDiagnostics`. As there are extensive uses of `ReadOnlyBindingDiagnostics`, and many should continue to eagerly resolve diagnostics, I've left it as resolving by default, and only adjusted this one codepath. Fixes #71039.
dotnet · Dec 15, 2023 · 38f6bcf · 38f6bcf
1 parent 0a8c34e
commit 38f6bcf
Show file tree

Hide file tree

Showing 47 changed files with 265 additions and 143 deletions.
diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
@@ -6857,7 +6857,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);
+                                    new BoundTypeOrValueData(leftSymbol, boundValue, valueDiagnostics.ToReadOnlyAndFree(forceDiagnosticResolution: false), boundType, typeDiagnostics.ToReadOnlyAndFree(forceDiagnosticResolution: false)), leftType);
                             }
 
                             typeDiagnostics.Free();
@@ -9738,7 +9738,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);

diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
@@ -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 &&
+                            // 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 &&
                                 !(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);
                             }
                         }

diff --git a/src/Compilers/CSharp/Portable/Binder/LockOrUsingBinder.cs b/src/Compilers/CSharp/Portable/Binder/LockOrUsingBinder.cs
@@ -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;

diff --git a/src/Compilers/CSharp/Portable/Binder/MethodGroupResolution.cs b/src/Compilers/CSharp/Portable/Binder/MethodGroupResolution.cs
@@ -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)
         {
         }
@@ -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));

diff --git a/src/Compilers/CSharp/Portable/Binder/SwitchBinder.cs b/src/Compilers/CSharp/Portable/Binder/SwitchBinder.cs
@@ -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);
             }
         }
 

diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs
@@ -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)");

diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml b/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
@@ -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"/>
@@ -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>
 

diff --git a/src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs b/src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs
@@ -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;
@@ -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)
                 {
@@ -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)
                 {

diff --git a/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs b/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs
@@ -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();
         }

diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
@@ -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);
                     }
@@ -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));
                     }
                 }
@@ -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);
@@ -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;
@@ -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);
 
@@ -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)
             {
@@ -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)

diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
@@ -1094,7 +1094,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)
                         {

diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.PlaceholderLocal.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.PlaceholderLocal.cs
@@ -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(

diff --git a/src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs b/src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs