diff --git a/src/Bicep.Cli/Rpc/CliJsonRpcServer.cs b/src/Bicep.Cli/Rpc/CliJsonRpcServer.cs index 03284b1d89c..7760038bc3e 100644 --- a/src/Bicep.Cli/Rpc/CliJsonRpcServer.cs +++ b/src/Bicep.Cli/Rpc/CliJsonRpcServer.cs @@ -175,7 +175,7 @@ public async Task GetDeploymentGraph(GetDeploymentGr { var compilation = await GetCompilation(compiler, request.Path); var model = compilation.GetEntrypointSemanticModel(); - var dependenciesBySymbol = ResourceDependencyVisitor.GetResourceDependencies(model, new() { IncludeExisting = true }) + var dependenciesBySymbol = ResourceDependencyVisitor.GetResourceDependencies(model) .Where(x => !x.Key.Type.IsError()) .ToImmutableDictionary(x => x.Key, x => x.Value); @@ -200,7 +200,7 @@ public async Task GetDeploymentGraph(GetDeploymentGr foreach (var (symbol, dependencies) in dependenciesBySymbol) { var source = nodesBySymbol.TryGetValue(symbol); - foreach (var dependency in dependencies.Where(d => d.Kind == ResourceDependencyKind.Primary)) + foreach (var dependency in dependencies) { var target = nodesBySymbol.TryGetValue(dependency.Resource); if (source is { } && target is { }) diff --git a/src/Bicep.Core.IntegrationTests/Emit/DependencyInferenceTests.cs b/src/Bicep.Core.IntegrationTests/Emit/DependencyInferenceTests.cs new file mode 100644 index 00000000000..435bf4089be --- /dev/null +++ b/src/Bicep.Core.IntegrationTests/Emit/DependencyInferenceTests.cs @@ -0,0 +1,704 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Bicep.Core.Configuration; +using Bicep.Core.IntegrationTests.Extensibility; +using Bicep.Core.UnitTests; +using Bicep.Core.UnitTests.Assertions; +using Bicep.Core.UnitTests.Utils; +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Microsoft.WindowsAzure.ResourceStack.Common.Json; +using Newtonsoft.Json.Linq; + +namespace Bicep.Core.IntegrationTests.Emit; + +[TestClass] +public class DependencyInferenceTests +{ + [TestMethod] + public void Implicit_dependencies_on_existing_resources_are_reflected_in_symbolic_name_template() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + """ + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = { + name: replace(deployedSa.name, '1', '2') + } + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + properties: { + allowSharedKeyAccess: existingSa.properties.allowSharedKeyAccess + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.newSa.dependsOn", """["existingSa"]"""); + result.Template.Should().HaveJsonAtPath("$.resources.existingSa.dependsOn", """["deployedSa"]"""); + } + + [TestMethod] + public void Explicit_dependencies_on_existing_resources_are_reflected_in_symbolic_name_template() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + """ + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = { + name: replace(deployedSa.name, '1', '2') + } + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + dependsOn: [ + existingSa + ] + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.newSa.dependsOn", """["existingSa"]"""); + result.Template.Should().HaveJsonAtPath("$.resources.existingSa.dependsOn", """["deployedSa"]"""); + } + + [TestMethod] + public void Implicit_dependencies_on_existing_resources_are_expressed_as_direct_dependencies_on_transitive_dependencies_in_non_symbolic_name_template() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: false)), + """ + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = { + name: replace(deployedSa.name, '1', '2') + } + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + properties: { + allowSharedKeyAccess: existingSa.properties.allowSharedKeyAccess + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.GetProperty("resources").Should().BeOfType() + .Subject.Count.Should().Be(2); + result.Template.Should().HaveJsonAtPath("$.resources[1].dependsOn", """["[resourceId('Microsoft.Storage/storageAccounts', 'account1')]"]"""); + } + + [TestMethod] + public void Explicit_dependencies_on_existing_resources_are_expressed_as_direct_dependencies_on_transitive_dependencies_in_non_symbolic_name_template() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: false)), + """ + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = { + name: replace(deployedSa.name, '1', '2') + } + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + dependsOn: [ + existingSa + ] + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.GetProperty("resources").Should().BeOfType() + .Subject.Count.Should().Be(2); + result.Template.Should().HaveJsonAtPath("$.resources[1].dependsOn", """["[resourceId('Microsoft.Storage/storageAccounts', 'account1')]"]"""); + } + + [TestMethod] + public void Implicit_dependencies_on_deployed_resource_identifying_properties_are_expressed_in_symbolic_name_template() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + """ + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource secondDeployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: replace(deployedSa.name, '1', '2') + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: replace(secondDeployedSa.name, '2', '3') + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.newSa.dependsOn", """["secondDeployedSa"]"""); + result.Template.Should().HaveJsonAtPath("$.resources.secondDeployedSa.dependsOn", """["deployedSa"]"""); + } + + [DataTestMethod] + [DataRow(true)] + [DataRow(false)] + public void Implicit_dependencies_on_existing_resource_identifying_properties_are_expressed_as_direct_dependencies_on_transitive_dependencies_in_symbolic_name_template(bool useArrayAccess) + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + $$""" + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = { + name: replace(deployedSa.name, '1', '2') + } + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + properties: { + allowSharedKeyAccess: bool(existingSa{{(useArrayAccess ? "['name']" : ".name")}}) + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.newSa.dependsOn", """["existingSa"]"""); + result.Template.Should().HaveJsonAtPath("$.resources.existingSa.dependsOn", """["deployedSa"]"""); + } + + [DataTestMethod] + [DataRow(true)] + [DataRow(false)] + public void Implicit_dependencies_on_existing_resource_collection_identifying_properties_are_expressed_as_direct_dependencies_on_transitive_dependencies_in_symbolic_name_template(bool useArrayAccess) + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + $$""" + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = [for i in range(0, 1): { + name: '${replace(deployedSa.name, '1', '2')}_${i}' + }] + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + properties: { + allowSharedKeyAccess: bool(existingSa[0]{{(useArrayAccess ? "['name']" : ".name")}}) + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.newSa.dependsOn", """["[format('existingSa[{0}]', 0)]"]"""); + result.Template.Should().HaveJsonAtPath("$.resources.existingSa.dependsOn", """["deployedSa"]"""); + } + + [TestMethod] + public void Existing_resource_function_calls_are_expressed_as_direct_dependencies_on_transitive_dependencies_in_symbolic_name_template() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + """ + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = { + name: replace(deployedSa.name, '1', '2') + } + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + properties: { + allowSharedKeyAccess: !empty(existingSa.listKeys().keys) + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.newSa.dependsOn", """["existingSa"]"""); + result.Template.Should().HaveJsonAtPath("$.resources.existingSa.dependsOn", """["deployedSa"]"""); + } + + [TestMethod] + public void Existing_resource_collection_function_calls_are_expressed_as_direct_dependencies_on_transitive_dependencies_in_symbolic_name_template() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + """ + resource deployedSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account1' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + } + + resource existingSa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = [for i in range(0, 1): { + name: '${replace(deployedSa.name, '1', '2')}_${i}' + }] + + resource newSa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'account3' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + properties: { + allowSharedKeyAccess: !empty(existingSa[0].listKeys().keys) + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.newSa.dependsOn", """["[format('existingSa[{0}]', 0)]"]"""); + result.Template.Should().HaveJsonAtPath("$.resources.existingSa.dependsOn", """["deployedSa"]"""); + } + + [TestMethod] + public void Using_an_existing_resource_as_an_explicit_parent_does_not_generate_an_explicit_dependency() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + """ + resource virtualNetwork 'Microsoft.Network/virtualNetworks@2020-08-01' existing = { + name: 'vnet' + } + + resource subnet 'Microsoft.Network/virtualNetworks/subnets@2020-08-01' existing = { + parent: virtualNetwork + name: 'subnet' + } + + resource sa 'Microsoft.Storage/storageAccounts@2023-05-01' = { + name: 'storage' + location: resourceGroup().location + sku: { + name: 'Standard_LRS' + } + kind: 'StorageV2' + properties: { + networkAcls: { + defaultAction: 'Deny' + virtualNetworkRules: [ + { + action: 'Allow' + id: subnet.id + } + ] + } + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.sa.dependsOn", """["subnet"]"""); + } + + [DataTestMethod] + [DataRow(true)] + [DataRow(false)] + public void Non_looped_resource_depending_on_looped_existing_resource_should_depend_on_transitive_resource_collections(bool useSymbolicNameCodegen) + { + var result = CompilationHelper.Compile( + new UnitTests.ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: useSymbolicNameCodegen)), + """ + resource vnets 'Microsoft.Network/virtualNetworks@2024-03-01' = [for i in range(0, 10): { + name: 'vnet${i}' + }] + + resource subnets 'Microsoft.Network/virtualNetworks/subnets@2024-03-01' existing = [for i in range(0, 10): { + parent: vnets[i] + name: 'subnet' + }] + + resource vault 'Microsoft.KeyVault/vaults@2023-07-01' = { + name: 'vault' + location: resourceGroup().location + properties: { + sku: { + name: 'standard' + family: 'A' + } + tenantId: subscription().tenantId + networkAcls: { + virtualNetworkRules: [for i in range(0, 10): { + id: subnets[i].id + }] + } + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + + if (useSymbolicNameCodegen) + { + result.Template.Should().HaveJsonAtPath("$.resources.vault.dependsOn", """["subnets"]"""); + } + else + { + result.Template.Should().HaveJsonAtPath("$.resources[?(@.name=='vault')].dependsOn", """["vnets"]"""); + } + } + + [DataTestMethod] + [DataRow(true)] + [DataRow(false)] + public void Looped_resource_depending_on_looped_existing_resource_should_depend_on_transitive_resource_element(bool useSymbolicNameCodegen) + { + var result = CompilationHelper.Compile( + new UnitTests.ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: useSymbolicNameCodegen)), + """ + resource vnets 'Microsoft.Network/virtualNetworks@2024-03-01' = [for i in range(0, 2): { + name: 'vnet${i}' + }] + + resource subnets 'Microsoft.Network/virtualNetworks/subnets@2024-03-01' existing = [for j in range(0, 10): { + parent: vnets[j % 2] + name: 'subnet' + }] + + resource vault 'Microsoft.KeyVault/vaults@2023-07-01' = [for k in range(10, 10): { + name: 'vault${k}' + location: resourceGroup().location + properties: { + sku: { + name: 'standard' + family: 'A' + } + tenantId: subscription().tenantId + networkAcls: { + virtualNetworkRules: [{ + id: subnets[k - 10].id + }] + } + } + }] + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + + if (useSymbolicNameCodegen) + { + result.Template.Should().HaveJsonAtPath( + "$.resources.vault.dependsOn", + """["[format('subnets[{0}]', sub(range(10, 10)[copyIndex()], 10))]"]"""); + } + else + { + result.Template.Should().HaveJsonAtPath( + "$.resources[?(@.type=='Microsoft.KeyVault/vaults')].dependsOn", + """["[resourceId('Microsoft.Network/virtualNetworks', format('vnet{0}', range(0, 2)[mod(range(0, 10)[sub(range(10, 10)[copyIndex()], 10)], 2)]))]"]"""); + } + } + + [DataTestMethod] + [DataRow(true)] + [DataRow(false)] + public void Looped_resource_depending_on_looped_variable_should_depend_on_transitive_resource_element(bool useSymbolicNameCodegen) + { + var result = CompilationHelper.Compile( + new UnitTests.ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: useSymbolicNameCodegen)), + """ + resource vnets 'Microsoft.Network/virtualNetworks@2024-03-01' = [for i in range(0, 2): { + name: 'vnet${i}' + }] + + resource subnets 'Microsoft.Network/virtualNetworks/subnets@2024-03-01' existing = [for j in range(0, 10): { + parent: vnets[j % 2] + name: 'subnet' + }] + + resource vault 'Microsoft.KeyVault/vaults@2023-07-01' = [for k in range(10, 10): { + name: 'vault${k}' + location: resourceGroup().location + properties: { + sku: { + name: 'standard' + family: 'A' + } + tenantId: subscription().tenantId + networkAcls: { + virtualNetworkRules: [{ + id: subnetIds[k - 10] + }] + } + } + }] + + var subnetIds = [for l in range(20, 10): subnets[l - 20].id] + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + + if (useSymbolicNameCodegen) + { + result.Template.Should().HaveJsonAtPath( + "$.resources.vault.dependsOn", + """["[format('subnets[{0}]', sub(range(20, 10)[sub(range(10, 10)[copyIndex()], 10)], 20))]"]"""); + } + else + { + result.Template.Should().HaveJsonAtPath( + "$.resources[?(@.type=='Microsoft.KeyVault/vaults')].dependsOn", + """["[resourceId('Microsoft.Network/virtualNetworks', format('vnet{0}', range(0, 2)[mod(range(0, 10)[sub(range(20, 10)[sub(range(10, 10)[copyIndex()], 10)], 20)], 2)]))]"]"""); + } + } + + [DataTestMethod] + [DataRow(true)] + [DataRow(false)] + public void CopyIndex_only_appears_in_compiled_expression_if_all_links_in_chain_use_a_loop_variable_reference(bool useSymbolicNameCodegen) + { + var result = CompilationHelper.Compile( + new UnitTests.ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: useSymbolicNameCodegen)), + """ + resource vnets 'Microsoft.Network/virtualNetworks@2024-03-01' = [for i in range(0, 2): { + name: 'vnet${i}' + }] + + resource subnets 'Microsoft.Network/virtualNetworks/subnets@2024-03-01' existing = [for j in range(0, 10): { + parent: vnets[0] + name: 'subnet${j}' + }] + + resource vault 'Microsoft.KeyVault/vaults@2023-07-01' = [for k in range(10, 10): { + name: 'vault${k}' + location: resourceGroup().location + properties: { + sku: { + name: 'standard' + family: 'A' + } + tenantId: subscription().tenantId + networkAcls: { + virtualNetworkRules: [{ + id: subnets[k - 10].id + }] + } + } + }] + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + + if (useSymbolicNameCodegen) + { + result.Template.Should().HaveJsonAtPath("$.resources.vault.dependsOn", """["[format('subnets[{0}]', sub(range(10, 10)[copyIndex()], 10))]"]"""); + } + else + { + result.Template.Should().HaveJsonAtPath( + "$.resources[?(@.type=='Microsoft.KeyVault/vaults')].dependsOn", + """["[resourceId('Microsoft.Network/virtualNetworks', format('vnet{0}', range(0, 2)[0]))]"]"""); + } + } + + [DataTestMethod] + [DataRow(true)] + [DataRow(false)] + public void CopyIndex_only_appears_in_compiled_expression_if_all_links_in_chain_use_a_loop_variable_reference_2(bool useSymbolicNameCodegen) + { + var result = CompilationHelper.Compile( + new UnitTests.ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: useSymbolicNameCodegen)), + """ + resource vnets 'Microsoft.Network/virtualNetworks@2024-03-01' = [for i in range(0, 2): { + name: 'vnet${i}' + }] + + resource subnets 'Microsoft.Network/virtualNetworks/subnets@2024-03-01' existing = [for j in range(0, 10): { + parent: vnets[j % 2] + name: 'subnet${j}' + }] + + resource vault 'Microsoft.KeyVault/vaults@2023-07-01' = [for k in range(10, 10): { + name: 'vault${k}' + location: resourceGroup().location + properties: { + sku: { + name: 'standard' + family: 'A' + } + tenantId: subscription().tenantId + networkAcls: { + virtualNetworkRules: [{ + id: subnets[0].id + }] + } + } + }] + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + + if (useSymbolicNameCodegen) + { + result.Template.Should().HaveJsonAtPath("$.resources.vault.dependsOn", """["[format('subnets[{0}]', 0)]"]"""); + } + else + { + result.Template.Should().HaveJsonAtPath( + "$.resources[?(@.type=='Microsoft.KeyVault/vaults')].dependsOn", + """["[resourceId('Microsoft.Network/virtualNetworks', format('vnet{0}', range(0, 2)[mod(range(0, 10)[0], 2)]))]"]"""); + } + } + + [TestMethod] + public void Extensibility_resources_always_generate_explicit_dependency() + { + var result = CompilationHelper.Compile( + new UnitTests.ServiceBuilder().WithFeatureOverrides(new(ExtensibilityEnabled: true)) + .WithConfigurationPatch(c => c.WithExtensions(""" + { + "az": "builtin:", + "kubernetes": "builtin:", + "microsoftGraph": "builtin:", + "foo": "builtin:", + "bar": "builtin:" + } + """)) + .WithNamespaceProvider(TestExtensibilityNamespaceProvider.CreateWithDefaults()), + """ + extension bar with { + connectionString: 'connectionString' + } + + resource container 'container' existing = { + name: 'containerName' + } + + resource sa 'Microsoft.Storage/storageAccounts@2023-05-01' existing = { + name: container.name + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.sa.dependsOn", """["container"]"""); + } + + [TestMethod] + public void Using_an_existing_resource_as_a_scope_does_not_generate_an_explicit_dependency() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(SymbolicNameCodegenEnabled: true)), + ("main.bicep", """ + targetScope = 'subscription' + + resource rg 'Microsoft.Resources/resourceGroups@2024-07-01' existing = { + name: 'rg' + } + + module empty 'empty.bicep' = { + scope: rg + name: 'empty' + } + """), + ("empty.bicep", string.Empty)); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().HaveJsonAtPath("$.resources.empty.dependsOn", """["rg"]"""); + } +} diff --git a/src/Bicep.Core.IntegrationTests/ScenarioTests.cs b/src/Bicep.Core.IntegrationTests/ScenarioTests.cs index ee8ad58b597..2a24cd6bd89 100644 --- a/src/Bicep.Core.IntegrationTests/ScenarioTests.cs +++ b/src/Bicep.Core.IntegrationTests/ScenarioTests.cs @@ -6339,4 +6339,195 @@ param condition bool result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); } + + [TestMethod] + public void Test_Issue15513() + { + var result = CompilationHelper.Compile( + new ServiceBuilder().WithFeatureOverrides(new(ExtensibilityEnabled: true)), + """ + #disable-next-line BCP407 + extension microsoftGraph + + param entraGroup object = { + name: 'ExampleGroup2' + type: 'Security' + members: [ + { + name: '{application name}' + type: 'Application' + } + ] + owners: [ + { + name: '{user identity name}' + resourceGroup: '{resource group name}' + type: 'UserAssignedManagedIdentity' + } + ] + } + + var defaultMember = { + subscriptionId: subscription().subscriptionId + resourceGroup: '' + name: '' + appId: '' + } + + resource memberManagedIdentities 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-07-31-preview' existing = [ + for (member, i) in entraGroup.members: if (member.type =~ 'UserAssignedManagedIdentity') { + //https://github.com/Azure/bicep/issues/13937 + name: empty(union(defaultMember, member).name) ? 'dummy${i}' : member.name + scope: resourceGroup(union(defaultMember, member).subscriptionId, union(defaultMember, member).resourceGroup) + } + ] + + resource memberApplications 'Microsoft.Graph/applications@v1.0' existing = [ + for (member, i) in entraGroup.members: if (member.type =~ 'Application') { + //https://github.com/Azure/bicep/issues/13937 + uniqueName: empty(union(defaultMember, member).name) ? 'dummy${i}' : member.name + } + ] + + resource memberServicePrincipals 'Microsoft.Graph/servicePrincipals@v1.0' existing = [ + for (member, i) in entraGroup.members: if (member.type =~ 'Application') { + appId: memberApplications[i].appId + } + ] + + resource memberServicePrincipalsStandalone 'Microsoft.Graph/servicePrincipals@v1.0' existing = [ + for (member, i) in entraGroup.members: if (member.type =~ 'ServicePrincipal') { + //https://github.com/Azure/bicep/issues/13937 + appId: empty(union(defaultMember, member).appId) ? 'dummy${i}' : member.appId + } + ] + + resource memberGroups 'Microsoft.Graph/groups@v1.0' existing = [ + for (member, i) in entraGroup.members: if (member.type =~ 'Group') { + //https://github.com/Azure/bicep/issues/13937 + uniqueName: empty(union(defaultMember, member).name) ? 'dummy${i}' : member.name + } + ] + + resource ownerManagedIdentities 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-07-31-preview' existing = [ + for (owner, i) in entraGroup.owners: if (owner.type =~ 'UserAssignedManagedIdentity') { + //https://github.com/Azure/bicep/issues/13937 + name: empty(union(defaultMember, owner).name) ? 'dummy${i}' : owner.name + scope: resourceGroup(union(defaultMember, owner).subscriptionId, union(defaultMember, owner).resourceGroup) + } + ] + + resource ownerApplications 'Microsoft.Graph/applications@v1.0' existing = [ + for (owner, i) in entraGroup.owners: if (owner.type =~ 'Application') { + //https://github.com/Azure/bicep/issues/13937 + uniqueName: empty(union(defaultMember, owner).name) ? 'dummy${i}' : owner.name + } + ] + + resource ownerServicePrincipals 'Microsoft.Graph/servicePrincipals@v1.0' existing = [ + for (owner, i) in entraGroup.owners: if (owner.type =~ 'Application') { + appId: ownerApplications[i].appId + } + ] + + resource ownerServicePrincipalsStandalone 'Microsoft.Graph/servicePrincipals@v1.0' existing = [ + for (owner, i) in entraGroup.owners: if (owner.type =~ 'ServicePrincipal') { + //https://github.com/Azure/bicep/issues/13937 + appId: empty(union(defaultMember, owner).appId) ? 'dummy${i}' : owner.appId + } + ] + + resource entraGroupRes 'Microsoft.Graph/groups@v1.0' = { + uniqueName: entraGroup.name + displayName: entraGroup.name + mailEnabled: false + mailNickname: entraGroup.name + securityEnabled: true + members: [ + for (member, i) in entraGroup.members: member.type =~ 'UserAssignedManagedIdentity' + ? memberManagedIdentities[i].properties.principalId + : member.type =~ 'Application' + ? memberServicePrincipals[i].id + : member.type =~ 'ServicePrincipal' + ? memberServicePrincipalsStandalone[i].id + : member.type =~ 'Group' ? memberGroups[i].id : member.type =~ 'PrincipalId' ? member.principalId : '' + ] + owners: [ + for (owner, i) in entraGroup.owners: owner.type =~ 'UserAssignedManagedIdentity' + ? ownerManagedIdentities[i].properties.principalId + : owner.type =~ 'Application' + ? ownerServicePrincipals[i].id + : owner.type =~ 'ServicePrincipal' ? ownerServicePrincipalsStandalone[i].id : owner.type =~ 'PrincipalId' ? owner.principalId : '' + ] + } + """); + + result.Should().NotHaveAnyDiagnostics(); + result.Template.Should().NotBeNull(); + result.Template.Should().HaveJsonAtPath("$.resources.entraGroupRes.dependsOn", """ + [ + "memberGroups", + "memberManagedIdentities", + "memberServicePrincipals", + "memberServicePrincipalsStandalone", + "ownerManagedIdentities", + "ownerServicePrincipals", + "ownerServicePrincipalsStandalone" + ] + """); + } + + [TestMethod] + public void Test_Issue15686_repro() + { + var result = CompilationHelper.Compile(""" + param eventSubscriptionName string + param eventTypes string[] + param eventGridTopicName string + param backendAppId string + param functionAppName string + param functionName string + + resource functionApp 'Microsoft.Web/sites@2023-01-01' existing = { + name: functionAppName + } + + resource eventGridTopic 'Microsoft.EventGrid/topics@2022-06-15' existing = { + name: eventGridTopicName + } + + resource eventSubscription 'Microsoft.EventGrid/eventSubscriptions@2022-06-15' = { + name: eventSubscriptionName + scope: eventGridTopic + properties: { + eventDeliverySchema: 'EventGridSchema' + filter: { + enableAdvancedFilteringOnArrays: true + includedEventTypes: eventTypes + } + destination: { + endpointType: 'WebHook' + properties: { + maxEventsPerBatch: 1 + azureActiveDirectoryApplicationIdOrUri: backendAppId + azureActiveDirectoryTenantId: subscription().tenantId + endpointUrl: 'https://${functionApp.properties.defaultHostName}/runtime/webhooks/EventGrid?functionName=${functionName}&code=${listkeys('${functionApp.id}/host/default', '2016-08-01').systemkeys.eventgrid_extension}' + } + } + retryPolicy: { + eventTimeToLiveInMinutes: 65 + } + } + } + """); + + result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics(); + result.Template.Should().NotBeNull(); + result.Template.Should().HaveJsonAtPath("$.resources.eventSubscription.dependsOn", """ + [ + "eventGridTopic", + "functionApp" + ] + """); + } } diff --git a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.ir.bicep b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.ir.bicep index fc70dd8c8a0..2d9b1902c89 100644 --- a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.ir.bicep +++ b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.ir.bicep @@ -1,5 +1,7 @@ resource basicParent 'My.Rp/parentType@2020-12-01' = { //@[00:2073) ProgramExpression +//@[00:0000) | ├─ResourceDependencyExpression [UNPARENTED] +//@[00:0000) | | └─ResourceReferenceExpression [UNPARENTED] //@[00:0000) | └─ResourceDependencyExpression [UNPARENTED] //@[00:0000) | └─ResourceReferenceExpression [UNPARENTED] //@[00:0000) | └─ResourceDependencyExpression [UNPARENTED] @@ -8,6 +10,8 @@ resource basicParent 'My.Rp/parentType@2020-12-01' = { //@[00:0000) | | └─ResourceReferenceExpression [UNPARENTED] //@[00:0000) | └─ResourceDependencyExpression [UNPARENTED] //@[00:0000) | └─ResourceReferenceExpression [UNPARENTED] +//@[00:0000) | ├─ResourceDependencyExpression [UNPARENTED] +//@[00:0000) | | └─ResourceReferenceExpression [UNPARENTED] //@[00:0000) | └─ResourceDependencyExpression [UNPARENTED] //@[00:0000) | └─ResourceReferenceExpression [UNPARENTED] //@[00:0000) | └─ResourceDependencyExpression [UNPARENTED] diff --git a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.json b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.json index bbd9ddba5bb..fd6f6c79b60 100644 --- a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.json +++ b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.json @@ -5,7 +5,7 @@ "_generator": { "name": "bicep", "version": "dev", - "templateHash": "15295876813466684932" + "templateHash": "13207151182888561919" } }, "parameters": { @@ -35,7 +35,8 @@ "style": "[reference(resourceId('My.Rp/parentType/childType', 'basicParent', 'basicChild'), '2020-12-01').style]" }, "dependsOn": [ - "[resourceId('My.Rp/parentType/childType', 'basicParent', 'basicChild')]" + "[resourceId('My.Rp/parentType/childType', 'basicParent', 'basicChild')]", + "[resourceId('My.Rp/parentType', 'basicParent')]" ] }, { @@ -82,7 +83,8 @@ "style": "[reference(resourceId('My.Rp/parentType/childType', 'conditionParent', 'conditionChild'), '2020-12-01').style]" }, "dependsOn": [ - "[resourceId('My.Rp/parentType/childType', 'conditionParent', 'conditionChild')]" + "[resourceId('My.Rp/parentType/childType', 'conditionParent', 'conditionChild')]", + "[resourceId('My.Rp/parentType', 'conditionParent')]" ] }, { diff --git a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.sourcemap.bicep b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.sourcemap.bicep index 5f268a044c3..2beeb0f28ea 100644 --- a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.sourcemap.bicep +++ b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.sourcemap.bicep @@ -37,7 +37,8 @@ resource basicParent 'My.Rp/parentType@2020-12-01' = { //@ "apiVersion": "2020-12-01", //@ "name": "[format('{0}/{1}/{2}', 'basicParent', 'basicChild', 'basicGrandchild')]", //@ "dependsOn": [ -//@ "[resourceId('My.Rp/parentType/childType', 'basicParent', 'basicChild')]" +//@ "[resourceId('My.Rp/parentType/childType', 'basicParent', 'basicChild')]", +//@ "[resourceId('My.Rp/parentType', 'basicParent')]" //@ ] //@ }, name: 'basicGrandchild' @@ -151,7 +152,8 @@ resource conditionParent 'My.Rp/parentType@2020-12-01' = if (createParent) { //@ "apiVersion": "2020-12-01", //@ "name": "[format('{0}/{1}/{2}', 'conditionParent', 'conditionChild', 'conditionGrandchild')]", //@ "dependsOn": [ -//@ "[resourceId('My.Rp/parentType/childType', 'conditionParent', 'conditionChild')]" +//@ "[resourceId('My.Rp/parentType/childType', 'conditionParent', 'conditionChild')]", +//@ "[resourceId('My.Rp/parentType', 'conditionParent')]" //@ ] //@ }, name: 'conditionGrandchild' diff --git a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.symbolicnames.json b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.symbolicnames.json index 22c63d37b0d..7a5d6c9c527 100644 --- a/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.symbolicnames.json +++ b/src/Bicep.Core.Samples/Files/baselines/NestedResources_LF/main.symbolicnames.json @@ -6,7 +6,7 @@ "_generator": { "name": "bicep", "version": "dev", - "templateHash": "16972434704035043060" + "templateHash": "9375757315581642416" } }, "parameters": { @@ -36,7 +36,8 @@ "style": "[reference('basicParent::basicChild').style]" }, "dependsOn": [ - "basicParent::basicChild" + "basicParent::basicChild", + "basicParent" ] }, "basicParent::basicChild": { @@ -73,7 +74,8 @@ "style": "[reference('existingParent::existingChild').style]" }, "dependsOn": [ - "existingParent::existingChild" + "existingParent::existingChild", + "existingParent" ] }, "existingParent::existingChild": { @@ -95,7 +97,8 @@ "style": "[reference('conditionParent::conditionChild').style]" }, "dependsOn": [ - "conditionParent::conditionChild" + "conditionParent::conditionChild", + "conditionParent" ] }, "conditionParent::conditionChild": { diff --git a/src/Bicep.Core.Samples/Files/user_submitted/101/template-spec-deploy/main.symbolicnames.json b/src/Bicep.Core.Samples/Files/user_submitted/101/template-spec-deploy/main.symbolicnames.json index 6c0c89c4180..4617215c39b 100644 --- a/src/Bicep.Core.Samples/Files/user_submitted/101/template-spec-deploy/main.symbolicnames.json +++ b/src/Bicep.Core.Samples/Files/user_submitted/101/template-spec-deploy/main.symbolicnames.json @@ -6,7 +6,7 @@ "_generator": { "name": "bicep", "version": "dev", - "templateHash": "15369804211876997670" + "templateHash": "1118487364434069107" } }, "parameters": { @@ -57,7 +57,7 @@ "parameters": {} }, "dependsOn": [ - "ts" + "ts::tsVersion" ] } } diff --git a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoUnnecessaryDependsOnRuleTests.cs b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoUnnecessaryDependsOnRuleTests.cs index 3bec2f075a6..85ab17b4f2d 100644 --- a/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoUnnecessaryDependsOnRuleTests.cs +++ b/src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoUnnecessaryDependsOnRuleTests.cs @@ -300,6 +300,42 @@ public void If_DuplicateEntries_ShouldFailForEach() ); } + [TestMethod] + public void If_DuplicateEntries_WhereOneValid_ShouldFailForLast() + { + CompileAndTest( + """ + resource otherVn 'Microsoft.Network/virtualNetworks@2020-06-01' = { + name: 'otherVn' + } + + resource vn 'Microsoft.Network/virtualNetworks@2020-06-01' existing = { + name: 'vn' + + resource subnet1 'subnets@2020-06-01' = { + name: 'subnet1' + properties: { + addressPrefix: '10.0.1.0/24' + } + } + + resource subnet2 'subnets@2020-06-01' = { + name: 'subnet2' + properties: { + addressPrefix: '10.0.1.0/24' + } + dependsOn: [ + otherVn + subnet1 + otherVn + ] + } + } + """, + OnCompileErrors.IncludeErrors, + ["Remove unnecessary dependsOn entry 'otherVn'."]); + } + [TestMethod] public void If_Explicit_DependsOn_ToParent_FromGrandChild_UsingColonNotation_ShouldFail() { @@ -452,29 +488,28 @@ public void If_ReferencesResourceCollection_Should_IgnoreAndPass() ); } - // TODO: We don't currently support analyzing dependencies to modules - //[TestMethod] - //public void If_Unnecessary_DependsOn_ForModule_Should_Fail() - //{ - // CompileAndTest(@" - // module m1 'module.bicep' = { - // name: 'm1' - // dependsOn: [] - // } - - // resource vn 'Microsoft.Network/virtualNetworks@2021-02-01' = [for i in range(0, 1): { - // name: '${m1.name}${i}' - // dependsOn: [ - // m1 // fails - // ] - // }] - // ", - // OnCompileErrors.Ignore, // Will get an error about not finding the module - // new string[] { - // "Remove unnecessary dependsOn entry 'm1'." - // } - // ); - //} + [TestMethod] + public void If_Unnecessary_DependsOn_ForModule_Should_Fail() + { + CompileAndTest(@" + module m1 'module.bicep' = { + name: 'm1' + dependsOn: [] + } + + resource vn 'Microsoft.Network/virtualNetworks@2021-02-01' = [for i in range(0, 1): { + name: '${m1.name}${i}' + dependsOn: [ + m1 // fails + ] + }] + ", + OnCompileErrors.Ignore, // Will get an error about not finding the module + [ + "Remove unnecessary dependsOn entry 'm1'." + ] + ); + } [TestMethod] public void TolerantOfSyntaxErrors_1() diff --git a/src/Bicep.Core/Analyzers/Linter/Rules/NoUnnecessaryDependsOnRule.cs b/src/Bicep.Core/Analyzers/Linter/Rules/NoUnnecessaryDependsOnRule.cs index 59ccfe6f012..87b1d7a9b97 100644 --- a/src/Bicep.Core/Analyzers/Linter/Rules/NoUnnecessaryDependsOnRule.cs +++ b/src/Bicep.Core/Analyzers/Linter/Rules/NoUnnecessaryDependsOnRule.cs @@ -7,6 +7,7 @@ using Bicep.Core.Emit; using Bicep.Core.Semantics; using Bicep.Core.Syntax; +using Microsoft.WindowsAzure.ResourceStack.Common.Extensions; namespace Bicep.Core.Analyzers.Linter.Rules { @@ -28,12 +29,7 @@ public override string FormatMessage(params object[] values) public override IEnumerable AnalyzeInternal(SemanticModel model, DiagnosticLevel diagnosticLevel) { - Lazy>> inferredDependenciesMap = - new( - () => ResourceDependencyVisitor.GetResourceDependencies( - model, - new ResourceDependencyVisitor.Options { IgnoreExplicitDependsOn = true })); - var visitor = new ResourceVisitor(this, inferredDependenciesMap, model, diagnosticLevel); + var visitor = new ResourceVisitor(this, model, diagnosticLevel); visitor.Visit(model.SourceFile.ProgramSyntax); return visitor.diagnostics; } @@ -47,10 +43,10 @@ private class ResourceVisitor : AstVisitor private readonly SemanticModel model; private readonly DiagnosticLevel diagnosticLevel; - public ResourceVisitor(NoUnnecessaryDependsOnRule parent, Lazy>> inferredDependenciesMap, SemanticModel model, DiagnosticLevel diagnosticLevel) + public ResourceVisitor(NoUnnecessaryDependsOnRule parent, SemanticModel model, DiagnosticLevel diagnosticLevel) { this.parent = parent; - this.inferredDependenciesMap = inferredDependenciesMap; + this.inferredDependenciesMap = new(BuildDependencyGraph); this.model = model; this.diagnosticLevel = diagnosticLevel; } @@ -75,6 +71,44 @@ public override void VisitResourceDeclarationSyntax(ResourceDeclarationSyntax sy base.VisitResourceDeclarationSyntax(syntax); } + private ImmutableDictionary> BuildDependencyGraph() + { + var directDependencyGraph = ResourceDependencyVisitor.GetResourceDependencies( + model, + new() { IgnoreExplicitDependsOn = true }); + var builder = ImmutableDictionary.CreateBuilder>(); + + foreach (var kvp in directDependencyGraph) + { + if (kvp.Key is VariableSymbol) + { + continue; + } + + var allDependencies = ImmutableHashSet.CreateBuilder(); + Queue dependenciesToWalk = new(kvp.Value); + + while (dependenciesToWalk.TryDequeue(out var dependency)) + { + if (allDependencies.Contains(dependency)) + { + continue; + } + + if (directDependencyGraph.TryGetValue(dependency.Resource, out var transitiveDependencies)) + { + dependenciesToWalk.EnqueueRange(transitiveDependencies); + } + + allDependencies.Add(dependency); + } + + builder[kvp.Key] = allDependencies.ToImmutable(); + } + + return builder.ToImmutable(); + } + private void VisitResourceOrModuleDeclaration(SyntaxBase declaringSyntax, ObjectSyntax body) { var dependsOnProperty = body.TryGetPropertyByName(LanguageConstants.ResourceDependsOnPropertyName); @@ -86,17 +120,21 @@ private void VisitResourceOrModuleDeclaration(SyntaxBase declaringSyntax, Object return; } + HashSet explicitDependencies = new(); + foreach (ArrayItemSyntax declaredDependency in declaredDependencies.Items) { - if (model.GetSymbolInfo(declaredDependency.Value) is not ResourceSymbol referencedResource || + var symbolInfo = model.GetSymbolInfo(declaredDependency.Value); + if (symbolInfo is not DeclaredSymbol referent || // Ignore dependsOn entries pointing to a resource collection - dependency analysis would // be complex and user probably knows what they're doing. - referencedResource.IsCollection) + (symbolInfo as ResourceSymbol)?.IsCollection is true || + (symbolInfo as ModuleSymbol)?.IsCollection is true) { continue; } - if (!inferredDependencies.Any(d => d.Resource == referencedResource)) + if (!inferredDependencies.Any(d => d.Resource == referent) && explicitDependencies.Add(referent)) { // There are no inferred dependencies - the dependsOn entry is valid continue; @@ -128,7 +166,7 @@ private void VisitResourceOrModuleDeclaration(SyntaxBase declaringSyntax, Object parent.CreateDiagnosticForSpan( diagnosticLevel, declaredDependency.Span, - referencedResource.Name)); + referent.Name)); } else { @@ -137,7 +175,7 @@ private void VisitResourceOrModuleDeclaration(SyntaxBase declaringSyntax, Object diagnosticLevel, declaredDependency.Span, new CodeFix(CoreResources.NoUnnecessaryDependsOnRuleCodeFix, isPreferred: true, CodeFixKind.QuickFix, codeReplacement), - referencedResource.Name)); + referent.Name)); } } } diff --git a/src/Bicep.Core/Emit/EmitterContext.cs b/src/Bicep.Core/Emit/EmitterContext.cs index 99ba022b789..3ad961c3917 100644 --- a/src/Bicep.Core/Emit/EmitterContext.cs +++ b/src/Bicep.Core/Emit/EmitterContext.cs @@ -20,7 +20,7 @@ public EmitterContext(SemanticModel semanticModel) SemanticModel = semanticModel; DataFlowAnalyzer = new(semanticModel); VariablesToInline = InlineDependencyVisitor.GetVariablesToInline(semanticModel); - ResourceDependencies = ResourceDependencyVisitor.GetResourceDependencies(semanticModel, new() { IncludeExisting = Settings.EnableSymbolicNames }); + ResourceDependencies = ResourceDependencyVisitor.GetResourceDependencies(semanticModel); FunctionVariables = FunctionVariableGeneratorVisitor.GetFunctionVariables(semanticModel); importClosureInfoLazy = new(() => ImportClosureInfo.Calculate(semanticModel), LazyThreadSafetyMode.PublicationOnly); } diff --git a/src/Bicep.Core/Emit/ResourceDependency.cs b/src/Bicep.Core/Emit/ResourceDependency.cs index a313d67fd06..9a98538a530 100644 --- a/src/Bicep.Core/Emit/ResourceDependency.cs +++ b/src/Bicep.Core/Emit/ResourceDependency.cs @@ -8,5 +8,4 @@ namespace Bicep.Core.Emit; public record ResourceDependency( DeclaredSymbol Resource, - SyntaxBase? IndexExpression, - ResourceDependencyKind Kind); + SyntaxBase? IndexExpression); diff --git a/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs b/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs index c6a11e1c5e2..c07d9a5a4ee 100644 --- a/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs +++ b/src/Bicep.Core/Emit/ResourceDependencyVisitor.cs @@ -7,6 +7,7 @@ using Bicep.Core.Semantics; using Bicep.Core.Semantics.Metadata; using Bicep.Core.Syntax; +using Bicep.Core.TypeSystem; namespace Bicep.Core.Emit { @@ -17,12 +18,10 @@ public class ResourceDependencyVisitor : AstVisitor private readonly IDictionary> resourceDependencies; private DeclaredSymbol? currentDeclaration; - public struct Options { // If true, only inferred dependencies will be returned, not those declared explicitly by dependsOn entries public bool? IgnoreExplicitDependsOn; - public bool? IncludeExisting; } /// @@ -35,24 +34,28 @@ public static ImmutableDictionary>(); - foreach (var kvp in visitor.resourceDependencies) + Queue nonInlinedVariables = new(visitor.resourceDependencies + .Where(kvp => kvp.Value.Count == 0) + .Select(kvp => kvp.Key) + .OfType()); + + while (nonInlinedVariables.TryDequeue(out var nonInlinedVariable)) { - if (kvp.Key is ResourceSymbol || kvp.Key is ModuleSymbol) + foreach (var kvp in visitor.resourceDependencies) { - output[kvp.Key] = OptimizeDependencies(kvp.Value); + if (kvp.Value.RemoveWhere(d => ReferenceEquals(nonInlinedVariable, d.Resource)) > 0 && + kvp.Value.Count == 0 && + kvp.Key is VariableSymbol variable) + { + nonInlinedVariables.Enqueue(variable); + } } } - return output.ToImmutableDictionary(); - } - private static ImmutableHashSet OptimizeDependencies(HashSet dependencies) => - dependencies - .GroupBy(dep => dep.Resource) - .SelectMany(group => @group.FirstOrDefault(dep => dep.IndexExpression == null) is { } dependencyWithoutIndex - ? dependencyWithoutIndex.AsEnumerable() - : @group) - .ToImmutableHashSet(); + return visitor.resourceDependencies.ToImmutableDictionary( + kvp => kvp.Key, + kvp => kvp.Value.ToImmutableHashSet()); + } private ResourceDependencyVisitor(SemanticModel model, Options? options) { @@ -64,36 +67,23 @@ private ResourceDependencyVisitor(SemanticModel model, Options? options) public override void VisitResourceDeclarationSyntax(ResourceDeclarationSyntax syntax) { - int GetIndexOfAncestor(ImmutableArray ancestors) - { - for (int i = ancestors.Length - 1; i >= 0; i--) - { - if (!ancestors[i].Resource.IsExistingResource || options?.IncludeExisting == true) - { - // we found the non-existing resource - we're done - return i; - } - } - - // no non-existing resources are found in the ancestors list - return -1; - } - if (model.ResourceMetadata.TryLookup(syntax) is not DeclaredResourceMetadata resource) { // When invoked by BicepDeploymentGraphHandler, it's possible that the declaration is unbound. return; } - // Resource ancestors are always dependencies. - var ancestors = this.model.ResourceAncestors.GetAncestors(resource); - var lastAncestorIndex = GetIndexOfAncestor(ancestors); + HashSet dependencies = new(); + if (model.ResourceAncestors.GetAncestors(resource).LastOrDefault() is { } parent) + { + dependencies.Add(new(parent.Resource.Symbol, parent.IndexExpression)); + } + resourceDependencies[resource.Symbol] = dependencies; // save previous declaration as we may call this recursively var prevDeclaration = this.currentDeclaration; - this.currentDeclaration = resource.Symbol; - this.resourceDependencies[resource.Symbol] = new HashSet(ancestors.Select((a, i) => new ResourceDependency(a.Resource.Symbol, a.IndexExpression, i == lastAncestorIndex ? ResourceDependencyKind.Primary : ResourceDependencyKind.Transitive))); + base.VisitResourceDeclarationSyntax(syntax); // restore previous declaration @@ -136,22 +126,6 @@ public override void VisitVariableDeclarationSyntax(VariableDeclarationSyntax sy this.currentDeclaration = prevDeclaration; } - private IEnumerable GetResourceDependencies(DeclaredSymbol declaredSymbol) - { - if (!resourceDependencies.TryGetValue(declaredSymbol, out var dependencies)) - { - // recursively visit dependent variables - this.Visit(declaredSymbol.DeclaringSyntax); - - if (!resourceDependencies.TryGetValue(declaredSymbol, out dependencies)) - { - return []; - } - } - - return dependencies; - } - public override void VisitVariableAccessSyntax(VariableAccessSyntax syntax) { if (currentDeclaration is null) @@ -168,29 +142,31 @@ public override void VisitVariableAccessSyntax(VariableAccessSyntax syntax) switch (model.GetSymbolInfo(syntax)) { case VariableSymbol variableSymbol: - var varDependencies = GetResourceDependencies(variableSymbol); - - currentResourceDependencies.UnionWith(varDependencies); + currentResourceDependencies.Add(new(variableSymbol, GetIndexExpression(syntax, variableSymbol.IsCopyVariable))); return; case ResourceSymbol resourceSymbol: - if (resourceSymbol.DeclaringResource.IsExistingResource() && options?.IncludeExisting != true) - { - var existingDependencies = GetResourceDependencies(resourceSymbol); - - currentResourceDependencies.UnionWith(existingDependencies); - return; - } - - currentResourceDependencies.Add(new ResourceDependency(resourceSymbol, GetIndexExpression(syntax, resourceSymbol.IsCollection), ResourceDependencyKind.Primary)); + currentResourceDependencies.Add(new(resourceSymbol, GetIndexExpression(syntax, resourceSymbol.IsCollection))); return; case ModuleSymbol moduleSymbol: - currentResourceDependencies.Add(new ResourceDependency(moduleSymbol, GetIndexExpression(syntax, moduleSymbol.IsCollection), ResourceDependencyKind.Primary)); + currentResourceDependencies.Add(new(moduleSymbol, GetIndexExpression(syntax, moduleSymbol.IsCollection))); return; } } + private ObjectPropertySyntax? TryGetCurrentDeclarationTopLevelProperty(string propertyName) + { + ObjectSyntax? declaringSyntax = this.currentDeclaration switch + { + ResourceSymbol resourceSymbol => (resourceSymbol.DeclaringSyntax as ResourceDeclarationSyntax)?.TryGetBody(), + ModuleSymbol moduleSymbol => (moduleSymbol.DeclaringSyntax as ModuleDeclarationSyntax)?.TryGetBody(), + _ => null + }; + + return declaringSyntax?.TryGetPropertyByName(propertyName); + } + public override void VisitResourceAccessSyntax(ResourceAccessSyntax syntax) { if (currentDeclaration is null) @@ -207,19 +183,11 @@ public override void VisitResourceAccessSyntax(ResourceAccessSyntax syntax) switch (model.GetSymbolInfo(syntax)) { case ResourceSymbol resourceSymbol: - if (resourceSymbol.DeclaringResource.IsExistingResource()) - { - var existingDependencies = GetResourceDependencies(resourceSymbol); - - currentResourceDependencies.UnionWith(existingDependencies); - return; - } - - currentResourceDependencies.Add(new ResourceDependency(resourceSymbol, GetIndexExpression(syntax, resourceSymbol.IsCollection), ResourceDependencyKind.Primary)); + currentResourceDependencies.Add(new(resourceSymbol, GetIndexExpression(syntax, resourceSymbol.IsCollection))); return; case ModuleSymbol moduleSymbol: - currentResourceDependencies.Add(new ResourceDependency(moduleSymbol, GetIndexExpression(syntax, moduleSymbol.IsCollection), ResourceDependencyKind.Primary)); + currentResourceDependencies.Add(new(moduleSymbol, GetIndexExpression(syntax, moduleSymbol.IsCollection))); return; } } @@ -247,7 +215,7 @@ public override void VisitResourceAccessSyntax(ResourceAccessSyntax syntax) { ResourceSymbol resourceSymbol => resourceSymbol.DeclaringResource.GetBody(), ModuleSymbol moduleSymbol => moduleSymbol.DeclaringModule.GetBody(), - VariableSymbol variableSymbol => variableSymbol.DeclaringVariable.Value, + VariableSymbol variableSymbol => variableSymbol.DeclaringVariable.GetBody(), _ => throw new NotImplementedException($"Unexpected current declaration type '{this.currentDeclaration?.GetType().Name}'.") }; @@ -271,7 +239,7 @@ public override void VisitObjectPropertySyntax(ObjectPropertySyntax propertySynt if (propertySyntax.Key is IdentifierSyntax key && key.NameEquals(LanguageConstants.ResourceDependsOnPropertyName)) { // ... that is the a top-level resource or module property? - if (this.IsTopLevelPropertyOfCurrentDeclaration(propertySyntax)) + if (ReferenceEquals(TryGetCurrentDeclarationTopLevelProperty(key.IdentifierName), propertySyntax)) { // Yes - don't include dependencies from this property value return; @@ -281,18 +249,5 @@ public override void VisitObjectPropertySyntax(ObjectPropertySyntax propertySynt base.VisitObjectPropertySyntax(propertySyntax); } - - private bool IsTopLevelPropertyOfCurrentDeclaration(ObjectPropertySyntax propertySyntax) - { - SyntaxBase? declaringSyntax = this.currentDeclaration switch - { - ResourceSymbol resourceSymbol => (resourceSymbol.DeclaringSyntax as ResourceDeclarationSyntax)?.TryGetBody(), - ModuleSymbol moduleSymbol => (moduleSymbol.DeclaringSyntax as ModuleDeclarationSyntax)?.TryGetBody(), - _ => null - }; - IEnumerable? currentDeclarationProperties = (declaringSyntax as ObjectSyntax)?.Properties; - - return currentDeclarationProperties?.Contains(propertySyntax) ?? false; - } } } diff --git a/src/Bicep.Core/Intermediate/ExpressionBuilder.cs b/src/Bicep.Core/Intermediate/ExpressionBuilder.cs index aa80282b5af..0536e6167bd 100644 --- a/src/Bicep.Core/Intermediate/ExpressionBuilder.cs +++ b/src/Bicep.Core/Intermediate/ExpressionBuilder.cs @@ -617,12 +617,6 @@ private DeclaredModuleExpression ConvertModule(ModuleDeclarationSyntax syntax) GetBatchSize(syntax)); } - var dependencies = Context.ResourceDependencies[symbol] - .Where(ShouldGenerateDependsOn) - .OrderBy(x => x.Resource.Name) // order to generate a deterministic template - .Select(x => ToDependencyExpression(x, body)) - .ToImmutableArray(); - return new DeclaredModuleExpression( syntax, symbol, @@ -630,9 +624,20 @@ private DeclaredModuleExpression ConvertModule(ModuleDeclarationSyntax syntax) body, bodyExpression, parameters is not null ? ConvertWithoutLowering(parameters.Value) : null, - dependencies); + BuildDependencyExpressions(symbol, body)); } + private ImmutableArray BuildDependencyExpressions(DeclaredSymbol dependent, SyntaxBase body) + => Context.ResourceDependencies[dependent] + .SelectMany(dd => ToDependencyExpressions(dd, body, Context.ResourceDependencies)) + .GroupBy(t => t.Target.Resource) + .SelectMany(g => g.FirstOrDefault(t => t.Target.IndexExpression is null) is { } dependencyOnCollection + ? dependencyOnCollection.AsEnumerable() + : g.Distinct(t => t.Target)) + .OrderBy(t => t.Target.Resource.Name) // order to generate a deterministic template + .Select(t => t.Expression) + .ToImmutableArray(); + private DeclaredResourceExpression ConvertResource(ResourceDeclarationSyntax syntax) { var resource = Context.SemanticModel.ResourceMetadata.TryLookup(syntax) as DeclaredResourceMetadata @@ -725,19 +730,13 @@ void AddLoop(LoopExpressionContext newLoop) GetBatchSize(syntax)); } - var dependencies = Context.ResourceDependencies[resource.Symbol] - .Where(ShouldGenerateDependsOn) - .OrderBy(x => x.Resource.Name) // order to generate a deterministic template - .Select(x => ToDependencyExpression(x, body)) - .ToImmutableArray(); - return new DeclaredResourceExpression( syntax, resource, Context.ResourceScopeData[resource], body, bodyExpression, - dependencies); + BuildDependencyExpressions(resource.Symbol, body)); } private Expression ConvertArray(ArraySyntax array) @@ -1303,51 +1302,149 @@ private static SyntaxBase GetModuleNameSyntax(ModuleSymbol moduleSymbol) return null; } - private bool ShouldGenerateDependsOn(ResourceDependency dependency) + private record DependencyExpression( + ResourceDependency Target, + ResourceDependencyExpression Expression); + + private IEnumerable ToDependencyExpressions( + ResourceDependency dependency, + SyntaxBase newContext, + ImmutableDictionary> dependencies) { - if (dependency.Kind == ResourceDependencyKind.Transitive) + foreach (var path in GatherDependencyPaths([dependency], dependencies)) { - // transitive dependencies do not have to be emitted - return false; + Expression reference; + ResourceDependency target; + var localReplacements = this.localReplacements; + var allNodesInPathAccessCopyIndex = true; + + int i = 0; + do + { + target = path[i]; + var targetContext = i == 0 ? newContext : path[i - 1].Resource.DeclaringSyntax; + IndexReplacementContext? indexContext = null; + + switch (target.Resource) + { + case ResourceSymbol resource: + { + var metadata = Context.SemanticModel.ResourceMetadata.TryLookup(resource.DeclaringSyntax) as DeclaredResourceMetadata + ?? throw new InvalidOperationException("Failed to find resource in cache"); + + indexContext = (resource.IsCollection && target.IndexExpression is null) + ? null + : new ExpressionBuilder(Context, localReplacements) + .TryGetReplacementContext(metadata, target.IndexExpression, targetContext); + reference = new ResourceReferenceExpression(null, metadata, indexContext); + break; + } + case ModuleSymbol module: + { + indexContext = (module.IsCollection && target.IndexExpression is null) + ? null + : new ExpressionBuilder(Context, localReplacements) + .TryGetReplacementContext(module, target.IndexExpression, targetContext); + reference = new ModuleReferenceExpression(null, module, indexContext); + break; + } + case VariableSymbol variable: + { + indexContext = (variable.IsCopyVariable && target.IndexExpression is null) + ? null + : new ExpressionBuilder(Context, localReplacements) + .TryGetReplacementContext(variable.DeclaringVariable.GetBody(), target.IndexExpression, targetContext); + reference = new VariableReferenceExpression(null, variable); + break; + } + default: + throw new InvalidOperationException($"Found dependency '{target.Resource.Name}' of unexpected type {target.Resource.GetType()}"); + } + + localReplacements = indexContext?.LocalReplacements ?? localReplacements; + var copyIndexAccesses = indexContext is not null + ? CopyIndexExpressionCollector.FindContainedCopyIndexExpressions(indexContext.Index) + : []; + + if (copyIndexAccesses.Length > 0 && !allNodesInPathAccessCopyIndex) + { + target = target with { IndexExpression = null }; + reference = reference switch + { + ModuleReferenceExpression modRef => modRef with { IndexContext = null }, + ResourceReferenceExpression resourceRef => resourceRef with { IndexContext = null }, + _ => reference, + }; + } + + allNodesInPathAccessCopyIndex = allNodesInPathAccessCopyIndex && copyIndexAccesses.Length > 0; + } while (++i < path.Length); + + yield return new(target, new(null, reference)); } + } + + private class CopyIndexExpressionCollector : ExpressionVisitor + { + private readonly ImmutableArray.Builder copyIndexExpressions + = ImmutableArray.CreateBuilder(); + + private CopyIndexExpressionCollector() { } - return dependency.Resource switch + public static ImmutableArray FindContainedCopyIndexExpressions(Expression? expression) { - // 'existing' resources are only represented in the JSON if using symbolic names. - ResourceSymbol resource => Context.Settings.EnableSymbolicNames || !resource.DeclaringResource.IsExistingResource(), - ModuleSymbol => true, - _ => throw new InvalidOperationException($"Found dependency '{dependency.Resource.Name}' of unexpected type {dependency.GetType()}"), - }; + if (expression is null) + { +#pragma warning disable IDE0301 // Using a simplified collection initialization results in an allocation, whereas using ImmutableArray.Empty does not + return ImmutableArray.Empty; +#pragma warning restore IDE0301 + } + + var visitor = new CopyIndexExpressionCollector(); + expression.Accept(visitor); + + return visitor.copyIndexExpressions.ToImmutable(); + } + + public override void VisitCopyIndexExpression(CopyIndexExpression expression) + { + copyIndexExpressions.Add(expression); + } } - private ResourceDependencyExpression ToDependencyExpression(ResourceDependency dependency, SyntaxBase newContext) + private IEnumerable> GatherDependencyPaths( + ImmutableArray currentPath, + ImmutableDictionary> dependencies) { - switch (dependency.Resource) + if (IsDependencyPathTerminus(currentPath[^1])) { - case ResourceSymbol resource: - { - var metadata = Context.SemanticModel.ResourceMetadata.TryLookup(resource.DeclaringSyntax) as DeclaredResourceMetadata - ?? throw new InvalidOperationException("Failed to find resource in cache"); - - var indexContext = (resource.IsCollection && dependency.IndexExpression is null) ? null : - TryGetReplacementContext(metadata, dependency.IndexExpression, newContext); + yield return currentPath; + yield break; + } - var reference = new ResourceReferenceExpression(null, metadata, indexContext); - return new ResourceDependencyExpression(null, reference); - } - case ModuleSymbol module: - { - var indexContext = (module.IsCollection && dependency.IndexExpression is null) ? null : - TryGetReplacementContext(module, dependency.IndexExpression, newContext); + foreach (var transitiveDependency in dependencies[currentPath[^1].Resource]) + { + if (currentPath.Contains(transitiveDependency)) + { + // We've got a cycle. This should have been caught and blocked earlier + throw new InvalidOperationException("Dependency cycle detected: " + + string.Join(" -> ", currentPath.Select(d => d.Resource.Name))); + } - var reference = new ModuleReferenceExpression(null, module, indexContext); - return new ResourceDependencyExpression(null, reference); - } - default: - throw new InvalidOperationException($"Found dependency '{dependency.Resource.Name}' of unexpected type {dependency.GetType()}"); + foreach (var transitivePath in GatherDependencyPaths(currentPath.Add(transitiveDependency), dependencies)) + { + yield return transitivePath; + } } } + private bool IsDependencyPathTerminus(ResourceDependency dependency) => dependency.Resource switch + { + ModuleSymbol => true, + ResourceSymbol r => !r.DeclaringResource.IsExistingResource() || Context.Settings.EnableSymbolicNames, + _ => false, + }; + /// /// 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. diff --git a/src/Bicep.Core/Semantics/VariableSymbol.cs b/src/Bicep.Core/Semantics/VariableSymbol.cs index 14da800f093..5029541d862 100644 --- a/src/Bicep.Core/Semantics/VariableSymbol.cs +++ b/src/Bicep.Core/Semantics/VariableSymbol.cs @@ -17,6 +17,14 @@ public VariableSymbol(ISymbolContext context, string name, VariableDeclarationSy public override SymbolKind Kind => SymbolKind.Variable; + public bool IsCopyVariable => UnwrapIfParenthesized(DeclaringVariable.Value) is ForSyntax; + + private static SyntaxBase UnwrapIfParenthesized(SyntaxBase syntax) => syntax switch + { + ParenthesizedExpressionSyntax parenthesized => UnwrapIfParenthesized(parenthesized.Expression), + _ => syntax, + }; + public override IEnumerable Descendants { get diff --git a/src/Bicep.Core/Syntax/VariableDeclarationSyntax.cs b/src/Bicep.Core/Syntax/VariableDeclarationSyntax.cs index 8e4b19a7098..4b37a2c3ce8 100644 --- a/src/Bicep.Core/Syntax/VariableDeclarationSyntax.cs +++ b/src/Bicep.Core/Syntax/VariableDeclarationSyntax.cs @@ -29,6 +29,19 @@ public VariableDeclarationSyntax(IEnumerable leadingNodes, Token key public SyntaxBase Value { get; } + public SyntaxBase? TryGetBody() => UnwrapBody(Value); + + private static SyntaxBase? UnwrapBody(SyntaxBase body) => body switch + { + SkippedTriviaSyntax => null, + ForSyntax @for => @for.Body, + ParenthesizedExpressionSyntax parenthesized => UnwrapBody(parenthesized.Expression), + var otherwise => otherwise, + }; + + public SyntaxBase GetBody() => TryGetBody() ?? + throw new InvalidOperationException($"A valid body is not available on this variable due to errors. Use {nameof(TryGetBody)}() instead."); + public override void Accept(ISyntaxVisitor visitor) => visitor.VisitVariableDeclarationSyntax(this); public override TextSpan Span => TextSpan.Between(this.LeadingNodes.FirstOrDefault() ?? Keyword, Value); diff --git a/src/Bicep.LangServer/Handlers/BicepDeploymentGraphHandler.cs b/src/Bicep.LangServer/Handlers/BicepDeploymentGraphHandler.cs index a91f2cb0f6f..0d68689aed5 100644 --- a/src/Bicep.LangServer/Handlers/BicepDeploymentGraphHandler.cs +++ b/src/Bicep.LangServer/Handlers/BicepDeploymentGraphHandler.cs @@ -70,7 +70,7 @@ public static BicepDeploymentGraph CreateDeploymentGraph(CompilationContext cont { var (semanticModel, filePath, parentId) = queue.Dequeue(); var nodesBySymbol = new Dictionary(); - var dependenciesBySymbol = ResourceDependencyVisitor.GetResourceDependencies(semanticModel, new ResourceDependencyVisitor.Options() { IncludeExisting = true }) + var dependenciesBySymbol = ResourceDependencyVisitor.GetResourceDependencies(semanticModel) .Where(x => x.Key.Name != LanguageConstants.MissingName && x.Key.Name != LanguageConstants.ErrorName) .ToImmutableDictionary(x => x.Key, x => x.Value); @@ -130,7 +130,7 @@ moduleSemanticModel is SemanticModel bicepModel && continue; } - foreach (var dependency in dependencies.Where(d => d.Kind == ResourceDependencyKind.Primary)) + foreach (var dependency in dependencies) { if (!nodesBySymbol.TryGetValue(dependency.Resource, out var target)) {