Skip to content

Commit

Permalink
Narrow object types when assigned value is not an object literal or v…
Browse files Browse the repository at this point in the history
…ariable reference (Azure#12798)

Resolves Azure#12590 

The TypeValidator's object type narrowing logic currently only runs when
the assigned value is or can be traced directly back to an object
literal (i.e., an `ObjectSyntax` node). This PR updates the narrowing
logic for both object and discriminated objects to be type- rather than
syntax-driven, which allows narrowing to be performed on the return
value of expressions, property/array accesses, and references to
resources, module outputs, and imported variables.

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/12798)
  • Loading branch information
jeskew committed Jan 4, 2024
1 parent df7406b commit e77a133
Show file tree
Hide file tree
Showing 9 changed files with 328 additions and 186 deletions.
43 changes: 43 additions & 0 deletions src/Bicep.Core.IntegrationTests/CompileTimeImportTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1935,6 +1935,49 @@ func getSubnetNumber(plan_name string) string => isFirstPlan(getPlanNumber(plan_
evaluated.Should().HaveValueAtPath("outputs.out.value", "ASP999");
}

[TestMethod]
public void Imported_objects_can_be_used_in_object_type_narrowing()
{
var result = CompilationHelper.Compile(ServicesWithCompileTimeTypeImports,
("main.bicep", """
import {obj} from 'mod.bicep'

@sealed()
output out {} = obj
"""),
("mod.bicep", """
@export()
var obj = {foo: 'foo', bar: 'bar'}
"""));

result.Should().HaveDiagnostics(new[]
{
("BCP037", DiagnosticLevel.Error, """The property "bar" is not allowed on objects of type "{ }". No other properties are allowed."""),
("BCP037", DiagnosticLevel.Error, """The property "foo" is not allowed on objects of type "{ }". No other properties are allowed."""),
});
}

[TestMethod]
public void Imported_objects_can_be_used_in_discriminated_object_type_narrowing()
{
var result = CompilationHelper.Compile(ServicesWithCompileTimeTypeImports,
("main.bicep", """
import {obj} from 'mod.bicep'

@discriminator('type')
output out {type: 'foo', pop: bool} | {type: 'bar', quux: string} = obj
"""),
("mod.bicep", """
@export()
var obj = {type: 'foo', bar: 'bar'}
"""));

result.Should().HaveDiagnostics(new[]
{
("BCP035", DiagnosticLevel.Error, """The specified "output" declaration is missing the following required properties: "pop"."""),
});
}

// https://github.com/Azure/bicep/issues/12897
[TestMethod]
public void LanguageVersion_2_should_be_used_if_types_imported_via_wildcard()
Expand Down
65 changes: 61 additions & 4 deletions src/Bicep.Core.IntegrationTests/ScenarioTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1518,7 +1518,7 @@ public void Test_Issue2535()
}
");

result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics();
result.ExcludingLinterDiagnostics().Should().NotHaveAnyCompilationBlockingDiagnostics();
result.Template.Should().HaveValueAtPath("$.resources[0].properties.details.parent", "[managementGroup()]");

