From 0fa9cd69016d3bbb6b6789079362743b290c2ad6 Mon Sep 17 00:00:00 2001 From: Marcin Jastrzebski Date: Wed, 30 Mar 2022 18:50:07 -0700 Subject: [PATCH] fixed codegen for nested loops with complex index expressions --- .../Issue6075Tests.cs | 361 ++++++++++++++++++ src/Bicep.Core/Emit/ExpressionConverter.cs | 131 ++++++- src/Bicep.Core/Emit/ExpressionEmitter.cs | 4 +- .../Emit/ResourceDependencyVisitor.cs | 2 +- src/Bicep.Core/Emit/SymbolReplacer.cs | 63 +++ .../Semantics/ResourceAncestorGraph.cs | 16 +- src/Bicep.Core/Syntax/SyntaxFactory.cs | 2 + src/vscode-bicep/.vscode/launch.json | 3 +- 8 files changed, 560 insertions(+), 22 deletions(-) create mode 100644 src/Bicep.Core.IntegrationTests/Issue6075Tests.cs create mode 100644 src/Bicep.Core/Emit/SymbolReplacer.cs diff --git a/src/Bicep.Core.IntegrationTests/Issue6075Tests.cs b/src/Bicep.Core.IntegrationTests/Issue6075Tests.cs new file mode 100644 index 00000000000..fc71973af0e --- /dev/null +++ b/src/Bicep.Core.IntegrationTests/Issue6075Tests.cs @@ -0,0 +1,361 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Bicep.Core.UnitTests; +using Bicep.Core.UnitTests.Assertions; +using Bicep.Core.UnitTests.Utils; +using FluentAssertions.Execution; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Newtonsoft.Json.Linq; +using System.Diagnostics.CodeAnalysis; + +namespace Bicep.Core.IntegrationTests +{ + /// + /// Fixing https://github.com/Azure/bicep/issues/6075 required major changes to how name expressions and classic dependencies + /// are emitted, so that complexity warrants adding a special test class for the entire issue. + /// + [TestClass] + public class Issue6075Tests + { + private const string OriginalReproBicep = @" +var numberOfAccounts = 2 +var blobsPerAccount = 3 +var saprefix = uniqueString(resourceGroup().id) + +resource sa 'Microsoft.Storage/storageAccounts@2021-08-01' = [for i in range(0, numberOfAccounts): { + name: '${saprefix}${i}' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' +}] + +resource blobSvc 'Microsoft.Storage/storageAccounts/blobServices@2021-08-01' = [for j in range(0, numberOfAccounts): { + parent: sa[j] + name: 'default' +}] + +resource containers 'Microsoft.Storage/storageAccounts/blobServices/containers@2021-08-01' = [for k in range(0, (numberOfAccounts * blobsPerAccount)): { + parent: blobSvc[k % numberOfAccounts] + name: 'container${k % blobsPerAccount}' +}] +"; + + [NotNull] + public TestContext? TestContext { get; set; } + + [TestMethod] + public void OriginalReproTemplate_Classic_ShouldProduceExpectedExpressions() + { + var result = Compile(OriginalReproBicep, false); + result.Should().GenerateATemplate(); + + /* + The generated template was manually verified to be deployable. + */ + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources[0].copy.name", "sa"); + template.Should().HaveValueAtPath("$.resources[0].name", "[format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[copyIndex()])]"); + template.Should().NotHaveValueAtPath("$.resources[0].dependsOn"); + + template.Should().HaveValueAtPath("$.resources[1].copy.name", "blobSvc"); + template.Should().HaveValueAtPath("$.resources[1].name", "[format('{0}/{1}', format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[range(0, variables('numberOfAccounts'))[copyIndex()]]), 'default')]"); + template.Should().HaveValueAtPath("$.resources[1].dependsOn", new JArray("[resourceId('Microsoft.Storage/storageAccounts', format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[range(0, variables('numberOfAccounts'))[copyIndex()]]))]")); + + template.Should().HaveValueAtPath("$.resources[2].copy.name", "containers"); + template.Should().HaveValueAtPath("$.resources[2].name", "[format('{0}/{1}/{2}', format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[range(0, variables('numberOfAccounts'))[mod(range(0, mul(variables('numberOfAccounts'), variables('blobsPerAccount')))[copyIndex()], variables('numberOfAccounts'))]]), 'default', format('container{0}', mod(range(0, mul(variables('numberOfAccounts'), variables('blobsPerAccount')))[copyIndex()], variables('blobsPerAccount'))))]"); + template.Should().HaveValueAtPath("$.resources[2].dependsOn", new JArray("[resourceId('Microsoft.Storage/storageAccounts/blobServices', format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[range(0, variables('numberOfAccounts'))[mod(range(0, mul(variables('numberOfAccounts'), variables('blobsPerAccount')))[copyIndex()], variables('numberOfAccounts'))]]), 'default')]")); + } + } + + [TestMethod] + public void OriginalReproTemplate_Symbolic_ShouldProduceExpectedExpressions() + { + CompilationHelper.CompilationResult result = Compile(OriginalReproBicep, true); + result.Should().GenerateATemplate(); + + /* + The generated template was manually verified to be deployable. + */ + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources.sa.copy.name", "sa"); + template.Should().HaveValueAtPath("$.resources.sa.name", "[format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[copyIndex()])]"); + template.Should().NotHaveValueAtPath("$.resources.sa.dependsOn"); + + template.Should().HaveValueAtPath("$.resources.blobSvc.copy.name", "blobSvc"); + template.Should().HaveValueAtPath("$.resources.blobSvc.name", "[format('{0}/{1}', format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[range(0, variables('numberOfAccounts'))[copyIndex()]]), 'default')]"); + template.Should().HaveValueAtPath("$.resources.blobSvc.dependsOn", new JArray("[format('sa[{0}]', range(0, variables('numberOfAccounts'))[copyIndex()])]")); + + template.Should().HaveValueAtPath("$.resources.containers.copy.name", "containers"); + template.Should().HaveValueAtPath("$.resources.containers.name", "[format('{0}/{1}/{2}', format('{0}{1}', variables('saprefix'), range(0, variables('numberOfAccounts'))[range(0, variables('numberOfAccounts'))[mod(range(0, mul(variables('numberOfAccounts'), variables('blobsPerAccount')))[copyIndex()], variables('numberOfAccounts'))]]), 'default', format('container{0}', mod(range(0, mul(variables('numberOfAccounts'), variables('blobsPerAccount')))[copyIndex()], variables('blobsPerAccount'))))]"); + template.Should().HaveValueAtPath("$.resources.containers.dependsOn", new JArray("[format('blobSvc[{0}]', mod(range(0, mul(variables('numberOfAccounts'), variables('blobsPerAccount')))[copyIndex()], variables('numberOfAccounts')))]")); + } + } + + private const string AllIndexVariablesBicep = @" +resource vnet 'Microsoft.Network/virtualNetworks@2021-05-01' = [for (ii, i) in range(0, 2): { + name: string(i) +}] + +resource subnet 'Microsoft.Network/virtualNetworks/subnets@2021-05-01' = [for (jj, j) in range(0, 6): { + parent: vnet[j % 2] + name: string(j) +}] + +resource thing 'Microsoft.Network/virtualNetworks/subnets/things@2021-05-01' = [for (kk, k) in range(0, 24): { + parent: subnet[k % 6] + name: string(k) +}] +"; + + [TestMethod] + public void ThreeNestedResources_Classic_AllIndexVariables_ShouldProduceExpectedExpressions() + { + var result = Compile(AllIndexVariablesBicep, false); + result.Should().GenerateATemplate(); + + /* + The generated template was manually verified to be deployable. + */ + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources[0].copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources[0].name", "[string(copyIndex())]"); + template.Should().NotHaveValueAtPath("$.resources[0].dependsOn"); + + template.Should().HaveValueAtPath("$.resources[1].copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources[1].name", "[format('{0}/{1}', string(mod(copyIndex(), 2)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[1].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks', string(mod(copyIndex(), 2)))]")); + + template.Should().HaveValueAtPath("$.resources[2].copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources[2].name", "[format('{0}/{1}/{2}', string(mod(mod(copyIndex(), 6), 2)), string(mod(copyIndex(), 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[2].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks/subnets', string(mod(mod(copyIndex(), 6), 2)), string(mod(copyIndex(), 6)))]")); + } + } + + [TestMethod] + public void ThreeNestedResources_Symbolic_AllIndexVariables_ShouldProduceExpectedExpressions() + { + var result = Compile(AllIndexVariablesBicep, true); + result.Should().GenerateATemplate(); + + /* + The generated template was manually verified to be deployable. + */ + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources.vnet.copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources.vnet.name", "[string(copyIndex())]"); + template.Should().NotHaveValueAtPath("$.resources.vnet.dependsOn"); + + template.Should().HaveValueAtPath("$.resources.subnet.copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources.subnet.name", "[format('{0}/{1}', string(mod(copyIndex(), 2)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.subnet.dependsOn", new JArray("[format('vnet[{0}]', mod(copyIndex(), 2))]")); + + template.Should().HaveValueAtPath("$.resources.thing.copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources.thing.name", "[format('{0}/{1}/{2}', string(mod(mod(copyIndex(), 6), 2)), string(mod(copyIndex(), 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.thing.dependsOn", new JArray("[format('subnet[{0}]', mod(copyIndex(), 6))]")); + } + } + + private const string TopItemVariableBicep = @" +resource vnet 'Microsoft.Network/virtualNetworks@2021-05-01' = [for (ii, i) in range(0, 2): { + name: string(ii) +}] + +resource subnet 'Microsoft.Network/virtualNetworks/subnets@2021-05-01' = [for (jj, j) in range(0, 6): { + parent: vnet[j % 2] + name: string(j) +}] + +resource thing 'Microsoft.Network/virtualNetworks/subnets/things@2021-05-01' = [for (kk, k) in range(0, 24): { + parent: subnet[k % 6] + name: string(k) +}] +"; + + [TestMethod] + public void ThreeNestedResources_TopItemVariable_Classic_ShouldProduceExpectedExpressions() + { + var result = Compile(TopItemVariableBicep, false); + result.Should().GenerateATemplate(); + + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources[0].copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources[0].name", "[string(range(0, 2)[copyIndex()])]"); + template.Should().NotHaveValueAtPath("$.resources[0].dependsOn"); + + template.Should().HaveValueAtPath("$.resources[1].copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources[1].name", "[format('{0}/{1}', string(range(0, 2)[mod(copyIndex(), 2)]), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[1].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks', string(range(0, 2)[mod(copyIndex(), 2)]))]")); + + template.Should().HaveValueAtPath("$.resources[2].copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources[2].name", "[format('{0}/{1}/{2}', string(range(0, 2)[mod(mod(copyIndex(), 6), 2)]), string(mod(copyIndex(), 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[2].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks/subnets', string(range(0, 2)[mod(mod(copyIndex(), 6), 2)]), string(mod(copyIndex(), 6)))]")); + } + } + + [TestMethod] + public void ThreeNestedResources_TopItemVariable_Symbolic_ShouldProduceExpectedExpressions() + { + var result = Compile(TopItemVariableBicep, true); + result.Should().GenerateATemplate(); + + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources.vnet.copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources.vnet.name", "[string(range(0, 2)[copyIndex()])]"); + template.Should().NotHaveValueAtPath("$.resources.vnet.dependsOn"); + + template.Should().HaveValueAtPath("$.resources.subnet.copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources.subnet.name", "[format('{0}/{1}', string(range(0, 2)[mod(copyIndex(), 2)]), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.subnet.dependsOn", new JArray("[format('vnet[{0}]', mod(copyIndex(), 2))]")); + + template.Should().HaveValueAtPath("$.resources.thing.copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources.thing.name", "[format('{0}/{1}/{2}', string(range(0, 2)[mod(mod(copyIndex(), 6), 2)]), string(mod(copyIndex(), 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.thing.dependsOn", new JArray("[format('subnet[{0}]', mod(copyIndex(), 6))]")); + } + } + + private const string MiddleItemVariableBicep = @" +resource vnet 'Microsoft.Network/virtualNetworks@2021-05-01' = [for (ii, i) in range(0, 2): { + name: string(i) +}] + +resource subnet 'Microsoft.Network/virtualNetworks/subnets@2021-05-01' = [for (jj, j) in range(0, 6): { + parent: vnet[jj % 2] + name: string(j) +}] + +resource thing 'Microsoft.Network/virtualNetworks/subnets/things@2021-05-01' = [for (kk, k) in range(0, 24): { + parent: subnet[k % 6] + name: string(k) +}] +"; + + [TestMethod] + public void ThreeNestedResources_MiddleItemVariable_Classic_ShouldProduceExpectedExpressions() + { + var result = Compile(MiddleItemVariableBicep, false); + result.Should().GenerateATemplate(); + + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources[0].copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources[0].name", "[string(copyIndex())]"); + template.Should().NotHaveValueAtPath("$.resources[0].dependsOn"); + + template.Should().HaveValueAtPath("$.resources[1].copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources[1].name", "[format('{0}/{1}', string(mod(range(0, 6)[copyIndex()], 2)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[1].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks', string(mod(range(0, 6)[copyIndex()], 2)))]")); + + template.Should().HaveValueAtPath("$.resources[2].copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources[2].name", "[format('{0}/{1}/{2}', string(mod(range(0, 6)[mod(copyIndex(), 6)], 2)), string(mod(copyIndex(), 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[2].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks/subnets', string(mod(range(0, 6)[mod(copyIndex(), 6)], 2)), string(mod(copyIndex(), 6)))]")); + } + } + + [TestMethod] + public void ThreeNestedResources_MiddleItemVariable_Symbolic_ShouldProduceExpectedExpressions() + { + var result = Compile(MiddleItemVariableBicep, true); + result.Should().GenerateATemplate(); + + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources.vnet.copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources.vnet.name", "[string(copyIndex())]"); + template.Should().NotHaveValueAtPath("$.resources.vnet.dependsOn"); + + template.Should().HaveValueAtPath("$.resources.subnet.copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources.subnet.name", "[format('{0}/{1}', string(mod(range(0, 6)[copyIndex()], 2)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.subnet.dependsOn", new JArray("[format('vnet[{0}]', mod(range(0, 6)[copyIndex()], 2))]")); + + template.Should().HaveValueAtPath("$.resources.thing.copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources.thing.name", "[format('{0}/{1}/{2}', string(mod(range(0, 6)[mod(copyIndex(), 6)], 2)), string(mod(copyIndex(), 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.thing.dependsOn", new JArray("[format('subnet[{0}]', mod(copyIndex(), 6))]")); + } + } + + private const string BottomItemVariableBicep = @" +resource vnet 'Microsoft.Network/virtualNetworks@2021-05-01' = [for (ii, i) in range(0, 2): { + name: string(i) +}] + +resource subnet 'Microsoft.Network/virtualNetworks/subnets@2021-05-01' = [for (jj, j) in range(0, 6): { + parent: vnet[j % 2] + name: string(j) +}] + +resource thing 'Microsoft.Network/virtualNetworks/subnets/things@2021-05-01' = [for (kk, k) in range(0, 24): { + parent: subnet[kk % 6] + name: string(k) +}] +"; + + [TestMethod] + public void ThreeNestedResources_BottomItemVariable_Classic_ShouldProduceExpectedExpressions() + { + var result = Compile(BottomItemVariableBicep, false); + result.Should().GenerateATemplate(); + + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources[0].copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources[0].name", "[string(copyIndex())]"); + template.Should().NotHaveValueAtPath("$.resources[0].dependsOn"); + + template.Should().HaveValueAtPath("$.resources[1].copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources[1].name", "[format('{0}/{1}', string(mod(copyIndex(), 2)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[1].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks', string(mod(copyIndex(), 2)))]")); + + template.Should().HaveValueAtPath("$.resources[2].copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources[2].name", "[format('{0}/{1}/{2}', string(mod(mod(range(0, 24)[copyIndex()], 6), 2)), string(mod(range(0, 24)[copyIndex()], 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources[2].dependsOn", new JArray("[resourceId('Microsoft.Network/virtualNetworks/subnets', string(mod(mod(range(0, 24)[copyIndex()], 6), 2)), string(mod(range(0, 24)[copyIndex()], 6)))]")); + } + } + + [TestMethod] + public void ThreeNestedResources_BottomItemVariable_Symbolic_ShouldProduceExpectedExpressions() + { + var result = Compile(BottomItemVariableBicep, true); + result.Should().GenerateATemplate(); + + var template = result.Template; + using (new AssertionScope()) + { + template.Should().HaveValueAtPath("$.resources.vnet.copy.name", "vnet"); + template.Should().HaveValueAtPath("$.resources.vnet.name", "[string(copyIndex())]"); + template.Should().NotHaveValueAtPath("$.resources.vnet.dependsOn"); + + template.Should().HaveValueAtPath("$.resources.subnet.copy.name", "subnet"); + template.Should().HaveValueAtPath("$.resources.subnet.name", "[format('{0}/{1}', string(mod(copyIndex(), 2)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.subnet.dependsOn", new JArray("[format('vnet[{0}]', mod(copyIndex(), 2))]")); + + template.Should().HaveValueAtPath("$.resources.thing.copy.name", "thing"); + template.Should().HaveValueAtPath("$.resources.thing.name", "[format('{0}/{1}/{2}', string(mod(mod(range(0, 24)[copyIndex()], 6), 2)), string(mod(range(0, 24)[copyIndex()], 6)), string(copyIndex()))]"); + template.Should().HaveValueAtPath("$.resources.thing.dependsOn", new JArray("[format('subnet[{0}]', mod(range(0, 24)[copyIndex()], 6))]")); + } + } + + private CompilationHelper.CompilationResult Compile(string bicep, bool symbolicNameCodegenEnabled) + { + var context = new CompilationHelper.CompilationHelperContext(Features: BicepTestConstants.CreateFeaturesProvider(this.TestContext, symbolicNameCodegenEnabled: symbolicNameCodegenEnabled)); + return CompilationHelper.Compile(context, bicep); + } + } +} diff --git a/src/Bicep.Core/Emit/ExpressionConverter.cs b/src/Bicep.Core/Emit/ExpressionConverter.cs index 6c58eddd3aa..ff6239983e6 100644 --- a/src/Bicep.Core/Emit/ExpressionConverter.cs +++ b/src/Bicep.Core/Emit/ExpressionConverter.cs @@ -8,6 +8,7 @@ using System.Linq; using Azure.Deployments.Core.Extensions; using Azure.Deployments.Expression.Expressions; +using Bicep.Core.DataFlow; using Bicep.Core.Extensions; using Bicep.Core.Semantics; using Bicep.Core.Semantics.Metadata; @@ -180,7 +181,7 @@ public ExpressionConverter CreateConverterForIndexReplacement(SyntaxBase nameSyn // regardless if there is an index expression or not, we don't need to append replacements return this; - case 1 when indexExpression != null: + case 1 when indexExpression is not null: // TODO: Run data flow analysis on the array expression as well. (Will be needed for nested resource loops) var @for = inaccessibleLocalLoops.Single(); var current = this; @@ -510,8 +511,8 @@ public IEnumerable GetResourceNameSegments(DeclaredResourceM var parentNames = ancestors.SelectMany((x, i) => { - var nameExpression = CreateConverterForIndexReplacement(x.Resource.NameSyntax, x.IndexExpression, resource.Symbol.NameSyntax) - .ConvertExpression(x.Resource.NameSyntax); + var expression = GetResourceNameAncestorSyntaxSegment(resource, i); + var nameExpression = this.ConvertExpression(expression); if (i == 0 && firstAncestorNameLength > 1) { @@ -538,6 +539,127 @@ public IEnumerable GetResourceNameSegments(DeclaredResourceM new JTokenExpression(i))); } + /// + /// Returns a collection of name segment expressions for the specified resource. Local variable replacements + /// are performed so the expressions are valid in the language/binding scope of the specified resource. + /// + /// The resource + public IEnumerable GetResourceNameSyntaxSegments(DeclaredResourceMetadata resource) + { + var ancestors = this.context.SemanticModel.ResourceAncestors.GetAncestors(resource); + var nameExpression = resource.NameSyntax; + + return ancestors + .Select((x, i) => GetResourceNameAncestorSyntaxSegment(resource, i)) + .Concat(nameExpression); + } + + /// + /// Calculates the expression that represents the parent name corresponding to the specified ancestor of the specified resource. + /// The expressions returned are modified by performing the necessary local variable replacements. + /// + /// The declared resource metadata + /// the index of the ancestor (0 means the ancestor closest to the root) + private SyntaxBase GetResourceNameAncestorSyntaxSegment(DeclaredResourceMetadata resource, int startingAncestorIndex) + { + var ancestors = this.context.SemanticModel.ResourceAncestors.GetAncestors(resource); + if(startingAncestorIndex >= ancestors.Length) + { + // not enough ancestors + throw new ArgumentException($"Resource type has {ancestors.Length} ancestor types but name expression was requested for ancestor type at index {startingAncestorIndex}."); + } + + /* + * Consider the following example: + * + * resource one 'MS.Example/ones@...' = [for (_, i) in range(0, ...) : { + * name: name_exp1(i) + * }] + * + * resource two 'MS.Example/ones/twos@...' = [for (_, j) in range(0, ...) : { + * parent: one[index_exp2(j)] + * name: name_exp2(j) + * }] + * + * resource three 'MS.Example/ones/twos/threes@...' = [for (_, k) in range(0, ...) : { + * parent: two[index_exp3(k)] + * name: name_exp3(k) + * }] + * + * name_exp* and index_exp* are expressions represented here as functions + * + * The name segment expressions for "three" are the following: + * 0. name_exp1(index_exp2(index_exp3(k))) + * 1. name_exp2(index_exp3(k)) + * 2. name_exp3(k) + * + * (The formula can be generalized to more levels of nesting.) + * + * This function can be used to get 0 and 1 above by passing 0 or 1 respectively as the startingAncestorIndex. + * The name segment 2 above must be obtained from the resource directly. + * + * Given that we don't have proper functions in our runtime AND that our expressions don't have side effects, + * the formula is implemented via local variable replacement. + */ + + // the initial ancestor gives us the base expression + SyntaxBase? rewritten = ancestors[startingAncestorIndex].Resource.NameSyntax; + + for(int i = startingAncestorIndex; i < ancestors.Length; i++) + { + var ancestor = ancestors[i]; + + // local variable replacement will be done in context of the next ancestor + // or the resource itself if we're on the last ancestor + var newContext = i < ancestors.Length - 1 ? ancestors[i + 1].Resource : resource; + + var inaccessibleLocals = this.context.DataFlowAnalyzer.GetInaccessibleLocalsAfterSyntaxMove(rewritten, newContext.Symbol.NameSyntax); + var inaccessibleLocalLoops = inaccessibleLocals.Select(local => GetEnclosingForExpression(local)).Distinct().ToList(); + + switch (inaccessibleLocalLoops.Count) + { + case 0 when i == startingAncestorIndex: + /* + * There are no local vars to replace. It is impossible for a local var to be introduced at the next level + * so we can just bail out with the result. + * + * This path is followed by non-loop resources. + * + * Case 0 is not possible for non-starting ancestor index because + * once we have a local variable replacement, it will propagate to the next levels + */ + return ancestor.Resource.NameSyntax; + + case 1 when ancestor.IndexExpression is not null: + if (LocalSymbolDependencyVisitor.GetLocalSymbolDependencies(this.context.SemanticModel, rewritten).SingleOrDefault(s => s.LocalKind == LocalKind.ForExpressionItemVariable) is { } loopItemSymbol) + { + // rewrite the straggler from previous iteration + // TODO: Nested loops will require DFA on the ForSyntax.Expression + rewritten = SymbolReplacer.Replace(this.context.SemanticModel, new Dictionary { [loopItemSymbol] = SyntaxFactory.CreateArrayAccess(GetEnclosingForExpression(loopItemSymbol).Expression, ancestor.IndexExpression) }, rewritten); + } + + // TODO: Run data flow analysis on the array expression as well. (Will be needed for nested resource loops) + var @for = inaccessibleLocalLoops.Single(); + + var replacements = inaccessibleLocals.ToDictionary(local => (Symbol)local, local => local.LocalKind switch + { + LocalKind.ForExpressionIndexVariable => ancestor.IndexExpression, + LocalKind.ForExpressionItemVariable => SyntaxFactory.CreateArrayAccess(@for.Expression, ancestor.IndexExpression), + _ => throw new NotImplementedException($"Unexpected local kind '{local.LocalKind}'.") + }); + + rewritten = SymbolReplacer.Replace(this.context.SemanticModel, replacements, rewritten); + + break; + + default: + throw new NotImplementedException("Mismatch between count of index expressions and inaccessible symbols during array access index expression rewriting."); + } + } + + return rewritten; + } + public LanguageExpression GetFullyQualifiedResourceName(DeclaredResourceMetadata resource) { var nameValueSyntax = resource.NameSyntax; @@ -603,12 +725,13 @@ public LanguageExpression GetFullyQualifiedResourceId(ResourceMetadata resource) } else if (resource is DeclaredResourceMetadata declared) { + var nameSegments = GetResourceNameSegments(declared); return ScopeHelper.FormatFullyQualifiedResourceId( context, this, context.ResourceScopeData[declared], resource.TypeReference.FormatType(), - GetResourceNameSegments(declared)); + nameSegments); } else { diff --git a/src/Bicep.Core/Emit/ExpressionEmitter.cs b/src/Bicep.Core/Emit/ExpressionEmitter.cs index 71d05dec73f..e8db6dca97b 100644 --- a/src/Bicep.Core/Emit/ExpressionEmitter.cs +++ b/src/Bicep.Core/Emit/ExpressionEmitter.cs @@ -130,7 +130,9 @@ public void EmitIndexedSymbolReference(ModuleSymbol moduleSymbol, SyntaxBase ind public void EmitResourceIdReference(DeclaredResourceMetadata resource, SyntaxBase? indexExpression, SyntaxBase newContext) { - var converterForContext = this.converter.CreateConverterForIndexReplacement(resource.NameSyntax, indexExpression, newContext); + var nameComponents = SyntaxFactory.CreateArray(this.converter.GetResourceNameSyntaxSegments(resource)); + + var converterForContext = this.converter.CreateConverterForIndexReplacement(nameComponents, indexExpression, newContext); var resourceIdExpression = converterForContext.GetFullyQualifiedResourceId(resource); var serialized = ExpressionSerializer.SerializeExpression(resourceIdExpression); diff --git a/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs b/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs index 3f8c861f033..c4ceedd2eb3 100644 --- a/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs +++ b/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs @@ -237,7 +237,7 @@ public override void VisitResourceAccessSyntax(ResourceAccessSyntax syntax) _ => throw new NotImplementedException($"Unexpected current declaration type '{this.currentDeclaration?.GetType().Name}'.") }; - // using the resource/module body as the context to allow indexed depdnencies relying on the resource/module loop index to work as expected + // using the resource/module body as the context to allow indexed dependencies relying on the resource/module loop index to work as expected var inaccessibleLocals = dfa.GetInaccessibleLocalsAfterSyntaxMove(candidateIndexExpression, context); if (inaccessibleLocals.Any()) { diff --git a/src/Bicep.Core/Emit/SymbolReplacer.cs b/src/Bicep.Core/Emit/SymbolReplacer.cs new file mode 100644 index 00000000000..21cca6ca606 --- /dev/null +++ b/src/Bicep.Core/Emit/SymbolReplacer.cs @@ -0,0 +1,63 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Bicep.Core.Semantics; +using Bicep.Core.Syntax; +using System.Collections.Generic; + +namespace Bicep.Core.Emit +{ + public class SymbolReplacer : SyntaxRewriteVisitor + { + private readonly SemanticModel semanticModel; + private IReadOnlyDictionary replacements; + + private SymbolReplacer(SemanticModel semanticModel, IReadOnlyDictionary replacements) + { + this.semanticModel = semanticModel; + this.replacements = replacements; + } + + public static SyntaxBase Replace(SemanticModel semanticModel, IReadOnlyDictionary replacements, SyntaxBase syntax) => + new SymbolReplacer(semanticModel, replacements).Rewrite(syntax); + + protected override SyntaxBase ReplaceVariableAccessSyntax(VariableAccessSyntax syntax) + { + if (this.semanticModel.GetSymbolInfo(syntax) is not { } symbol || !this.replacements.TryGetValue(symbol, out var replacementSyntax)) + { + // unbound variable access or not a symbol that we need to replace + // leave syntax as-is + return base.ReplaceVariableAccessSyntax(syntax); + } + + // inject the replacement syntax + return replacementSyntax; + } + } + + public class SyntaxReplacer: SyntaxRewriteVisitor + { + private IReadOnlyDictionary replacements; + + private SyntaxReplacer(IReadOnlyDictionary replacements) + { + this.replacements = replacements; + } + + public static SyntaxBase Replace(IReadOnlyDictionary replacements, SyntaxBase syntax) => + new SyntaxReplacer(replacements).Rewrite(syntax); + + protected override SyntaxBase ReplaceVariableAccessSyntax(VariableAccessSyntax syntax) + { + if(!this.replacements.TryGetValue(syntax, out var replacementSyntax)) + { + // no match + // leave syntax as-is + return base.ReplaceVariableAccessSyntax(syntax); + } + + // inject the replacment syntax + return replacementSyntax; + } + } +} diff --git a/src/Bicep.Core/Semantics/ResourceAncestorGraph.cs b/src/Bicep.Core/Semantics/ResourceAncestorGraph.cs index f817dcfa1bd..7647fb3542e 100644 --- a/src/Bicep.Core/Semantics/ResourceAncestorGraph.cs +++ b/src/Bicep.Core/Semantics/ResourceAncestorGraph.cs @@ -17,21 +17,7 @@ public enum ResourceAncestorType ParentProperty, } - public class ResourceAncestor - { - public ResourceAncestor(ResourceAncestorType ancestorType, DeclaredResourceMetadata resource, SyntaxBase? indexExpression) - { - AncestorType = ancestorType; - Resource = resource; - IndexExpression = indexExpression; - } - - public ResourceAncestorType AncestorType { get; } - - public DeclaredResourceMetadata Resource { get; } - - public SyntaxBase? IndexExpression { get; } - } + public record ResourceAncestor(ResourceAncestorType AncestorType, DeclaredResourceMetadata Resource, SyntaxBase? IndexExpression); private readonly ImmutableDictionary> data; diff --git a/src/Bicep.Core/Syntax/SyntaxFactory.cs b/src/Bicep.Core/Syntax/SyntaxFactory.cs index abb1569a14b..a83bc4c9638 100644 --- a/src/Bicep.Core/Syntax/SyntaxFactory.cs +++ b/src/Bicep.Core/Syntax/SyntaxFactory.cs @@ -130,6 +130,8 @@ public static ArraySyntax CreateArray(IEnumerable items) RightSquareToken); } + public static ArrayAccessSyntax CreateArrayAccess(SyntaxBase baseExpression, SyntaxBase indexExpression) => new(baseExpression, LeftSquareToken, indexExpression, RightSquareToken); + public static SyntaxBase CreateObjectPropertyKey(string text) { if (Regex.IsMatch(text, "^[a-zA-Z][a-zA-Z0-9_]*$")) diff --git a/src/vscode-bicep/.vscode/launch.json b/src/vscode-bicep/.vscode/launch.json index 6883faa6e38..3f010b32ee6 100644 --- a/src/vscode-bicep/.vscode/launch.json +++ b/src/vscode-bicep/.vscode/launch.json @@ -18,7 +18,8 @@ "env": { "BICEP_TRACING_ENABLED": "true", "BICEP_LANGUAGE_SERVER_PATH": "${workspaceRoot}/../Bicep.LangServer/bin/Debug/net6.0/Bicep.LangServer.dll", - + //"BICEP_SYMBOLIC_NAME_CODEGEN_EXPERIMENTAL": "true", + // Defaulting to "verbose" when debugging so that new telemetry IDs during development aren't accidentally sent and created before they're finalized "DEBUGTELEMETRY": "v", // "" or "0" or "false": send telemetry as normal; "1": debug mode (no telemetry sent); "verbose": telemetry in console, don't send (from microsoft/vscode-azext-utils) }