Handle more (if not all) corner cases in DTC validation #2769
Conversation
| @@ -292,22 +292,22 @@ module runtimeInvalidModule2 'empty.bicep' = { | |||
|
|
|||
| module runtimeInvalidModule3 'empty.bicep' = { | |||
| name: runtimeValidRes1.sku.name | |||
| //@[8:33) [BCP120 (Error)] The property "name" must be evaluable at the start of the deployment, and cannot depend on any values that have not yet been calculated. Accessible properties of runtimeValidRes1 are "apiVersion", "id", "name", "type". |runtimeValidRes1.sku.name| | |||
| //@[8:28) [BCP120 (Error)] The property "name" must be evaluable at the start of the deployment, and cannot depend on any values that have not yet been calculated. Accessible properties of runtimeValidRes1 are "apiVersion", "id", "name", "type". |runtimeValidRes1.sku| | |||
There was a problem hiding this comment.
The old DeployTimeConstantVisitor contains some ad-hoc logic to make sure the error on the full path. Personally I like the new error message better not only because it simplifies the code, but also essentially it is the top level property that triggers the error.
| this.diagnosticWriter.Write(DiagnosticBuilder.ForPosition(syntax.ForKeyword).VariableLoopsRuntimeDependencyNotAllowed(variableChain)); | ||
|
|
||
| break; | ||
|
|
There was a problem hiding this comment.
I removed the logic to check DTC in ForSyntax from the class because the new DTC validator now convers it, and it can produce more precise error messages.
src/Bicep.Core/TypeSystem/DeployTimeConstantContainerVisitor.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (syntax.TryGetKeyText() is { } propertyName && | ||
| this.semanticModel.Binder.GetParent(syntax) is { } objectSyntax && | ||
| this.semanticModel.TypeManager.GetDeclaredType(objectSyntax) is ObjectType objectType && |
There was a problem hiding this comment.
Does this mean we'll ignore DiscriminiatedObjectType instances? Using the assigned type (rather than declared) should have resolved discriminated object types -> object types, as long as the DTC flag has been preserved properly.
There was a problem hiding this comment.
You are right, we should also handle DiscriminiatedObjectType, but using the assigned type won't work for properties whose values contain string interpolations or array accesses, as the DTC flag is not propagated in those cases. I've updated the code block to process DiscriminiatedObjectType instances and turned it into a method of ObjectPropertySyntax.
src/Bicep.Core/TypeSystem/DeployTimeConstantContainerVisitor.cs
Outdated
Show resolved
Hide resolved
| private void FlagIfPropertyNotReadableAtDeployTime(SyntaxBase syntax, string propertyName, DeclaredSymbol accessedSymbol, ObjectType accessedBodyType) | ||
| { | ||
| if (accessedBodyType.Properties.TryGetValue(propertyName, out var propertyType) && | ||
| !propertyType.Flags.HasFlag(TypePropertyFlags.ReadableAtDeployTime)) |
There was a problem hiding this comment.
That's good to know. I was searching for details about this and I happened to find a benchmark testing HasFlag vs bitwise operations, which suggests that we don't need to do the optimization to eliminate boxing/unboxing by ourselves. The .NET team has implemented some JIT optimization to generate bitwise operations for us since .NET core 2.1, which is documented in https://devblogs.microsoft.com/dotnet/ryujit-just-in-time-compiler-optimization-enhancements/.
| //@[15:65) [BCP120 (Error)] The property "name" must be evaluable at the start of the deployment, and cannot depend on any values that have not yet been calculated. Accessible properties of moduleLoopForRuntimeCheck are "name". |moduleLoopForRuntimeCheck[1].outputs.stringOutputB| | ||
| //@[67:117) [BCP120 (Error)] The property "name" must be evaluable at the start of the deployment, and cannot depend on any values that have not yet been calculated. Accessible properties of moduleLoopForRuntimeCheck are "name". |moduleLoopForRuntimeCheck[1].outputs.stringOutputA| | ||
| //@[15:51) [BCP120 (Error)] The property "name" must be evaluable at the start of the deployment, and cannot depend on any values that have not yet been calculated. Accessible properties of moduleLoopForRuntimeCheck are "name". |moduleLoopForRuntimeCheck[1].outputs| | ||
| //@[67:103) [BCP120 (Error)] The property "name" must be evaluable at the start of the deployment, and cannot depend on any values that have not yet been calculated. Accessible properties of moduleLoopForRuntimeCheck are "name". |moduleLoopForRuntimeCheck[1].outputs| |
There was a problem hiding this comment.
I like the change to the span that the squiggly is being displayed on. Seeing it here, I wonder if there's something we can do to make this message a bit more obvious - given that the squiggly is being displayed on the value rather than the key, it's not immediately obvious that name in the message relates to the property value assignment, which could be especially hard to follow on a larger expression.
What about something like: 'This expression is being used in an assignment to the "{key}" property of the "{type}" type, which requires a value that can be calculated at the start of the deployment. Properties of "{symbol}" which can be calculated at the start include "{list}".'
I'm interested to hear other people's thoughts here as well - worth spending a bit of time getting right I think!
There was a problem hiding this comment.
Good suggestion! Created #2863 to track it. Will submit another PR to implement it.
| ? variableDeclarationSyntax.Name.IdentifierName | ||
| : null; | ||
|
|
||
| protected (DeclaredSymbol?, ObjectType?) TryExtractResourceOrModuleSymbolAndBodyType(SyntaxBase syntax, bool isCollection = false) |
There was a problem hiding this comment.
Makes sense. Removed the default value.
The PR fixes #2291 and fixes #2622.
This is basically an attempt to improve the current deploy-time constant checking logic to handle all cases. As the current
DeployTimeConstantVisitorandDeployTimeConstaintVariableVisitormaintain several states and do the validation in one pass, it becomes a bit head-scratching when trying to update it to cover more edge cases. To make the code more maintainable I split the two visitors into three:DeployTimeConstantContainerVisitorthat collects all syntaxes requiring deploy-time constant values (DTC containers)DeployTimeDirectViolationVisitorthat visits all DTC containers and validate property accesses and function callsDeployTimeIndirectViolationVisitorthat visits all DTC containers and validate variable dependencies