Skip to content

Commit 422b7f2

Browse files
authored
Prevent infinite recursion during type narrowing (#17028)
Resolves #16727 In practice, infinite recursion usually occurs when we are narrowing a value against a target where both have the same type. The scenario test and linked issue demonstrate this with resource-derived types, but this can also happen with a user-defined type. ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/17028)
1 parent 6757832 commit 422b7f2

File tree

4 files changed

+79
-24
lines changed

4 files changed

+79
-24
lines changed

src/Bicep.Core.IntegrationTests/ScenarioTests.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7211,4 +7211,23 @@ public void List_functions_should_use_symbolic_name()
72117211

72127212
result.ExcludingLinterDiagnostics().Should().NotHaveAnyDiagnostics();
72137213
}
7214+
7215+
[TestMethod]
7216+
public void Test_Issue16727()
7217+
{
7218+
var result = CompilationHelper.Compile("""
7219+
resource nsg 'Microsoft.Network/networkSecurityGroups@2024-05-01' = {
7220+
name: 'example'
7221+
location: 'location'
7222+
properties: {
7223+
securityRules: []
7224+
}
7225+
tags: {}
7226+
}
7227+
7228+
output properties resourceOutput<'Microsoft.Network/networkSecurityGroups@2024-05-01'>.properties = nsg.properties
7229+
""");
7230+
7231+
result.Should().NotHaveAnyDiagnostics();
7232+
}
72147233
}

src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1873,4 +1873,20 @@ public void Safe_FromEnd_indexing_of_tuple_resolves_correct_type()
18731873
("BCP033", DiagnosticLevel.Error, "Expected a value of type \"int | null\" but the provided value is of type \"null | string\"."),
18741874
});
18751875
}
1876+
1877+
[TestMethod]
1878+
public void Narrowing_a_recursive_type_against_itself_does_not_recur_infinitely()
1879+
{
1880+
var result = CompilationHelper.Compile("""
1881+
type recursiveType = {
1882+
recursion: recursiveType?
1883+
}
1884+
1885+
param p recursiveType
1886+
1887+
output o recursiveType = p
1888+
""");
1889+
1890+
result.Should().NotHaveAnyDiagnostics();
1891+
}
18761892
}

src/Bicep.Core/TypeSystem/DeclaredTypeManager.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1003,7 +1003,7 @@ private TypeSymbol FinalizeTypePropertyType(
10031003
return ErrorType.Create(DiagnosticBuilder.ForPosition(syntax).AccessExpressionForbiddenBase());
10041004
}
10051005

1006-
return EnsureNonParameterizedType(propertyNameSyntax, GetTypePropertyType(baseExpressionType, propertyName, propertyNameSyntax));
1006+
return EnsureNonParameterizedType(propertyNameSyntax, typePropertyType);
10071007
}
10081008

10091009
private static TypeSymbol GetTypePropertyType(ITypeReference baseExpressionType, string propertyName, SyntaxBase propertyNameSyntax)

src/Bicep.Core/TypeSystem/TypeValidator.cs

Lines changed: 43 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@ private record TypeValidatorConfig(
3232
bool DisallowAny,
3333
SyntaxBase? OriginSyntax,
3434
TypeMismatchDiagnosticWriter? OnTypeMismatch,
35-
bool IsResourceDeclaration);
35+
bool IsResourceDeclaration,
36+
HashSet<(SyntaxBase expression, TypeSymbol expressionType, TypeSymbol targetType)> currentlyProcessing);
3637

3738
private TypeValidator(ITypeManager typeManager, IBinder binder, IDiagnosticLookup parsingErrorLookup, IDiagnosticWriter diagnosticWriter)
3839
{
@@ -218,7 +219,8 @@ public static TypeSymbol NarrowTypeAndCollectDiagnostics(ITypeManager typeManage
218219
DisallowAny: false,
219220
OriginSyntax: null,
220221
OnTypeMismatch: null,
221-
IsResourceDeclaration: isResourceDeclaration);
222+
IsResourceDeclaration: isResourceDeclaration,
223+
currentlyProcessing: new());
222224

223225
var validator = new TypeValidator(typeManager, binder, parsingErrorLookup, diagnosticWriter);
224226

@@ -1139,7 +1141,7 @@ static TypeSymbol RemoveImplicitNull(TypeSymbol type, bool typeWasPreserved)
11391141
diagnosticWriter.Write(diagnosticTarget, x => x.CannotAssignToReadOnlyProperty(resourceTypeInaccuracy || ShouldWarnForPropertyMismatch(targetType), declaredProperty.Name, resourceTypeInaccuracy));
11401142
}
11411143

1142-
narrowedProperties.Add(new NamedTypeProperty(declaredProperty.Name, declaredProperty.TypeReference.Type, declaredProperty.Flags));
1144+
narrowedProperties.Add(declaredProperty);
11431145
continue;
11441146
}
11451147

@@ -1149,25 +1151,40 @@ static TypeSymbol RemoveImplicitNull(TypeSymbol type, bool typeWasPreserved)
11491151
x => x.FallbackPropertyUsed(shouldDowngrade: false, declaredProperty.Name));
11501152
}
11511153

