Block runtime property refs in runtime functions, fix a stack overflow #2623
Conversation
|
|
||
|
|
||
| this.deployTimeConstantScopeSyntax = currentDeployTimeConstantScopeSyntax; | ||
| } |
There was a problem hiding this comment.
The visitor only flags runtime property references, which is not enough. We should also block the runtime function calls at any location that requires a deploy-time constant. I've created #2622 to track this.
However, as the visitor gets a bit head-scratching, I feel like we should do a refactoring first and split the code instead of keeping adding stuffs, which will make it even less readable.
To simplify this, I think we would need 3 visitors. The first one will collect all syntaxes that requires deploy-time constants. The second one visits all the collected syntaxes and flags runtime properties and function calls. The third one also visits the collected syntaxes, but it only checks indirect runtime references in variable declarations.
Thoughts?
There was a problem hiding this comment.
I think splitting into three much simpler visitors absolutely makes sense to me.
There was a problem hiding this comment.
Sounds good! I'll start the refactoring once I'm done with other bug fixes.
| ")); | ||
|
|
||
| result.Should().HaveDiagnostics(new[] { | ||
| // TODO: change the first diagnostic once https://github.com/Azure/bicep/issues/2624 is fixed. |
There was a problem hiding this comment.
Found a bug #2624 related to list* function validation while adding this test.
shenglol
changed the title
shenglol
changed the title
Fixes #2547.
Fixes #2494.