Loops - code generation and emit limit checks #1521
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1521 +/- ##
==========================================
+ Coverage 94.94% 95.05% +0.10%
==========================================
Files 356 362 +6
Lines 19403 19804 +401
Branches 14 13 -1
==========================================
+ Hits 18422 18824 +402
+ Misses 981 980 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
majastrz
force-pushed
the
majastrz/go-round-and-round
branch
3 times, most recently
from
ac95b81 to
81237ac
Compare
majastrz
force-pushed
the
majastrz/go-round-and-round
branch
5 times, most recently
from
956934d to
b3fe81c
Compare
majastrz
changed the title
- range() now returns int[] instead of array - better union type array formatting - loop variable completions - removed brackets from loops spec
majastrz
force-pushed
the
majastrz/go-round-and-round
branch
from
b3fe81c to
9453dc0
Compare
I originally had separate methods for single and collection resources, but pretty much everywhere I would have to check if a symbol is a collection and then call one or the other, so it made sense to collapse into one. Refers to: src/Bicep.Core/Emit/EmitHelpers.cs:14 in 9453dc0. [](commit_id = 9453dc0, deletion_comment = False) |
I didn't know what to call this. When we have resource/module symbol references via variable access, we are taking a name expression of a resource to construct a reference() or *resourceId() function expression. When the name expression contains local variables (say, a loop item or index var) and is used to construct something in another context outside the loop body like dependsOn or reference() call in some other resource, the standard replacement with [copyIndex()] produces an incorrect result. If we pretend we moved the expression into a new context, we can use the new Refers to: src/Bicep.Core/Emit/ExpressionConverter.cs:312 in 9453dc0. [](commit_id = 9453dc0, deletion_comment = False) |
This was also the least "viral" option out of a bunch that I tried. In reply to: 784865454 [](ancestors = 784865454) Refers to: src/Bicep.Core/Emit/ExpressionConverter.cs:312 in 9453dc0. [](commit_id = 9453dc0, deletion_comment = False) |
This "solution" predates my immutable converter context solution. I'm going to try to leverage this here as well to remove the hacky expression visitor. #Resolved Refers to: src/Bicep.Core/Emit/ExpressionEmitter.cs:194 in 9453dc0. [](commit_id = 9453dc0, deletion_comment = False) |
This effectively blocks directly assigning a resource/module collection to a property in a resource body without an indexer. You can use a resource/module collection with an array indexer pretty much anywhere, though. And we do allow the resource/module collection without an indexer in Refers to: src/Bicep.Core/Emit/ForSyntaxValidatorVisitor.cs:86 in 9453dc0. [](commit_id = 9453dc0, deletion_comment = False) |
| @@ -129,7 +129,7 @@ resource secret 'Microsoft.KeyVault/vaults/secrets@2019-09-01' = { | |||
| /* | |||
| TODO: Replace the first secret workaround above with this once we have loops | |||
There was a problem hiding this comment.
Can you uncomment this code now then? #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
| @@ -2,7 +2,7 @@ | |||
| "profiles": { | |||
There was a problem hiding this comment.
Should we just gitignore this file? #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
| @@ -5,7 +5,7 @@ | |||
| "detail": "dependsOn", | |||
| "documentation": { | |||
| "kind": "markdown", | |||
| "value": "Type: `resource | module[]` \nWrite-only property \n" | |||
| "value": "Type: `(module[] | resource | module | resource[])[]` \nWrite-only property \n" | |||
There was a problem hiding this comment.
The ordering is a little weird - presumably this is what's shown to the user. Can we order this somehow? #WontFix
There was a problem hiding this comment.
Should be able to. I assume you're looking for resource | resource[] | module | module[]?
In reply to: 582066499 [](ancestors = 582066499)
There was a problem hiding this comment.
Either that or resource | module | resource[] | module[] - both seem like good options. #WontFix
There was a problem hiding this comment.
It's actually more complicated than I thought because resource | module part is one member of the union. (Due to how ResourceRef type is implemented.) We can improve this later.
In reply to: 582234791 [](ancestors = 582234791)
There was a problem hiding this comment.
It's actually more complicated than I thought because
resource | modulepart is one member of the union. (Due to how ResourceRef type is implemented.) We can improve this later.In reply to: 582234791 [](ancestors = 582234791)
So it's really ((resource | module) | resource[] | module[]) flattened into one union? #Resolved
There was a problem hiding this comment.
Yeah unfortunately. I really wish we didn't do it that way now :(
In reply to: 582331000 [](ancestors = 582331000)
There was a problem hiding this comment.
I think I'll add a special case to enclose it in parentheses for now.
In reply to: 582332560 [](ancestors = 582332560,582331000)
|
|
||
| private HashSet<LocalVariableSymbol> SymbolDependencies { get; } = new(); | ||
|
|
||
| public static ISet<LocalVariableSymbol> GetLocalSymbolDependencies(SemanticModel semanticModel, SyntaxBase syntax) |
There was a problem hiding this comment.
Worth making immutable? #Resolved
There was a problem hiding this comment.
| @@ -792,7 +792,7 @@ public ErrorDiagnostic ArgumentCountMismatch(int argumentCount, int mininumArgum | |||
| public ErrorDiagnostic LoopsNotSupported() => new( | |||
There was a problem hiding this comment.
[nit] if we're calling them for-expressions in the message, then would be good to update the name of this function #Resolved
| } | ||
|
|
||
| public LanguageExpression GetFullyQualifiedResourceId(ModuleSymbol moduleSymbol) | ||
| public LanguageExpression GetFullyQualifiedResourceId(ModuleSymbol moduleSymbol, SyntaxBase newContext) |
There was a problem hiding this comment.
Is newContext used inside ScopeHelper.FormatFullyQualifiedResourceId()? Seemed like it was being passed recursively but I couldn't see other usages. #Resolved
There was a problem hiding this comment.
Good call! I was able to remove a few occurrences of it. #Resolved
| // this is a giant hack | ||
| // the named copy index in the serialized expression is incorrect | ||
| // because the object syntax here does not match the JSON equivalent due to the presence of { "value": ... } wrappers | ||
| // for now, we will manually replace the copy index in the converted expression | ||
| // this also will not work for nested property loops |
There was a problem hiding this comment.
What's the less-hacky solution? Will you create an issue for it? #Resolved
There was a problem hiding this comment.
Will try it in this PR - basically use the local symbol replacement logic to do it instead of the ExpressionVisitor.
In reply to: 582201312 [](ancestors = 582201312)
There was a problem hiding this comment.
Decided to leave as is for now. Will have to revisit when we support nested property loops.
In reply to: 582207976 [](ancestors = 582207976,582201312)
| if (propertyLookup.Contains(true)) | ||
| { | ||
| // we have properties whose value is a for-expression | ||
| this.EmitProperty("copy", () => |
There was a problem hiding this comment.
Do we block conversion of properties in Bicep named "copy"? Is there any option for codegen if we have a property named "copy"? (I guess that's somewhat orthogonal to this PR) #Resolved
There was a problem hiding this comment.
Good catch! Expressions are supported in the name property of a property copy loop, so I could emit name: format('copy'), but will need to verify that it works.
In reply to: 582202923 [](ancestors = 582202923)
There was a problem hiding this comment.
Actually, I realized the inverse is the problem. A property copy loop for a property called copy should probably be ok because name on the copy object could be set to "copy", but a regular resource with a property called "copy" is going to cause problems. That is a preexisting problem, though.
In reply to: 582219558 [](ancestors = 582219558,582202923)
| this.EmitPropertyWithTransform( | ||
| "count", | ||
| syntax.Expression, | ||
| arrayExpression => new FunctionExpression("length", new[] { arrayExpression }, Array.Empty<LanguageExpression>())); |
| if (body is IfConditionSyntax ifCondition) | ||
| { | ||
| body = ifCondition.Body; | ||
| emitter.EmitProperty("condition", ifCondition.ConditionExpression); | ||
| } | ||
| else if (body is ForSyntax @for) | ||
| { | ||
| body = @for.Body; | ||
| emitter.EmitProperty("copy", () => emitter.EmitCopyObject(resourceSymbol.Name, @for, input: null)); | ||
| } |
There was a problem hiding this comment.
Can't body be both for & if? #Resolved
There was a problem hiding this comment.
Not right now. That's one of the things that is coming right after 0.3. Forgot to mention it in the description.
In reply to: 582212603 [](ancestors = 582212603)
There was a problem hiding this comment.
No, never mind. It's in the description (the "no filtering" bit).
In reply to: 582231890 [](ancestors = 582231890,582212603)
| @@ -18,7 +18,7 @@ public static class BaselineHelper | |||
|
|
|||
| private const string SetBaseLineSettingName = "SetBaseLine"; | |||
|
|
|||
| public static bool ShouldSetBaseline(TestContext testContext) => | |||
| public static bool ShouldSetBaseline(TestContext testContext) => | |||
There was a problem hiding this comment.
[nit] Trailing whitespace. #Resolved
There was a problem hiding this comment.
Heh. I've been adding true || to make the tests run in baseline update mode in VS. Will remove in next commit.
In reply to: 582214047 [](ancestors = 582214047)
|
|
||
| namespace Bicep.Core.Semantics | ||
| { | ||
| public class SymbolHierarchy |
There was a problem hiding this comment.
What does it mean for a symbol to be a parent/child of another?
There was a problem hiding this comment.
It's a hierarchy. FileSymbol is a parent of all the global declarations, for example.
In reply to: 582221724 [](ancestors = 582221724)
There was a problem hiding this comment.
After our discussion I started to think more on this. I'm starting to view it as less a hierarchy of symbols but more of a hierarchy of semantically relevant things with some of them being symbols. I'm also starting to realize that too many things are called Symbols in the semantic model.
In reply to: 582234192 [](ancestors = 582234192,582221724)
| @@ -14,5 +14,10 @@ public TypedArrayType(ITypeReference itemReference, TypeSymbolValidationFlags va | |||
| public override ITypeReference Item { get; } | |||
|
|
|||
| public override TypeSymbolValidationFlags ValidationFlags { get; } | |||
|
|
|||
| private static string FormatTypeName(ITypeReference itemReference) => | |||
|
Should we handle the case where a variable is used as a ForSyntax body (block it or support it)? This doesn't emit any diagnostics and causes unhandled exception upon transpiling: UPDATE: #Resolved |
I ended up blocking this at the parser level for a couple of reasons:
The fix should take care of both issues. #Resolved |
|
|
||
| resource secret 'Microsoft.KeyVault/vaults/secrets@2019-09-01' = { | ||
| name: '${vault.name}/${firstSecretName}' | ||
| resource secrets 'Microsoft.KeyVault/vaults/secrets@2018-02-14' = [for secret in secretsObject.secrets: { |
There was a problem hiding this comment.
Can this be simplified to:
resource secrets 'Microsoft.KeyVault/vaults/secrets@2018-02-14' = [for secret in secretsObject.secrets: {
name: '${vault.name}/${secret.secretName}'
properties: {
value: secret.secretValue
}
}]There was a problem hiding this comment.
Will take a look in the next one.
There was a problem hiding this comment.
Took a look - yeah can totally be simplified.
Module and resource loops should now be fully functional. Property loops are supported as well but cannot be nested with other property loops due to runtime limitations. A property loop can be nested inside a resource or module loop, however. This fixes #185.
Additional changes:
resourceandmodulekeyword when using module/resource loops.range()function return type is nowint[]instead ofarray. This improves the quality of hovers when using loops with the range.[for thing in thing: thing]. (The change means that the loop variable refs inside the loop bodyObjectSyntaxcontinue binding in the loop scope, but the the array expression binds using the parent scope.)The following limitations remain:
[for (thing, index) in things: {...}]is not yet implemented. Coming after this PR.dependsOn, we are blocking references to a module/resource collection without an indexer. This should be a rare case, but will be relaxed in a future PR.