Add this.exists() and this.existingResource() as an experimental feature#17727
Add this.exists() and this.existingResource() as an experimental feature#17727tsmallig33 merged 20 commits intomainfrom
Conversation
|
Test this change out locally with the following install scripts (Action run 19446115486) VSCode
Azure CLI
|
Dotnet Test Results 102 files - 51 102 suites - 51 43m 28s ⏱️ - 20m 26s Results for commit b5b4fc6. ± Comparison against base commit c1d5325. This pull request removes 1962 and adds 678 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces this().exists as an experimental feature for Bicep, allowing developers to conditionally modify resource properties based on whether the resource already exists during deployment. The feature requires explicit enablement through experimental flags and is restricted to resource property contexts only.
Key changes:
- Adds a new experimental feature flag
thisExistsFunctionwith configuration support - Implements the
this()function that returns an object with anexistsproperty - Restricts usage to resource properties only, with proper validation and error handling
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/vscode-bicep/schemas/bicepconfig.schema.json |
Adds schema definition for the new thisExistsFunction experimental feature |
src/Bicep.Core/Semantics/Namespaces/AzNamespaceType.cs |
Implements the this() function with context validation and return type definition |
src/Bicep.Core/Semantics/Namespaces/NamespaceProvider.cs |
Passes feature provider to namespace creation for conditional function registration |
src/Bicep.Core/Intermediate/ |
Adds new ThisFunctionExpression type and visitor pattern support |
src/Bicep.Core/Emit/ExpressionConverter.cs |
Converts this().exists to ARM template targetExists() function |
src/Bicep.Core/Features/ |
Adds feature flag support across the feature provider infrastructure |
src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs |
Adds error message for invalid usage contexts |
src/Bicep.Core/Configuration/ExperimentalFeaturesEnabled.cs |
Extends configuration to include the new experimental feature |
| Tests and documentation files | Comprehensive test coverage and documentation for the new feature |
Comments suppressed due to low confidence (1)
tsmallig33
changed the title
tsmallig33
changed the title
| .Where(decl => decl.NameSource.IsValid && this.builtInNamespaces.ContainsKey(decl.Name)) | ||
| .Select(reservedSymbol => DiagnosticBuilder.ForPosition(reservedSymbol.NameSource).SymbolicNameCannotUseReservedNamespaceName(reservedSymbol.Name, this.builtInNamespaces.Keys))); | ||
|
|
||
| if (this.features.ThisNamespaceEnabled && scope is FileSymbol) |
There was a problem hiding this comment.
Is the scope type check meant to avoid raising a diagnostic on the this namespace symbol? I think this would miss cases where for expression introduces a local variable named this, as in:
resource foo 'type@version' = [for this in range(0, 10): { ... }]
There was a problem hiding this comment.
Yes, without the scope check, the diagnostic be raised when 'this' is declared inside resource scopes. I tested locally with for loops and found that defining [for this... would take precedence over the 'this' namespace so shouldn't need the diagnostics. I will add a unit test to cover this unless there are other scopes you can think of where this would miss the validation.
| // If it's a function call and no direct match found, check within LocalThisNamespaceSymbol | ||
| if (isFunctionCall) | ||
| { | ||
| var thisNamespace = scope.Declarations | ||
| .OfType<LocalThisNamespaceSymbol>() | ||
| .FirstOrDefault(); | ||
|
|
||
| if (thisNamespace?.TryGetNamespaceType() is NamespaceType namespaceType) | ||
| { | ||
| return namespaceType.MethodResolver.TryGetSymbol(identifierSyntax); | ||
| } | ||
| } |
There was a problem hiding this comment.
Probably out of scope for this PR, but I think there's some technical debt here that may complicate other features in this that have been discussed (like a this.parent property for syntactic child resources). Ideally, Bicep should bind to the matching unqualified symbol if one exists. For example, in the following:
func exists() bool => false
resource foo 'type@version' = {
name: 'foo'
properties: {
bar: exists() ? 'fizz' : 'buzz'
}
}the exists() function call should bind to the user-defined function even though it's within a resource body because template authors can fully qualify this.exists() to resolve ambiguity, but there's no global::exists()-type syntax to resolve the ambiguity in the other direction.
I would say that imported functions probably should work the same way (i.e., be callable unqualified unless ambiguous, in which case the file-local symbol should take precedence), and they currently do not, so it might make more sense to address this more generically instead of special-casing the this namespace when defined.
A few options here:
- Leave this section as is with a note to address before
thisis GA. - Back out the change here and require authors to call
this.exists()instead ofexists()
I can put a topic in the discussions queue to get more input. In any case, I don't think that should block this PR.
There was a problem hiding this comment.
Removed this section for now, which will require fully qualified access to this.exists() and this.existingResource()
There was a problem hiding this comment.
Out of curiosity, why is a new kind of symbol needed (instead of reusing BuiltInNamespaceSymbol)? I see that LocalThisNamespaceSymbol also keeps a reference to the declaring resource but may have missed where that is used.
There was a problem hiding this comment.
The BuiltInNamespaceSymbol is not a DeclaredSymbol, so LocalThisNamespaceSymbol is closer to LocalVariableSymbol and is used to provide the correct ThisNamespaceType as the DeclaredSymbol type.
There was a problem hiding this comment.
Why does it need to be a DeclaredSymbol? this is never declared (unlike local variables) but is implicitly available (like az and sys).
There was a problem hiding this comment.
So it can be declared in the resource body scope:
Is there another way you would recommend adding the symbol to the resource body scope?
There was a problem hiding this comment.
Got it, scopes can only contain DeclaredSymbol instances. It is odd that you have to declare a fake name source that maps back to a keyword rather than a name, but I guess there is no way around it.
I think this is tied to my comment on NameBindingVisitor; symbol resolution right now is relying on some assumptions that have gotten less true over time, and I think it's worth revisiting the design to see how to make it more flexible. Definitely out of scope for this PR.
Description
Adds this.exists() for use in resource properties.
Thisnamespace is scoped as a local symbol to resource bodiesExample Usage
Checklist
Microsoft Reviewers: Open in CodeFlow