Compile-time variable imports #11657
Conversation
…would include unexportable symbols
…e names are not valid Bicep identifiers)
# Conflicts: # src/Bicep.Core/Semantics/ImportedTypeSymbol.cs # src/Bicep.Core/Semantics/WildcardImportSymbol.cs # src/Bicep.LangServer/Handlers/BicepHoverHandler.cs
| { | ||
| AssertSyntaxType(originalSymbolName, nameof(originalSymbolName), typeof(StringSyntax), typeof(IdentifierSyntax), typeof(SkippedTriviaSyntax)); |
There was a problem hiding this comment.
What's the use case for supporting a string in the originalSymbolName field?
There was a problem hiding this comment.
Names in ARM templates don't have to be valid Bicep identifiers. You can have and use a variable named '@!$^%#, and there are only a few ways we could deal with that when importing from a compiled template:
- Raise a diagnostic on the
importstatement indicating that the imported template has issues, even though it doesn't violate any of ARM's rules. - Ignore any import whose name isn't a valid identifier in Bicep.
- Allow
importstatements to use a string for the original symbol name but require that such imports alias the symbol to a valid identifier (e.g.,import {'a-b' as ab} from 'template.json'.
I didn't feel great about option 2 because it silently drops exported symbols without explaining why to the user. Option 1 has a couple problems that made me think it's not a viable solution:
- There's no guarantee that the author of the template with the
importstatement can edit the imported artifact (they may be importing a template spec or registry module to which they have read privileges only). - The user will have to search the imported template for the problem, since the diagnostic will be associated with the
importstatement in the consuming template rather than the syntax that exports the symbol from the offending template.
I didn't see any drawbacks to option 3 besides some additional complexity in the parser. Similar syntax is supported in js imports, so there was a good example to follow.
| _ => throw new InvalidOperationException($"Unrecognized ARM LanguageExpression of type {expression.GetType().Name} encountered."), | ||
| }; | ||
|
|
||
| private static string SerializeExpression(FunctionExpression functionExpression) |
There was a problem hiding this comment.
Is there anything in the deployments engine to do this?
There was a problem hiding this comment.
Not that I could find, although there probably should be. I can open a PR to copy this code into a public method in one of the Azure.Deployments. nuget packages, but removing this code from Bicep would need to happen in a future PR.
| return collector.symbolsReferenced; | ||
| } | ||
|
|
||
| internal static IEnumerable<DeclaredSymbol> CollectSymbolsReferencedRecursive(IBinder binder, DeclaredSymbol symbol) |
There was a problem hiding this comment.
I'm surprised this functionality doesn't already exist in the codebase!
There was a problem hiding this comment.
We do something very similar in CyclicCheckVisitor, but I wasn't able to reuse that code path since there are some special cases where we permit syntactic cycles.
jeskew
force-pushed
the
jeskew/variable-imports
branch
from
96e1964
to
c76b8c2
Compare
# Conflicts: # src/Bicep.Core/Emit/CompileTimeImports/ImportClosureInfo.cs # src/Bicep.Core/Semantics/ImportedTypeSymbol.cs # src/Bicep.Core/Semantics/SemanticDiagnosticVisitor.cs # src/Bicep.Core/Semantics/WildcardImportSymbol.cs # src/Bicep.Core/Workspaces/SourceFileGrouping.cs # src/Bicep.LangServer/Completions/BicepCompletionProvider.cs # src/Bicep.LangServer/Handlers/BicepDefinitionHandler.cs # src/Bicep.LangServer/Handlers/BicepHoverHandler.cs
# Conflicts: # src/Bicep.Core/Semantics/ArmTemplateSemanticModel.cs # src/Bicep.Core/Semantics/ISemanticModel.cs # src/Bicep.Core/Semantics/SemanticModel.cs # src/Bicep.Core/Semantics/TemplateSpecSemanticModel.cs # src/Bicep.Core/Workspaces/SourceFileGrouping.cs
|
Test this change out locally with the following install scripts (Action run 6198858998) VSCode
Azure CLI
|
Partially addresses #10121
This PR updates the
compileTimeImportsexperimental feature to support variables in addition to type declarations.varstatements can be the target of an@export()decorator (provided that they only contain references to other variables and not to resources, modules, or parameters), and an imported variable can be used just like one declared in the importing template.Some aspects of this feature that should get special scrutiny:
TemplateWriter), so there is room to improve efficiency"copy"property to aforloop expression is not perfectly roundtrippable:copyIndex(<var name>, 1)will be transformed toadd(copyIndex(<var name>), 1)countcopy property will be transformed tolength(range(0, X)), whereXis the originalcountARM expression.Microsoft Reviewers: Open in CodeFlow