Skip to content

Commit

Permalink
Correct union assignability check when both sides of the assignment a…
Browse files Browse the repository at this point in the history 
…re unions (Azure#8899)

* Correct union assignability check when both sides of the assignment are unions

* Incorporate feedback
  • Loading branch information
@jeskew
jeskew authored Nov 4, 2022
1 parent 643fd59 commit addbdcd
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 16 deletions.
47 changes: 47 additions & 0 deletions src/Bicep.Core.IntegrationTests/ScenarioTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3972,6 +3972,53 @@ public void Test_Issue6863()
result.Template.Should().HaveValueAtPath("$.resources[2].dependsOn", new JArray("[resourceId('Microsoft.ServiceBus/namespaces/queues', variables('Names')[0], variables('Service_Bus_Queues')[copyIndex()])]"));
}

/// <summary>
/// https://github.com/Azure/bicep/issues/8890
/// </summary>
[TestMethod]
public void Test_Issue8890()
{
var result = CompilationHelper.Compile(@"
param location string = resourceGroup().location
@description('Optional. Enables system assigned managed identity on the resource.')
param systemAssignedIdentity bool = false
@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 identity = identityType != 'None' ? {
type: identityType
userAssignedIdentities: !empty(userAssignedIdentities) ? userAssignedIdentities : null
} : null
resource vm 'Microsoft.Compute/virtualMachines@2021-07-01' = {
name: 'name'
location: location
identity: identity
}
param usePython bool
resource functionApp 'Microsoft.Web/sites@2022-03-01' = {
name: 'fa'
location: location
kind: 'functionApp'
identity: identity
properties: {
siteConfig: {
pythonVersion: usePython ? '~3.10' : null
nodeVersion: !usePython ? '18' : null
}
}
}
");

result.Should().NotHaveAnyDiagnostics();
}

/// <summary>
/// https://github.com/Azure/bicep/issues/8884
/// </summary>
Expand Down
127 changes: 111 additions & 16 deletions src/Bicep.Core/TypeSystem/TypeValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,10 @@ public static TypeSymbol NarrowTypeAndCollectDiagnostics(ITypeManager typeManage
}

private TypeSymbol NarrowType(TypeValidatorConfig config, SyntaxBase expression, TypeSymbol targetType)
{
var expressionType = typeManager.GetTypeInfo(expression);
=> NarrowType(config, expression, typeManager.GetTypeInfo(expression), targetType);

private TypeSymbol NarrowType(TypeValidatorConfig config, SyntaxBase expression, TypeSymbol expressionType, TypeSymbol targetType)
{
if (config.DisallowAny && expressionType is AnyType)
{
// certain properties such as scope, parent, dependsOn do not allow values of "any" type
Expand Down Expand Up @@ -423,50 +424,144 @@ private TypeSymbol NarrowVariableAccessType(TypeValidatorConfig config, Variable
return targetType;
}


private TypeSymbol NarrowUnionType(TypeValidatorConfig config, SyntaxBase expression, TypeSymbol expressionType, UnionType targetType)
=> expressionType switch
{
UnionType expressionUnion => NarrowUnionTypeForUnionExpressionType(config, expression, expressionUnion, targetType),
_ => NarrowUnionTypeForSingleExpressionType(config, expression, expressionType, targetType),
};

private TypeSymbol NarrowUnionTypeForUnionExpressionType(TypeValidatorConfig config, SyntaxBase expression, UnionType expressionType, UnionType targetType)
{
List<UnionTypeMemberViabilityInfo> candidacyEvaluations = new();

foreach (var candidateType in targetType.Members)
foreach (var candidateExpressionType in expressionType.Members)
{
if (!AreTypesAssignable(expressionType, candidateType.Type))
if (!AreTypesAssignable(candidateExpressionType.Type, targetType))
{
candidacyEvaluations.Add(new(candidateType, null, ImmutableArray<IDiagnostic>.Empty));
candidacyEvaluations.Add(new(candidateExpressionType, null, ImmutableArray<IDiagnostic>.Empty));
continue;
}

var candidateDiagnostics = ToListDiagnosticWriter.Create();
var candidateCollector = new TypeValidator(typeManager, binder, candidateDiagnostics);
var narrowed = candidateCollector.NarrowType(config, expression, candidateType.Type);
var narrowed = candidateCollector.NarrowUnionType(config, expression, candidateExpressionType.Type, targetType);
candidacyEvaluations.Add(new(candidateExpressionType, narrowed, candidateDiagnostics.GetDiagnostics()));
}

// report all informational and warning diagnostics
diagnosticWriter.WriteMultiple(candidacyEvaluations.SelectMany(e => e.Diagnostics));

// if we have a perfect match, skip checking the rest of the union members
if (!candidateDiagnostics.GetDiagnostics().Any())
var errors = candidacyEvaluations.SelectMany(c =>
{
if (c.NarrowedType is null || c.Errors.Any())
{
return narrowed;
return c.Errors.Append(DiagnosticBuilder.ForPosition(expression).ArgumentTypeMismatch(c.UnionTypeMemberEvaluated.Type, targetType));
}

candidacyEvaluations.Add(new(candidateType, narrowed, candidateDiagnostics.GetDiagnostics()));
return c.Errors;
});

// If *any* variant of the expression type is not assignable, the expression type is not assignable
if (errors.Any())
{
return ErrorType.Create(errors);
}

return TypeHelper.CreateTypeUnion(candidacyEvaluations.Select(c => c.NarrowedType!));
}

private TypeSymbol NarrowUnionTypeForSingleExpressionType(TypeValidatorConfig config, SyntaxBase expression, TypeSymbol expressionType, UnionType targetType)
{
List<UnionTypeMemberViabilityInfo> candidacyEvaluations = new();

foreach (var candidateTargetType in targetType.Members)
{
if (!AreTypesAssignable(expressionType, candidateTargetType.Type))
{
candidacyEvaluations.Add(new(candidateTargetType, null, ImmutableArray<IDiagnostic>.Empty));
continue;
}

var candidateDiagnostics = ToListDiagnosticWriter.Create();
var candidateCollector = new TypeValidator(typeManager, binder, candidateDiagnostics);
var narrowed = candidateCollector.NarrowType(config, expression, expressionType, candidateTargetType.Type);
candidacyEvaluations.Add(new(candidateTargetType, narrowed, candidateDiagnostics.GetDiagnostics()));
}

var viableCandidates = candidacyEvaluations
.Select(c => c.Narrowed is {} Narrowed && !c.Diagnostics.OfType<ErrorDiagnostic>().Any() ? new ViableTypeCandidate(Narrowed, c.Diagnostics) : null)
.Select(c => c.NarrowedType is {} Narrowed && !c.Errors.Any() ? new ViableTypeCandidate(Narrowed, c.Diagnostics) : null)
.WhereNotNull()
.ToImmutableArray();

if (viableCandidates.Any())
{
// It's unclear what we should do with warning and informational diagnostics if there's no path that's assignable without issue.
// Should we report only diagnostics raised by every viable candidate? Or report only those raised by a single candidate?
// Erring on the side of caution here and just reporting them all.
diagnosticWriter.WriteMultiple(viableCandidates.SelectMany(c => c.Diagnostics));
// It's unclear what we should do with warning and informational diagnostics. Should we report only the intersection of diagnostics raised by every viable candidate?
// Erring on the side of caution here and just reporting them all unless there are one or more candidates that are assignable w/o warning.
if (viableCandidates.All(c => c.Diagnostics.Any()))
{
diagnosticWriter.WriteMultiple(viableCandidates.SelectMany(c => c.Diagnostics));
}

return TypeHelper.CreateTypeUnion(viableCandidates.Select(c => c.Type));
}

return ErrorType.Create(DiagnosticBuilder.ForPosition(expression).ArgumentTypeMismatch(expressionType, targetType));
}

private record UnionTypeMemberViabilityInfo(ITypeReference Type, TypeSymbol? Narrowed, IEnumerable<IDiagnostic> Diagnostics);
private record UnionTypeMemberViabilityInfo
{
internal UnionTypeMemberViabilityInfo(ITypeReference unionTypeMemberEvaluated, TypeSymbol? narrowedType, IEnumerable<IDiagnostic> diagnostics)
{
UnionTypeMemberEvaluated = unionTypeMemberEvaluated;
NarrowedType = narrowedType;

List<IDiagnostic> nonErrorDiagnostics = new();
List<ErrorDiagnostic> errorDiagnostics = new();

foreach (var diagnostic in diagnostics)
{
if (diagnostic.Level == DiagnosticLevel.Error)
{
errorDiagnostics.Add(AsErrorDiagnostic(diagnostic));
}
else
{
nonErrorDiagnostics.Add(diagnostic);
}
}

Diagnostics = nonErrorDiagnostics;
Errors = errorDiagnostics;
}

/// <summary>
/// The type being checked for assignability and narrowing
/// </summary>
public ITypeReference UnionTypeMemberEvaluated { get; }

/// <summary>
/// The narrowed type. Will be null if the type is unassignable
/// </summary>
public TypeSymbol? NarrowedType { get; }

/// <summary>
/// Any warning or informational diagnostics raised during type narrowing
/// </summary>
public IReadOnlyList<IDiagnostic> Diagnostics { get; }

/// <summary>
/// Any error-level diagnostics raised during type narrowing
/// </summary>
public IReadOnlyList<ErrorDiagnostic> Errors { get; }

private static ErrorDiagnostic AsErrorDiagnostic(IDiagnostic diagnostic) => diagnostic switch
{
ErrorDiagnostic errorDiagnostic => errorDiagnostic,
_ => new(diagnostic.Span, diagnostic.Code, diagnostic.Message, diagnostic.Uri, diagnostic.Styling),
};
}

private record ViableTypeCandidate(TypeSymbol Type, IEnumerable<IDiagnostic> Diagnostics);

Expand Down

0 comments on commit addbdcd

Please sign in to comment.