Do not resolve diagnostics early during attribute binding #71140
Conversation
We were attempt 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 avoided binding diagnostics in this scenario by passing around a BindingDiagnosticBag, rather than an `ImmutableBindingDiagnostic<AssemblySymbol>`. Doing this does require some limited and controlled mutation in the tree; to help ensure future maintainability, the mutation only occurs in one direction (the binder taking ownership of the bag). However, if we're uncomfortable with the mutation approach, we can instead simply leak the BindingDiagnosticBags in this scenario. This is a small enough source of them that we're unlikely to see major issues not pooling them. Fixes dotnet#71039.
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| @@ -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); | |||
There was a problem hiding this comment.
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.
| { | ||
| if (d.Code == (int)ErrorCode.WRN_PrimaryConstructorParameterIsShadowedAndNotPassedToBase && |
There was a problem hiding this comment.
.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.
|
Does VB have similar problem? |
|
Done with review pass (commit 1) |
…gs, we instead construct an ImmutableBindingDiagnostic with unresolved Diagnostics.
…e clearly communicate that it may have lazily-resolved diagnostics in it.
|
@AlekseyTs addressed your feedback. I would recommend reviewing the two new commits separately: I put the rename into a commit of its own with no other changes for easier review. |
|
It looks like VB build is broken |
|
Done with review pass (commit 4). There was no response to the following threads: #71140 (comment), #71140 (comment). |
|
@AlekseyTs feedback addressed. |
| @@ -960,7 +960,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols | |||
| Dim displayName As String = TryCast(attrData.CommonConstructorArguments(0).ValueInternal, String) | |||
|
|
|||
| If displayName Is Nothing Then | |||
| diagnostics.Add(ERRID.ERR_FriendAssemblyNameInvalid, If(nodeOpt IsNot Nothing, nodeOpt.GetLocation(), NoLocation.Singleton), displayName) | |||
| diagnostics.Add(ERRID.ERR_FriendAssemblyNameInvalid, If(nodeOpt IsNot Nothing, nodeOpt.GetLocation(), NoLocation.Singleton), "") | |||
There was a problem hiding this comment.
This change wasn't a cycle, we just clearly didn't have any tests covering it, as it is guaranteed to hit a Debug.Assert.
|
@dotnet/roslyn-compiler for a second review please. |
AlekseyTs
dismissed
their stale review
It looks like an additional follow up is needed
This place doesn't even have to use In reply to: 1854473186 Refers to: src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs:3053 in 47f13c0. [](commit_id = 47f13c0, deletion_comment = False) |
…dFree`" This reverts commit 90b2784.
|
@AlekseyTs reverted the broader resolution change after our discussion. |
|
@dotnet/roslyn-compiler for a second review please |
src/Compilers/Core/Portable/Emit/EditAndContinue/SymbolChanges.cs
Outdated
Show resolved
Hide resolved
ghost
added this to the 
We were attempt 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
ImmutableBindingDiagnosticsto avoid eagerly resolving diagnostics during construction. To better convey this new contract, I've also renamed the type toReadOnlyBindingDiagnostics. As there are extensive uses ofReadOnlyBindingDiagnostics, and many should continue to eagerly resolve diagnostics, I've left it as resolving by default, and only adjusted this one codepath. Fixes #71039.