Skip to content

Commit e00b2fc

Browse files
kalbert312kalbert312
authored
Improve compiler detection of invalid runtime value usages. (Azure#10250)
* Remove the index expression check. * Add test cases for diagnostics against a non-deploy time expression in for body expressions. * Renames. * Update the test cases. * Remove this, merged into other test due to overlap. * Consider non-string usages as collection access. String usages would really be property based access. * Try to evaluate string literals up through variables when checking array access index expressions for dtc violations. * Add test for variable stack. * Add a baseline for runtime value tests. Pick a select set of permutations from the var for body scenario set for the baseline. * Remove some diagnostic noise. * Change the condition to just exclude integer literal syntax. The scope of this code should only be concerned about string-based property access and multiple types of syntax can evaluate to a string literal. Add more test cases. * Partially revert commit 5498623. Semantic model's get type info should be used instead. * Use semantic model GetTypeInfo to do index expression type checks to make DTC validation more robust. * Split the tests so it's more debuggable. Leave TODO about union checking. * Revert. * Update baselines. * Formatting. * Add string literal checking for all-string unions. Add tests. Remove the check for integer literal. I think string based property access is the only thing that needs to be checked here. If a resource is resolvable and something else is used, it's going to be a type error, so having the dtc diagnostic and type diagnostic visible at the same time will make it easier for the programmer to figure out what to do next. * Add tests for nested resource access. * Fix entire resource cases. * Replicate the logic changes from direct visitor to indirect visitor. Test progress. * Fix test cases expectation of indirect usages of param values to expect BCP78 instead of BCP182. * Fix copy-paste error.. this visitor was using this method instead. * Reduce duplication. * Add some more nested resource test variants. Remove redundant checks. * Action, not Func. * Fix incorrect diagnostic. * Refactor to function. * Add existing resource tests. * Add interpolated object property key tests. * Rename the tests. This does not apply to all for body expression contexts. * PR feedback: Move exhaustive cases to the baselines. * PR feedback: include the violating property name in the diagnostic. This will allow displaying multiple diagnostics if there's union types involved. * PR feedback: Reduce the complexity of the scenario tests. Also add the property name to the diagnostic for the indirect case. * Update baselines. * Update the json baseline files. Update README. * Add some expression variants. * Add a conditional resource collection accessor case. * Add a test for string array accessor with type any. any() is meant to resolve type errors while DTC is a runtime concept. * Fix gap of not checking for any or union of ints when checking dtc for whole resource access. * Fix typo.
1 parent bf6aac5 commit e00b2fc

File tree

65 files changed

+11783
-730
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

65 files changed

+11783
-730
lines changed

‎CONTRIBUTING.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ If you have an active branch pushed to your GitHub fork, you can use the "Update
7878
* The name of the entry should match the name of the folder you create (same casing), and there should be a main.bicep file in that folder.
7979
1. Make changes to main.bicep.
8080
1. Create empty `main.<suffix>.bicep` assertion files in the folder. You need to create following suffixes: `diagnostics`, `formatted`, `symbols`, `syntax`, `tokens`
81+
* If the dataset is expected to compile successfully, add additional bicep files with suffixes `ir`, `sourcemap` and two json files `main.json` and `main.symbolicnames.json` with initial content `{}`.
8182
1. Follow [Updating test baselines](#updating-test-baselines) to generate baseline files.
8283
* The naming and file structure is important here as it's used by the test runner to assert e.g. whether the example should compile, and the end-of-line characters.
8384
* For tests that deal with module, you may see some unexpected behavior because the test could be using a mock file resolver instead of the standard one. Similarly, some tests may be using a mock resource type provider instead of the standard ones - usually that explains why some types aren't recognized in tests.

‎src/Bicep.Core.IntegrationTests/DeployTimeConstantTests.cs‎

Lines changed: 145 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
3+
4+
using System.Collections.Generic;
5+
using System.Text;
36
using Bicep.Core.Diagnostics;
47
using Bicep.Core.UnitTests.Assertions;
58
using Bicep.Core.UnitTests.Utils;
@@ -10,6 +13,35 @@ namespace Bicep.Core.IntegrationTests
1013
[TestClass]
1114
public class DeployTimeConstantTests
1215
{
16+
private static string GetDtcValidationResourceBaseline()
17+
{
18+
return @"
19+
resource foo 'Microsoft.Storage/storageAccounts@2022-09-01' = {
20+
name: 'foo'
21+
location: 'westus'
22+
sku: {
23+
name: 'Standard_LRS'
24+
}
25+
kind: 'StorageV2'
26+
27+
resource fooChild 'fileServices' = {
28+
name: 'default'
29+
}
30+
}
31+
resource foos 'Microsoft.Storage/storageAccounts@2022-09-01' = [for i in range(0, 2): {
32+
name: 'foo-${i}'
33+
location: 'westus'
34+
sku: {
35+
name: 'Standard_LRS'
36+
}
37+
kind: 'StorageV2'
38+
}]
39+
resource existingFoo 'Microsoft.Storage/storageAccounts@2022-09-01' existing = {
40+
name: 'existingFoo'
41+
}
42+
";
43+
}
44+
1345
[TestMethod]
1446
public void DtcValidation_EntireResourceOrModuleAccessAtInvalidLocations_ProducesDiagnostics()
1547
{
@@ -41,7 +73,7 @@ public void DtcValidation_EntireResourceOrModuleAccessAtInvalidLocations_Produce
4173
resource dnsZone 'Microsoft.Network/dnsZones@2018-05-01' = {
4274
name: 'dnsZone'
4375
location: 'global'
44-
76+
4577
resource aRecord 'A' = {
4678
name: 'aRecord'
4779
}
@@ -73,7 +105,7 @@ public void DtcValidation_EntireResourceOrModuleAccessAtInvalidLocations_Produce
73105
74106
resource subnet2 'Microsoft.Network/virtualNetworks/subnets@2022-07-01' = {
75107
name: 'subnet2'
76-
parent: vnet
108+
parent: vnet
77109
properties: {
78110
addressPrefix: '10.0.2.0/24'
79111
}
@@ -126,5 +158,116 @@ public void DtcValidation_RuntimeValueAsDtcPropertyKey_ProducesDiagnostics()
126158
("BCP120", DiagnosticLevel.Error, "This expression is being used in an assignment to the \"tags\" property of the \"Microsoft.Web/serverfarms\" type, which requires a value that can be calculated at the start of the deployment. Properties of dnsZone which can be calculated at the start include \"apiVersion\", \"id\", \"name\", \"type\"."),
127159
});
128160
}
161+
162+
[TestMethod]
163+
public void DtcValidation_VarForBodyExpression_Ok()
164+
{
165+
StringBuilder textSb = new(GetDtcValidationResourceBaseline());
166+
textSb.Append(
167+
@"
168+
param strParam string = 'id'
169+
var idAccessor = 'id'
170+
var strArray = ['id', 'properties']
171+
var indirect = {
172+
prop: foo.id
173+
}
174+
"
175+
);
176+
177+
textSb.AppendLine(
178+
@"
179+
var okVarForBody1 = [for i in range(0, 2): foo.id]
180+
var okVarForBody2 = [for i in range(0, 2): foo['id']]
181+
var okVarForBody3 = [for i in range(0, 2): {
182+
prop: foo[idAccessor]
183+
}]
184+
var okVarForBody4 = [for i in range(0, 2): foo[strArray[0]]]
185+
var okVarForBody5 = [for i in range(0, 2): foos[0].id]
186+
var okVarForBody6 = [for i in range(0, 2): foos[i].name]
187+
var okVarForBody7 = [for i in range(0, 2): foos[i]['name']]
188+
var okVarForBody8 = [for i in range(0, 2): {
189+
'${foos[i].name}': foo.id
190+
}]
191+
var okVarForBody9 = [for i in range(0, 2): foos[i + 2]['${'name'}']]
192+
var okVarForBody10 = [for i in range(0, 2): indirect.prop]
193+
"
194+
);
195+
196+
var finalText = textSb.ToString();
197+
var result = CompilationHelper.Compile(finalText);
198+
199+
var filteredDiagnostics = result.WithFilteredDiagnostics(d => d.Level == DiagnosticLevel.Error);
200+
filteredDiagnostics.Should().NotHaveAnyDiagnostics();
201+
}
202+
203+
[TestMethod]
204+
public void DtcValidation_RuntimeValue_VarForBodyExpression_ProducesDiagnostics()
205+
{
206+
StringBuilder textSb = new(GetDtcValidationResourceBaseline());
207+
textSb.Append(
208+
@"
209+
param cond bool = false
210+
param strParam string = 'id'
211+
var propertiesAccessor = 'properties'
212+
var strArray = ['id', 'properties']
213+
var indirect = {
214+
prop: foo.properties
215+
}
216+
"
217+
);
218+
219+
var expectedDiagnostics = new List<(string, DiagnosticLevel, string)>();
220+
void AddExpectedDtcDiagnostic(string varNameOfForBody, string resourceVarName, string? violatingPropertyName = null)
221+
{
222+
expectedDiagnostics.Add(("BCP182", DiagnosticLevel.Error, $"This expression is being used in the for-body of the variable \"{varNameOfForBody}\", which requires values that can be calculated at the start of the deployment.{(violatingPropertyName != null ? $" The property \"{violatingPropertyName}\" of {resourceVarName} cannot be calculated at the start." : "")} Properties of {resourceVarName} which can be calculated at the start include \"apiVersion\", \"id\", \"name\", \"type\"."));
223+
}
224+
225+
void AddExpectedIndirectDtcDiagnostic(string varNameOfForBody, string resourceVarName, string dtcVariablePath, string? violatingPropertyName)
226+
{
227+
expectedDiagnostics.Add(("BCP182", DiagnosticLevel.Error, $"This expression is being used in the for-body of the variable \"{varNameOfForBody}\", which requires values that can be calculated at the start of the deployment. You are referencing a variable which cannot be calculated at the start {dtcVariablePath}.{(violatingPropertyName != null ? $" The property \"{violatingPropertyName}\" of {resourceVarName} cannot be calculated at the start." : "")} Properties of {resourceVarName} which can be calculated at the start include \"apiVersion\", \"id\", \"name\", \"type\"."));
228+
}
229+
230+
textSb.AppendLine("var badVarForBody1 = [for i in range(0, 2): foo.properties]");
231+
AddExpectedDtcDiagnostic("badVarForBody1", "foo", "properties");
232+
233+
textSb.AppendLine("var badVarForBody2 = [for i in range(0, 2): foo['properties']]");
234+
AddExpectedDtcDiagnostic("badVarForBody2", "foo", "properties");
235+
236+
textSb.AppendLine("var badVarForBody3 = [for i in range(0, 2): { prop: foo[propertiesAccessor].accessTier }]");
237+
AddExpectedDtcDiagnostic("badVarForBody3", "foo", "properties");
238+
239+
textSb.AppendLine("var badVarForBody4 = [for i in range(0, 2): foo::fooChild.properties]");
240+
AddExpectedDtcDiagnostic("badVarForBody4", "fooChild", "properties");
241+
242+
textSb.AppendLine("var badVarForBody5 = [for i in range(0, 2): foo[strParam]]");
243+
AddExpectedDtcDiagnostic("badVarForBody5", "foo");
244+
245+
textSb.AppendLine("var badVarForBody6 = [for i in range(0, 2): foos[i].properties]");
246+
AddExpectedDtcDiagnostic("badVarForBody6", "foos", "properties");
247+
248+
textSb.AppendLine("var badVarForBody7 = [for i in range(0, 2): foos[i + 2]['${'properties'}']]");
249+
AddExpectedDtcDiagnostic("badVarForBody7", "foos", "properties");
250+
251+
textSb.AppendLine("var badVarForBody8 = [for i in range(0, 2): foo[strArray[1]]]");
252+
AddExpectedDtcDiagnostic("badVarForBody8", "foo", "properties");
253+
254+
textSb.AppendLine("var badVarForBody9 = [for i in range(0, 2): indirect.prop]");
255+
AddExpectedIndirectDtcDiagnostic("badVarForBody9", "foo", "(\"indirect\" -> \"foo\")", "properties");
256+
257+
textSb.AppendLine(@"var badVarForBody10 = [for i in range(0, 2): {
258+
'${foos[i].properties.accessTier}': true
259+
}]");
260+
AddExpectedDtcDiagnostic("badVarForBody10", "foos", "properties");
261+
262+
textSb.AppendLine("var badVarForBody11 = [for i in range(0, 2): foo[cond ? 'properties' : 'extendedLocation']]");
263+
AddExpectedDtcDiagnostic("badVarForBody11", "foo", "extendedLocation");
264+
AddExpectedDtcDiagnostic("badVarForBody11", "foo", "properties");
265+
266+
var finalText = textSb.ToString();
267+
var result = CompilationHelper.Compile(finalText);
268+
269+
var filteredDiagnostics = result.WithFilteredDiagnostics(d => d.Level == DiagnosticLevel.Error);
270+
filteredDiagnostics.Should().HaveDiagnostics(expectedDiagnostics);
271+
}
129272
}
130273
}