var evaluated = TemplateEvaluator.Evaluate(result.Template);
Expand Down Expand Up @@ -3992,7 +3992,7 @@ public void Test_Issue8890()
@description('Optional. The ID(s) to assign to the resource.')
param userAssignedIdentities object = {}
var identityType = systemAssignedIdentity ? (!empty(userAssignedIdentities) ? 'SystemAssigned,UserAssigned' : 'SystemAssigned') : (!empty(userAssignedIdentities) ? 'UserAssigned' : 'None')
var identityType = systemAssignedIdentity ? (!empty(userAssignedIdentities) ? 'SystemAssigned, UserAssigned' : 'SystemAssigned') : (!empty(userAssignedIdentities) ? 'UserAssigned' : 'None')
var identity = identityType != 'None' ? {
type: identityType
Expand Down Expand Up @@ -5378,11 +5378,11 @@ public void Test_Issue12640()
param rgs = {
'0': {
name: 'rg-test-1'
location:'westeurope'
location:'westeurope'
}
'1': {
name: 'rg-test-2'
location:'westeurope'
location:'westeurope'
}
}

Expand Down Expand Up @@ -5460,4 +5460,61 @@ public void Test_Issue12799()
result.Parameters.Should().HaveValueAtPath("parameters.one.value", true);
result.Parameters.Should().HaveValueAtPath("parameters.two.value", true);
}
// https://github.com/Azure/bicep/issues/12590
[TestMethod]
public void Test_Issue12590()
{
var result = CompilationHelper.Compile(
("main.bicep", """
var resources = {
st: {
type: 'storage'
name: 'st'
containers: []
}
kv: {
type: 'keyvault'
name: 'kv'
secrets: []
extraProperty: 'extra'
}
}

module deploy_resource_module 'resourceModule.bicep' = {
name: 'resourceModule'
params: {
subResource: resources.kv
}
}
"""),
("resourceModule.bicep", """
@sealed()
type storageAccount = {
type: 'storage'
name: string
containers: string[]
}

@sealed()
type keyVault = {
type: 'keyvault'
name: string
secrets: string[]
}

type Resource = {
@discriminator('type')
*: storageAccount | keyVault
}

param resource Resource?

param subResource keyVault?
"""));

result.Should().HaveDiagnostics(new[]
{
("BCP037", DiagnosticLevel.Error, """The property "extraProperty" is not allowed on objects of type "{ type: 'keyvault', name: string, secrets: string[] }". No other properties are allowed."""),
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ resource nicLoop 'Microsoft.Network/networkInterfaces@2020-11-01' = [for i in ra
ipConfigurations: [
// TODO: fix this
ipConfigurations[i]
//@[06:25) [BCP036 (Warning)] The property "id" expected a value of type "null | string" but the provided value is of type "true". If this is an inaccuracy in the documentation, please report it to the Bicep Team. (CodeDescription: bicep(https://aka.ms/bicep-type-issues)) |ipConfigurations[i]|
//@[06:25) [BCP037 (Warning)] The property "madeUpProperty" is not allowed on objects of type "NetworkInterfaceIPConfigurationPropertiesFormat". Permissible properties include "applicationGatewayBackendAddressPools", "applicationSecurityGroups", "loadBalancerBackendAddressPools", "loadBalancerInboundNatRules", "primary", "privateIPAddress", "privateIPAddressVersion", "privateIPAllocationMethod", "publicIPAddress", "virtualNetworkTaps". If this is an inaccuracy in the documentation, please report it to the Bicep Team. (CodeDescription: bicep(https://aka.ms/bicep-type-issues)) |ipConfigurations[i]|
//@[06:25) [BCP036 (Warning)] The property "subnet" expected a value of type "Subnet | null" but the provided value is of type "'hello'". If this is an inaccuracy in the documentation, please report it to the Bicep Team. (CodeDescription: bicep(https://aka.ms/bicep-type-issues)) |ipConfigurations[i]|
]
}
}]
Expand All @@ -57,6 +60,9 @@ resource nicLoop2 'Microsoft.Network/networkInterfaces@2020-11-01' = [for ipConf
ipConfigurations: [
// TODO: fix this
ipConfig
//@[06:14) [BCP036 (Warning)] The property "id" expected a value of type "null | string" but the provided value in source declaration "ipConfig" is of type "true". If this is an inaccuracy in the documentation, please report it to the Bicep Team. (CodeDescription: bicep(https://aka.ms/bicep-type-issues)) |ipConfig|
//@[06:14) [BCP037 (Warning)] The property "madeUpProperty" from source declaration "ipConfig" is not allowed on objects of type "NetworkInterfaceIPConfigurationPropertiesFormat". Permissible properties include "applicationGatewayBackendAddressPools", "applicationSecurityGroups", "loadBalancerBackendAddressPools", "loadBalancerInboundNatRules", "primary", "privateIPAddress", "privateIPAddressVersion", "privateIPAllocationMethod", "publicIPAddress", "virtualNetworkTaps". If this is an inaccuracy in the documentation, please report it to the Bicep Team. (CodeDescription: bicep(https://aka.ms/bicep-type-issues)) |ipConfig|
//@[06:14) [BCP036 (Warning)] The property "subnet" expected a value of type "Subnet | null" but the provided value in source declaration "ipConfig" is of type "'hello'". If this is an inaccuracy in the documentation, please report it to the Bicep Team. (CodeDescription: bicep(https://aka.ms/bicep-type-issues)) |ipConfig|
]
}
}]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ var networkSecurityGroup = {
sourcePortRange: '*'
sourceAddressPrefix: '*'
destinationAddressPrefix: '*'
destinationPortRange: 3389
destinationPortRange: '3389'
}
}
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"_generator": {
"name": "bicep",
"version": "dev",
"templateHash": "10400776470073514583"
"templateHash": "14830623468051456397"
}
},
"parameters": {
Expand Down Expand Up @@ -102,7 +102,7 @@
"sourcePortRange": "*",
"sourceAddressPrefix": "*",
"destinationAddressPrefix": "*",
"destinationPortRange": 3389
"destinationPortRange": "3389"
}
}
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"_generator": {
"name": "bicep",
"version": "dev",
"templateHash": "1986833384670339728"
"templateHash": "3396093801917295828"
}
},
"parameters": {
Expand Down Expand Up @@ -103,7 +103,7 @@
"sourcePortRange": "*",
"sourceAddressPrefix": "*",
"destinationAddressPrefix": "*",
"destinationPortRange": 3389
"destinationPortRange": "3389"
}
}
]
Expand Down
13 changes: 6 additions & 7 deletions src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -236,18 +236,17 @@ private static string ToQuotedStringWithCaseInsensitiveOrdering(IEnumerable<stri
"BCP034",
$"The enclosing array expected an item of type \"{expectedType}\", but the provided item was of type \"{actualType}\".");

