Avoid cycles in constant initializer #75366
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| { | ||
| var source = """ | ||
| const int x = | ||
| checked(x); |
There was a problem hiding this comment.
Note that the issue is not just with switch expressions. I added this test to demonstrate that. The stack overflow can happen anywhere we get a binder using this.GetBinder(node) because then we lose the current LocalInProgressBinder which the cycle detection was relying on.
| @@ -574,7 +574,7 @@ private void MakeConstantTuple(LocalSymbol inProgress, BoundExpression boundInit | |||
|
|
|||
| internal override ConstantValue GetConstantValue(SyntaxNode node, LocalSymbol inProgress, BindingDiagnosticBag diagnostics = null) | |||
| { | |||
| if (this.IsConst && inProgress == this) | |||
| if (this.IsConst && (inProgress == this || this.ForbiddenZone?.Contains(node) == true)) | |||
There was a problem hiding this comment.
This will catch a direct reference to x in the initializer for const x. Is there also a problem with an indirect reference: const int x = y; const int y = ...x...;?
There was a problem hiding this comment.
Could not reproduce that failing even in main. Added tests.
| public void ConstCycle_SwitchExpression_MutuallyReferencing() | ||
| { | ||
| var source = """ | ||
| const int y = x switch { }; |
| @@ -574,7 +574,7 @@ private void MakeConstantTuple(LocalSymbol inProgress, BoundExpression boundInit | |||
|
|
|||
| internal override ConstantValue GetConstantValue(SyntaxNode node, LocalSymbol inProgress, BindingDiagnosticBag diagnostics = null) | |||
| { | |||
| if (this.IsConst && inProgress == this) | |||
| if (this.IsConst && (inProgress == this || this.ForbiddenZone?.Contains(node) == true)) | |||
There was a problem hiding this comment.
While, perhaps, this change prevents the stack overflow for the specific scenario, I think we should be addressing the root cause of the problem, i.e. the binder hierarchy used to perform binding.
I am fine if we proceed with this change as a quick, low risk, temporary fix. But we should follow-up on the issue around binder hierarchy.
Fixes #75353.