Skip to content

Commit

Permalink
Improve ternary type inference (#15399)
Browse files Browse the repository at this point in the history
Resolves #15397

Worked on this as an alternative way to address #15114 (instead of
#15398). This PR does two things:

1. If a ternary has a literally-typed condition (i.e., it is definitely
true or definitely false), the type engine will use the type of the
active branch's expression. E.g., the type of `true ? 'a' : 'b'` is
`'a'`, and the type of `false ? 'a' : 'b'` is `'b'`. There was a TODO
comment in TypeAssignmentVisitor suggesting this change.
2. If the types of both branches can be combined instead of represented
as a union, the type engine will do so. For example, the type of
`unknownCondition ? 'a' : 'b'` is `'a' | 'b'` (a union), but the type of
`unknownCondition ? [stringParam] : [intParam]` is `[int | string]`
(assuming the type of `stringParam` is `string` and the type of
`intParam` is `int`). This change relies on existing type collapsing
logic, so it will handle things like combining refinements on string
types and combining objects into tagged unions if possible.

One change I made to the TypeCollapser is to collapse objects that
*can't* be combined into a tagged union into an object whose properties
are a union of the possible property types of the inputs. This is
similar to how we collapse tuple types. Given a template like the
following:

```bicep
param a {
  foo: string
}

param b {
  bar: string
  *: int
}

param condition bool

var value = condition ? a : b
```

`value` would have a type of `{ bar: string, foo: int | string, *: int
}`, with all properties flagged as optional.
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/15399)
  • Loading branch information
jeskew authored Oct 27, 2024
1 parent 7cf73ad commit d759532
Show file tree
Hide file tree
Showing 22 changed files with 429 additions and 189 deletions.
37 changes: 29 additions & 8 deletions src/Bicep.Core.IntegrationTests/ScenarioTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1985,10 +1985,11 @@ param isProdLike bool
// https://github.com/Azure/bicep/issues/2248
public void Test_Issue2248_UnionTypeInArrayAccessBaseExpression_NegativeCase()
{
var result = CompilationHelper.Compile(@"
var foos = true ? true : []
var primaryFoo = foos[0]
");
var result = CompilationHelper.Compile("""
param condition bool
var foos = condition ? true : []
var primaryFoo = foos[0]
""");
result.ExcludingLinterDiagnostics().Should().HaveDiagnostics(new[]
{
("BCP076", DiagnosticLevel.Error, "Cannot index over expression of type \"<empty array> | true\". Arrays or objects are required.")
Expand All @@ -2009,7 +2010,7 @@ param which bool
var chosenOne = which ? input : default
var p = chosenOne.foo
var p = chosenOne.?foo
");
result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics();
}
Expand Down Expand Up @@ -3341,8 +3342,8 @@ public void Test_Issue_9736_property_access_works_with_object_union_types()
""");

result.ExcludingLinterDiagnostics().Should().HaveDiagnostics([
("BCP070", DiagnosticLevel.Error, """Argument of type "(object | object) => (1 | 2)" is not assignable to parameter of type "any => string"."""),
("BCP070", DiagnosticLevel.Error, """Argument of type "(object | object) => (2 | 3)" is not assignable to parameter of type "any => string"."""),
("BCP070", DiagnosticLevel.Error, """Argument of type "(object | object) => int" is not assignable to parameter of type "any => string"."""),
("BCP070", DiagnosticLevel.Error, """Argument of type "(object | object) => int" is not assignable to parameter of type "any => string"."""),
]);
}

Expand Down Expand Up @@ -6211,7 +6212,7 @@ public void Test_Issue14839()
");

result.ExcludingLinterDiagnostics().Should().HaveDiagnostics([
("BCP053", DiagnosticLevel.Error, """The type "object" does not contain property "foo". Available properties include "bar"."""),
("BCP053", DiagnosticLevel.Error, """The type "object | object" does not contain property "foo". Available properties include "bar"."""),
]);
}

Expand All @@ -6230,6 +6231,26 @@ param firstItem object
output foo string[] = [for item in items: item.foo]
");

result.ExcludingLinterDiagnostics().Should().HaveDiagnostics([
("BCP187", DiagnosticLevel.Warning, """The property "foo" does not exist in the resource or type definition, although it might still be valid. If this is a resource type definition inaccuracy, report it using https://aka.ms/bicep-type-issues."""),
]);
}