‎src/Bicep.Core.Samples/DataSets.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
3+
34
using System.Collections.Generic;
45
using System.Collections.Immutable;
56
using System.Linq;
@@ -33,6 +34,10 @@ public static class DataSets
3334

3435
public static DataSet InvalidResources_CRLF => CreateDataSet();
3536

37+
public static DataSet InvalidRuntimeValueUsages_LF => CreateDataSet();
38+
39+
public static DataSet ValidDeployTimeUsages_LF => CreateDataSet();
40+
3641
public static DataSet InvalidTargetScopes_LF => CreateDataSet();
3742

3843
public static DataSet InvalidTypeDeclarations_LF => CreateDataSet();

‎src/Bicep.Core.Samples/Files/InvalidResources_CRLF/Completions/arrayPlusSymbols.json‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5209,23 +5209,6 @@
52095209
":"
52105210
]
52115211
},
5212-
{
5213-
"label": "runtimeInvalidRes9",
5214-
"kind": "interface",
5215-
"detail": "runtimeInvalidRes9",
5216-
"deprecated": false,
5217-
"preselect": false,
5218-
"sortText": "2_runtimeInvalidRes9",
5219-
"insertTextFormat": "plainText",
5220-
"insertTextMode": "adjustIndentation",
5221-
"textEdit": {
5222-
"range": {},
5223-
"newText": "runtimeInvalidRes9"
5224-
},
5225-
"commitCharacters": [
5226-
":"
5227-
]
5228-
},
52295212
{
52305213
"label": "runtimeValid",
52315214
"kind": "variable",
@@ -5257,6 +5240,23 @@
52575240
":"
52585241
]
52595242
},
5243+
{
5244+
"label": "runtimeValidRes10",
5245+
"kind": "interface",
5246+
"detail": "runtimeValidRes10",
5247+
"deprecated": false,
5248+
"preselect": false,
5249+
"sortText": "2_runtimeValidRes10",
5250+
"insertTextFormat": "plainText",
5251+
"insertTextMode": "adjustIndentation",
5252+
"textEdit": {
5253+
"range": {},
5254+
"newText": "runtimeValidRes10"
5255+
},
5256+
"commitCharacters": [
5257+
":"
5258+
]
5259+
},
52605260
{
52615261
"label": "runtimeValidRes2",
52625262
"kind": "interface",

‎src/Bicep.Core.Samples/Files/InvalidResources_CRLF/Completions/boolPropertyValuesPlusSymbols.json‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5173,23 +5173,6 @@
51735173
":"
51745174
]
51755175
},
5176-
{
5177-
"label": "runtimeInvalidRes9",
5178-
"kind": "interface",
5179-
"detail": "runtimeInvalidRes9",
5180-
"deprecated": false,
5181-
"preselect": false,
5182-
"sortText": "2_runtimeInvalidRes9",
5183-
"insertTextFormat": "plainText",
5184-
"insertTextMode": "adjustIndentation",
5185-
"textEdit": {
5186-
"range": {},
5187-
"newText": "runtimeInvalidRes9"
5188-
},
5189-
"commitCharacters": [
5190-
":"
5191-
]
5192-
},
51935176
{
51945177
"label": "runtimeValid",
51955178
"kind": "variable",
@@ -5221,6 +5204,23 @@
52215204
":"
52225205
]
52235206
},
5207+
{
5208+
"label": "runtimeValidRes10",
5209+
"kind": "interface",
5210+
"detail": "runtimeValidRes10",
5211+
"deprecated": false,
5212+
"preselect": false,
5213+
"sortText": "2_runtimeValidRes10",
5214+
"insertTextFormat": "plainText",
5215+
"insertTextMode": "adjustIndentation",
5216+
"textEdit": {
5217+
"range": {},
5218+
"newText": "runtimeValidRes10"
5219+
},
5220+
"commitCharacters": [
5221+
":"
5222+
]
5223+
},
52245224
{
52255225
"label": "runtimeValidRes2",
52265226
"kind": "interface",

‎src/Bicep.Core.Samples/Files/InvalidResources_CRLF/Completions/cleanupPreferencesPlusSymbols.json‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5201,23 +5201,6 @@
52015201
":"
52025202
]
52035203
},
5204-
{
5205-
"label": "runtimeInvalidRes9",
5206-
"kind": "interface",
5207-
"detail": "runtimeInvalidRes9",
5208-
"deprecated": false,
5209-
"preselect": false,
5210-
"sortText": "2_runtimeInvalidRes9",
5211-
"insertTextFormat": "plainText",
5212-
"insertTextMode": "adjustIndentation",
5213-
"textEdit": {
5214-
"range": {},
5215-
"newText": "runtimeInvalidRes9"
5216-
},
5217-
"commitCharacters": [
5218-
":"
5219-
]
5220-
},
52215204
{
52225205
"label": "runtimeValid",
52235206
"kind": "variable",
@@ -5249,6 +5232,23 @@
52495232
":"
52505233
]
52515234
},
5235+
{
5236+
"label": "runtimeValidRes10",
5237+
"kind": "interface",
5238+
"detail": "runtimeValidRes10",
5239+
"deprecated": false,
5240+
"preselect": false,
5241+
"sortText": "2_runtimeValidRes10",
5242+
"insertTextFormat": "plainText",
5243+
"insertTextMode": "adjustIndentation",
5244+
"textEdit": {
5245+
"range": {},
5246+
"newText": "runtimeValidRes10"
5247+
},
5248+
"commitCharacters": [
5249+
":"
5250+
]
5251+
},
52525252
{
52535253
"label": "runtimeValidRes2",
52545254
"kind": "interface",

‎src/Bicep.Core.Samples/Files/InvalidResources_CRLF/Completions/cliPropertyAccessIndexesPlusSymbols.json‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5432,23 +5432,6 @@
54325432
":"
54335433
]
54345434
},
5435-
{
5436-
"label": "runtimeInvalidRes9",
5437-
"kind": "interface",
5438-
"detail": "runtimeInvalidRes9",
5439-
"deprecated": false,
5440-
"preselect": false,
5441-
"sortText": "2_runtimeInvalidRes9",
5442-
"insertTextFormat": "plainText",
5443-
"insertTextMode": "adjustIndentation",
5444-
"textEdit": {
5445-
"range": {},
5446-
"newText": "runtimeInvalidRes9"
5447-
},
5448-
"commitCharacters": [
5449-
":"
5450-
]
5451-
},
54525435
{
54535436
"label": "runtimeValid",
54545437
"kind": "variable",
@@ -5480,6 +5463,23 @@
54805463
":"
54815464
]
54825465
},
5466+
{
5467+
"label": "runtimeValidRes10",
5468+
"kind": "interface",
5469+
"detail": "runtimeValidRes10",
5470+
"deprecated": false,
5471+
"preselect": false,
5472+
"sortText": "2_runtimeValidRes10",
5473+
"insertTextFormat": "plainText",
5474+
"insertTextMode": "adjustIndentation",
5475+
"textEdit": {
5476+
"range": {},
5477+
"newText": "runtimeValidRes10"
5478+
},
5479+
"commitCharacters": [
5480+
":"
5481+
]
5482+
},
54835483
{
54845484
"label": "runtimeValidRes2",
54855485
"kind": "interface",

0 commit comments

Comments
 (0)