public Diagnostic MissingRequiredProperties(bool warnInsteadOfError, Symbol? sourceDeclaration, ObjectSyntax objectSyntax, ICollection<string> properties, string blockName, bool showTypeInaccuracy, IDiagnosticLookup parsingErrorLookup)
public Diagnostic MissingRequiredProperties(bool warnInsteadOfError, Symbol? sourceDeclaration, ObjectSyntax? objectSyntax, ICollection<string> properties, string blockName, bool showTypeInaccuracy, IDiagnosticLookup parsingErrorLookup)
{
var sourceDeclarationClause = sourceDeclaration is not null
? $" from source declaration \"{sourceDeclaration.Name}\""
: string.Empty;

var newSyntax = SyntaxModifier.TryAddProperties(
objectSyntax,
properties.Select(p => SyntaxFactory.CreateObjectProperty(p, SyntaxFactory.EmptySkippedTrivia)),
parsingErrorLookup);

if (newSyntax is null)
if (objectSyntax is null ||
SyntaxModifier.TryAddProperties(
objectSyntax,
properties.Select(p => SyntaxFactory.CreateObjectProperty(p, SyntaxFactory.EmptySkippedTrivia)),
parsingErrorLookup) is not { } newSyntax)
{
// We're unable to come up with an automatic code fix - most likely because there are unhandled parse errors
return new Diagnostic(
Expand Down
5 changes: 5 additions & 0 deletions src/Bicep.Core/TypeSystem/TypeCollapser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,11 @@ internal ObjectCollapse(bool nullable)
{
var (members, viableDiscriminators) = discriminatedObjectTypeBuilder.Build();

if (members.Count == 1)
{
return nullable ? TypeHelper.CreateTypeUnion(members.Single(), LanguageConstants.Null) : members.Single();
}

var discriminator = viableDiscriminators
.OrderBy(possibleDiscriminator =>
{
Expand Down
Loading

0 comments on commit e77a133

Please sign in to comment.