[TestMethod]
// https://github.com/azure/bicep/issues/14839
public void Test_Issue14839_3()
{
var result = CompilationHelper.Compile("""
param firstItem object
var items = [
firstItem
{ bar: 'def' }
]
output foo string[] = [for item in items: item.?foo]
""");

result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ var toObject7 = toObject(range(0, 10), i => '${i}', () => null)
var ternary = map([123], true ? i => '${i}' : i => 'hello!')
//@[32:33) Local i. Type: any. Declaration start char: 32, length: 1
//@[46:47) Local i. Type: any. Declaration start char: 46, length: 1
//@[04:11) Variable ternary. Type: any. Declaration start char: 0, length: 60
//@[04:11) Variable ternary. Type: string[]. Declaration start char: 0, length: 60

var outsideArgs = i => 123
//@[18:19) Local i. Type: any. Declaration start char: 18, length: 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ output o object = 'str'
output exp string = 2 + 3
//@[20:25) [BCP033 (Error)] Expected a value of type "string" but the provided value is of type "5". (bicep https://aka.ms/bicep/core-diagnostics#BCP033) |2 + 3|
output union string = true ? 's' : 1
//@[22:36) [BCP033 (Error)] Expected a value of type "string" but the provided value is of type "'s' | 1". (bicep https://aka.ms/bicep/core-diagnostics#BCP033) |true ? 's' : 1|
output bad int = true && !4
//@[25:27) [BCP044 (Error)] Cannot apply operator "!" to operand of type "4". (bicep https://aka.ms/bicep/core-diagnostics#BCP044) |!4|
output deeper bool = true ? -true : (14 && 's') + 10
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2241,7 +2241,7 @@ resource dataCollectionRuleRes2 'Microsoft.Insights/dataCollectionRules@2021-04-
properties: {
description: dataCollectionRule.description
destinations: empty([]) ? [for x in []: {}] : [for x in []: {}]
//@[018:067) [BCP036 (Warning)] The property "destinations" expected a value of type "DataCollectionRuleDestinations | null" but the provided value is of type "object[] | object[]". If this is a resource type definition inaccuracy, report it using https://aka.ms/bicep-type-issues. (bicep https://aka.ms/bicep/core-diagnostics#BCP036) |empty([]) ? [for x in []: {}] : [for x in []: {}]|
//@[018:067) [BCP036 (Warning)] The property "destinations" expected a value of type "DataCollectionRuleDestinations | null" but the provided value is of type "object[]". If this is a resource type definition inaccuracy, report it using https://aka.ms/bicep-type-issues. (bicep https://aka.ms/bicep/core-diagnostics#BCP036) |empty([]) ? [for x in []: {}] : [for x in []: {}]|
//@[031:034) [BCP138 (Error)] For-expressions are not supported in this context. For-expressions may be used as values of resource, module, variable, and output declarations, or values of resource and module properties. (bicep https://aka.ms/bicep/core-diagnostics#BCP138) |for|
//@[051:054) [BCP138 (Error)] For-expressions are not supported in this context. For-expressions may be used as values of resource, module, variable, and output declarations, or values of resource and module properties. (bicep https://aka.ms/bicep/core-diagnostics#BCP138) |for|
dataSources: dataCollectionRule.dataSources
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ var reduceStringConcatEven = reduce(['abc', 'def', 'ghi'], '', (cur, next, i) =>
//@[064:067) Local cur. Type: 'abc' | 'def' | 'ghi'. Declaration start char: 64, length: 3
//@[069:073) Local next. Type: 'abc' | 'def' | 'ghi'. Declaration start char: 69, length: 4
//@[075:076) Local i. Type: int. Declaration start char: 75, length: 1
//@[004:026) Variable reduceStringConcatEven. Type: 'abc' | 'def' | 'ghi' | string. Declaration start char: 0, length: 117
//@[004:026) Variable reduceStringConcatEven. Type: string. Declaration start char: 0, length: 117
var reduceFactorial = reduce(range(1, 5), 1, (cur, next) => cur * next)
//@[046:049) Local cur. Type: int. Declaration start char: 46, length: 3
//@[051:055) Local next. Type: int. Declaration start char: 51, length: 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,16 @@ var w42__ = concat('xxxxx', 'xxxxxxxxxxx')
//@[04:09) Variable w42__. Type: string. Declaration start char: 0, length: 42

var w38___ = true? 'xxxxx' : 'xxxxxx'
//@[04:10) Variable w38___. Type: 'xxxxx' | 'xxxxxx'. Declaration start char: 0, length: 37
//@[04:10) Variable w38___. Type: 'xxxxx'. Declaration start char: 0, length: 37
var w39___ = true
//@[04:10) Variable w39___. Type: 'xxxxxx'. Declaration start char: 0, length: 39
? 'xxxxxx' : 'xxxxxx' // suffix
var w40___ = true ?'xxxxxx' : 'xxxxxxx'
//@[04:10) Variable w40___. Type: 'xxxxxx' | 'xxxxxxx'. Declaration start char: 0, length: 39
//@[04:10) Variable w40___. Type: 'xxxxxx'. Declaration start char: 0, length: 39
var w41___ = true ? 'xxxxxxx' : 'xxxxxxx'
//@[04:10) Variable w41___. Type: 'xxxxxxx'. Declaration start char: 0, length: 49
var w42___ = true ? 'xxxxxxx':'xxxxxxxx'
//@[04:10) Variable w42___. Type: 'xxxxxxx' | 'xxxxxxxx'. Declaration start char: 0, length: 40
//@[04:10) Variable w42___. Type: 'xxxxxxx'. Declaration start char: 0, length: 40

////////////////////////////////////////////////////////////////////////////////
//////////////////////////// Baselines for width 80 ////////////////////////////
Expand Down Expand Up @@ -103,9 +103,9 @@ var w78___ = /* xxxxxxxxxxxxxxxxxxxxxxxxxxxx */ true
? 1234567890
: 1234567890
var w79___ = /* xxxxxxxxxxxxxxxxxxxxxxxxxxxxx */ true ? { foo: 1 } : [12345678]
//@[04:10) Variable w79___. Type: [12345678] | object. Declaration start char: 0, length: 79
//@[04:10) Variable w79___. Type: object. Declaration start char: 0, length: 79
var w80___ = true ? { foo: true, bar: false } : [123, 234, 456, { xyz: 'xxxx' }]
//@[04:10) Variable w80___. Type: [123, 234, 456, object] | object. Declaration start char: 0, length: 80
//@[04:10) Variable w80___. Type: object. Declaration start char: 0, length: 80
var w81___ = /* xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx */ true ? 1234567890 : 1234567890
//@[04:10) Variable w81___. Type: 1234567890. Declaration start char: 0, length: 81
var w82___ = /* xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx */ true ? 1234567890 : 1234567890
Expand Down Expand Up @@ -185,22 +185,22 @@ var forceBreak10 = [1, 2, intersection({ foo: true, bar: false }, {
foo: true})]

var forceBreak11 = true // comment
//@[04:16) Variable forceBreak11. Type: false | true. Declaration start char: 0, length: 57
//@[04:16) Variable forceBreak11. Type: true. Declaration start char: 0, length: 57
? true
: false
var forceBreak12 = true ? true // comment
//@[04:16) Variable forceBreak12. Type: false | true. Declaration start char: 0, length: 53
//@[04:16) Variable forceBreak12. Type: true. Declaration start char: 0, length: 53
: false
var forceBreak13 = true
//@[04:16) Variable forceBreak13. Type: false | true. Declaration start char: 0, length: 57
//@[04:16) Variable forceBreak13. Type: true. Declaration start char: 0, length: 57
? true // comment
: false
var forceBreak14 = true ? {
//@[04:16) Variable forceBreak14. Type: false | object. Declaration start char: 0, length: 49
//@[04:16) Variable forceBreak14. Type: object. Declaration start char: 0, length: 49
foo: 42
} : false
var forceBreak15 = true ? { foo: 0 } : {
//@[04:16) Variable forceBreak15. Type: object | object. Declaration start char: 0, length: 52
//@[04:16) Variable forceBreak15. Type: object. Declaration start char: 0, length: 52
bar: 1}

var forceBreak16 = union({ foo: 0 }, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ resource withExpressions 'Microsoft.Storage/storageAccounts@2017-10-01' = {
properties: {
supportsHttpsTrafficOnly: !false
accessTier: true ? 'Hot' : 'Cold'
//@[16:37) [BCP036 (Warning)] The property "accessTier" expected a value of type "'Cool' | 'Hot' | null" but the provided value is of type "'Cold' | 'Hot'". If this is a resource type definition inaccuracy, report it using https://aka.ms/bicep-type-issues. (bicep https://aka.ms/bicep/core-diagnostics#BCP036) |true ? 'Hot' : 'Cold'|
encryption: {
keySource: 'Microsoft.Storage'
services: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ var isTrue = sys.max(1, 2) == 3
var isFalse = !isTrue
//@[04:11) Variable isFalse. Type: true. Declaration start char: 0, length: 21
var someText = isTrue ? sys.concat('a', sys.concat('b', 'c')) : 'someText'
//@[04:12) Variable someText. Type: 'someText' | string. Declaration start char: 0, length: 74
//@[04:12) Variable someText. Type: 'someText'. Declaration start char: 0, length: 74

// Bicep functions that cannot be converted into ARM functions
var scopesWithoutArmRepresentation = {
Expand All @@ -320,7 +320,7 @@ var scopesWithArmRepresentation = {
var issue1332_propname = 'ptest'
//@[04:22) Variable issue1332_propname. Type: 'ptest'. Declaration start char: 0, length: 32
var issue1332 = true ? {
//@[04:13) Variable issue1332. Type: object | object. Declaration start char: 0, length: 86
//@[04:13) Variable issue1332. Type: object. Declaration start char: 0, length: 86
prop1: {
'${issue1332_propname}': {}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ param myInt = sys.int(myBool ? 123 : 456)
//@[6:11) ParameterAssignment myInt. Type: int. Declaration start char: 0, length: 41

param myArray = [
//@[6:13) ParameterAssignment myArray. Type: ['a' | 'b', false, 579, 333, 6, 5, true, false, false, true]. Declaration start char: 0, length: 123
//@[6:13) ParameterAssignment myArray. Type: ['a', false, 579, 333, 6, 5, true, false, false, true]. Declaration start char: 0, length: 123
(true ? 'a' : 'b')
!true
123 + 456
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ var applicationGroupReferencesArr = (('' == allApplicationGroupReferences)
var hostpoolRequiredProps = {
friendlyName: hostpoolFriendlyName
description: hostpoolDescription
hostpoolType: hostpoolType
hostPoolType: hostpoolType
personalDesktopAssignmentType: personalDesktopAssignmentType
maxSessionLimit: maxSessionLimit
loadBalancerType: loadBalancerType
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@
"hostpoolRequiredProps": {
"friendlyName": "[parameters('hostpoolFriendlyName')]",
"description": "[parameters('hostpoolDescription')]",
"hostpoolType": "[parameters('hostpoolType')]",
"hostPoolType": "[parameters('hostpoolType')]",
"personalDesktopAssignmentType": "[parameters('personalDesktopAssignmentType')]",
"maxSessionLimit": "[parameters('maxSessionLimit')]",
"loadBalancerType": "[parameters('loadBalancerType')]",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@
"hostpoolRequiredProps": {
"friendlyName": "[parameters('hostpoolFriendlyName')]",
"description": "[parameters('hostpoolDescription')]",
"hostpoolType": "[parameters('hostpoolType')]",
"hostPoolType": "[parameters('hostpoolType')]",
"personalDesktopAssignmentType": "[parameters('personalDesktopAssignmentType')]",
"maxSessionLimit": "[parameters('maxSessionLimit')]",
"loadBalancerType": "[parameters('loadBalancerType')]",
Expand Down
97 changes: 94 additions & 3 deletions src/Bicep.Core.UnitTests/TypeSystem/TypeHelperTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,19 @@ public void Tagged_union_should_not_be_formed_when_discriminator_is_not_required
null),
};

TypeHelper.TryCollapseTypes(prospectiveTaggedUnionMembers).Should().BeNull();
var collapsed = TypeHelper.TryCollapseTypes(prospectiveTaggedUnionMembers).Should().BeAssignableTo<ObjectType>().Subject;
collapsed.Properties.Should().HaveCount(3);
collapsed.Properties.ContainsKey("type").Should().BeTrue();
collapsed.Properties["type"].TypeReference.Type.Name.Should().Be("'a' | 'b'");
collapsed.Properties["type"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();

collapsed.Properties.ContainsKey("foo").Should().BeTrue();
collapsed.Properties["foo"].TypeReference.Type.Name.Should().Be("string");
collapsed.Properties["foo"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();

collapsed.Properties.ContainsKey("bar").Should().BeTrue();
collapsed.Properties["bar"].TypeReference.Type.Name.Should().Be("int");
collapsed.Properties["bar"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();
}

[TestMethod]
Expand All @@ -203,20 +215,63 @@ public void Tagged_union_should_not_be_formed_when_multiple_members_use_same_dis
default,
new TypeProperty[]
{
new("type", TypeFactory.CreateStringLiteralType("b"), default),
new("type", TypeFactory.CreateStringLiteralType("b"), TypePropertyFlags.Required),
new("bar", LanguageConstants.Int, TypePropertyFlags.Required),
},
null),
new("{type: 'a', baz: int}",
default,
new TypeProperty[]
{
new("type", TypeFactory.CreateStringLiteralType("a"), default),
new("type", TypeFactory.CreateStringLiteralType("a"), TypePropertyFlags.Required),
new("baz", LanguageConstants.Int, TypePropertyFlags.Required),
},
null),
};

var collapsed = TypeHelper.TryCollapseTypes(prospectiveTaggedUnionMembers).Should().BeAssignableTo<ObjectType>().Subject;
collapsed.Properties.Should().HaveCount(4);
collapsed.Properties.ContainsKey("type").Should().BeTrue();
collapsed.Properties["type"].TypeReference.Type.Name.Should().Be("'a' | 'b'");
collapsed.Properties["type"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeTrue();

collapsed.Properties.ContainsKey("foo").Should().BeTrue();
collapsed.Properties["foo"].TypeReference.Type.Name.Should().Be("string");
collapsed.Properties["foo"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();

collapsed.Properties.ContainsKey("bar").Should().BeTrue();
collapsed.Properties["bar"].TypeReference.Type.Name.Should().Be("int");
collapsed.Properties["bar"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();

collapsed.Properties.ContainsKey("baz").Should().BeTrue();
collapsed.Properties["baz"].TypeReference.Type.Name.Should().Be("int");
collapsed.Properties["baz"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();
}

[TestMethod]
public void Union_should_not_be_collapsed_when_some_members_are_not_objects()
{
var prospectiveTaggedUnionMembers = new TypeSymbol[]
{
new ObjectType("{type: 'a', foo: string}",
default,
new TypeProperty[]
{
new("type", TypeFactory.CreateStringLiteralType("a"), TypePropertyFlags.Required),
new("foo", LanguageConstants.String, TypePropertyFlags.Required),
},
null),
new ObjectType("{type: 'b', bar: int}",
default,
new TypeProperty[]
{
new("type", TypeFactory.CreateStringLiteralType("b"), TypePropertyFlags.Required),
new("bar", LanguageConstants.Int, TypePropertyFlags.Required),
},
null),
LanguageConstants.String,
};

TypeHelper.TryCollapseTypes(prospectiveTaggedUnionMembers).Should().BeNull();
}

Expand Down Expand Up @@ -312,4 +367,40 @@ public void Tagged_union_formation_should_prefer_alpha_sorted_discriminator_when
collapsed.Should().BeOfType<DiscriminatedObjectType>();
collapsed.As<DiscriminatedObjectType>().DiscriminatorKey.Should().Be("fizz");
}

[TestMethod]
public void Object_collapse_should_incorporate_additionalProperties_types()
{
var toCollapse = new ObjectType[]
{
new("{foo: string}",
default,
new TypeProperty[]
{
new("foo", LanguageConstants.String, TypePropertyFlags.Required),
},
null),
new("{bar: string, *: int}",
default,
new TypeProperty[]
{
new("bar", LanguageConstants.String, TypePropertyFlags.Required),
},
LanguageConstants.Int),
};

var collapsed = TypeHelper.TryCollapseTypes(toCollapse).Should().BeAssignableTo<ObjectType>().Subject;
collapsed.Properties.Should().HaveCount(2);
collapsed.Properties.ContainsKey("foo").Should().BeTrue();
collapsed.Properties["foo"].TypeReference.Type.Name.Should().Be("int | string");
collapsed.Properties["foo"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();

collapsed.Properties.ContainsKey("bar").Should().BeTrue();
collapsed.Properties["bar"].TypeReference.Type.Name.Should().Be("string");
collapsed.Properties["bar"].Flags.HasFlag(TypePropertyFlags.Required).Should().BeFalse();

collapsed.AdditionalPropertiesType.Should().NotBeNull();
collapsed.AdditionalPropertiesType!.Type.Name.Should().Be("int");
collapsed.AdditionalPropertiesFlags.HasFlag(TypePropertyFlags.FallbackProperty).Should().BeTrue();
}
}
Loading

0 comments on commit d759532

Please sign in to comment.