1152-
var newConfig = new TypeValidatorConfig(
1153-
SkipConstantCheck: skipConstantCheckForProperty,
1154-
SkipTypeErrors: true,
1155-
DisallowAny: declaredProperty.Flags.HasFlag(TypePropertyFlags.DisallowAny),
1156-
OriginSyntax: config.OriginSyntax,
1157-
OnTypeMismatch: GetPropertyMismatchDiagnosticWriter(
1154+
var newConfig = config with
1155+
{
1156+
SkipConstantCheck = skipConstantCheckForProperty,
1157+
SkipTypeErrors = true,
1158+
DisallowAny = declaredProperty.Flags.HasFlag(TypePropertyFlags.DisallowAny),
1159+
OnTypeMismatch = GetPropertyMismatchDiagnosticWriter(
11581160
config: config,
11591161
shouldWarn: (config.IsResourceDeclaration && !declaredProperty.Flags.HasFlag(TypePropertyFlags.SystemProperty)) || ShouldWarn(declaredProperty.TypeReference.Type),
11601162
propertyName: declaredProperty.Name,
11611163
showTypeInaccuracyClause: config.IsResourceDeclaration && !declaredProperty.Flags.HasFlag(TypePropertyFlags.SystemProperty)),
1162-
IsResourceDeclaration: config.IsResourceDeclaration);
1164+
};
1165+
1166+
var propertyExpression = declaredPropertySyntax?.Value ?? expression;
1167+
TypeSymbol propertyTargetType = declaredProperty.TypeReference.Type;
1168+
TypeSymbol propertyExpressionType = expressionTypeProperty.TypeReference.Type;
1169+
1170+
TypeSymbol GetNarrowedPropertyType()
1171+
{
1172+
// append "| null" to the property type for non-required properties
1173+
var (propertyAssignmentType, typeWasPreserved) = AddImplicitNull(propertyTargetType, declaredProperty.Flags);
11631174

1164-
// append "| null" to the property type for non-required properties
1165-
var (propertyAssignmentType, typeWasPreserved) = AddImplicitNull(declaredProperty.TypeReference.Type, declaredProperty.Flags);
1175+
var narrowedType = NarrowType(newConfig, propertyExpression, propertyExpressionType, propertyAssignmentType);
1176+
return RemoveImplicitNull(narrowedType, typeWasPreserved);
1177+
}
11661178

1167-
var narrowedType = NarrowType(newConfig, declaredPropertySyntax?.Value ?? expression, expressionTypeProperty.TypeReference.Type, propertyAssignmentType);
1168-
narrowedType = RemoveImplicitNull(narrowedType, typeWasPreserved);
1179+
// In the case of a recursive type, eager narrowing can lead to infinite recursion. If we've
1180+
// already narrowed this (expressionSyntax, expressionType, targetType) triple, then all
1181+
// relevant diagnostics have already been raised. Use a deferred type reference to stop eagerly
1182+
// comparing and narrowing types from this point forward.
1183+
ITypeReference narrowedPropertyType = config.currentlyProcessing.Add((propertyExpression, propertyExpressionType, propertyTargetType))
1184+
? GetNarrowedPropertyType()
1185+
: new DeferredTypeReference(GetNarrowedPropertyType);
11691186

1170-
narrowedProperties.Add(new NamedTypeProperty(declaredProperty.Name, narrowedType, declaredProperty.Flags));
1187+
narrowedProperties.Add(new NamedTypeProperty(declaredProperty.Name, narrowedPropertyType, declaredProperty.Flags));
11711188
}
11721189
else
11731190
{
@@ -1236,19 +1253,22 @@ static TypeSymbol RemoveImplicitNull(TypeSymbol type, bool typeWasPreserved)
12361253
skipConstantCheckForProperty = true;
12371254
}
12381255

1239-
var newConfig = new TypeValidatorConfig(
1240-
SkipConstantCheck: skipConstantCheckForProperty,
1241-
SkipTypeErrors: true,
1242-
DisallowAny: targetType.AdditionalProperties.Flags.HasFlag(TypePropertyFlags.DisallowAny),
1243-
OriginSyntax: config.OriginSyntax,
1244-
OnTypeMismatch: GetPropertyMismatchDiagnosticWriter(config, ShouldWarn(targetType.AdditionalProperties.TypeReference.Type), extraProperty.Key, false),
1245-
IsResourceDeclaration: config.IsResourceDeclaration);
1256+
var newConfig = config with
1257+
{
1258+
SkipConstantCheck = skipConstantCheckForProperty,
1259+
SkipTypeErrors = true,
1260+
DisallowAny = targetType.AdditionalProperties.Flags.HasFlag(TypePropertyFlags.DisallowAny),
1261+
OnTypeMismatch = GetPropertyMismatchDiagnosticWriter(config, ShouldWarn(targetType.AdditionalProperties.TypeReference.Type), extraProperty.Key, false),
1262+
};
12461263

12471264
// append "| null" to the type on non-required properties
12481265
var (additionalPropertiesAssignmentType, _) = AddImplicitNull(targetType.AdditionalProperties.TypeReference.Type, targetType.AdditionalProperties.Flags);
12491266

12501267
// although we don't use the result here, it's important to call NarrowType to collect diagnostics
1251-
var narrowedType = NarrowType(newConfig, extraPropertySyntax?.Value ?? expression, extraProperty.Value.TypeReference.Type, additionalPropertiesAssignmentType);
1268+
if (config.currentlyProcessing.Add((extraPropertySyntax?.Value ?? expression, extraProperty.Value.TypeReference.Type, additionalPropertiesAssignmentType)))
1269+
{
1270+
var narrowedType = NarrowType(newConfig, extraPropertySyntax?.Value ?? expression, extraProperty.Value.TypeReference.Type, additionalPropertiesAssignmentType);
1271+
}
12521272

12531273
// TODO should we try and narrow the additional properties type? May be difficult
12541274
}

0 commit comments

Comments
 (0)