Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unhandled exception and stack overflow for resource parent property assignments #4384

Merged
merged 1 commit into from
Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ public void Parent_property_self_cycles_are_blocked()
{
var (template, diags, _) = CompilationHelper.Compile(@"
resource vmExt 'Microsoft.Compute/virtualMachines/extensions@2020-06-01' = {
name: 'vmExt'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test failed to catch the stack overflow when the name property is present because of this if condition in ResourceMetadataCache: https://github.com/Azure/bicep/pull/4384/files#diff-150614dce0ae8e84ce370301fb8495c5226eaceac78046576ed656b0e41d498aR47

parent: vmExt
location: 'eastus'
}
Expand Down
35 changes: 33 additions & 2 deletions src/Bicep.Core.IntegrationTests/ScenarioTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2110,7 +2110,7 @@ param zonesEnabled bool
("BCP036", DiagnosticLevel.Error, "The property \"name\" expected a value of type \"string\" but the provided value is of type \"null\"."),
("BCP036", DiagnosticLevel.Error, "The property \"scope\" expected a value of type \"resource | tenant\" but the provided value is of type \"null\"."),
("BCP036", DiagnosticLevel.Error, "The property \"name\" expected a value of type \"string\" but the provided value is of type \"null\"."),
("BCP036", DiagnosticLevel.Error, "The property \"parent\" expected a value of type \"resource\" but the provided value is of type \"null\"."),
("BCP036", DiagnosticLevel.Error, "The property \"parent\" expected a value of type \"Microsoft.Network/dnsZones\" but the provided value is of type \"null\"."),
});
}

Expand Down Expand Up @@ -2480,5 +2480,36 @@ public void Test_Issue4156()
["accessKey2"] = "[listKeys(resourceId('Microsoft.EventGrid/topics', format('{0}-ZZZ', variables('topics')[copyIndex()])), '2021-06-01-preview').key1]"
});
}

[TestMethod]
// https://github.com/Azure/bicep/issues/4212
public void Test_Issue4212()
{
var result = CompilationHelper.Compile(
("main.bicep", @"
module mod 'mod.bicep' = {
name: 'mod'
}

resource res 'Microsoft.Network/virtualNetworks/subnets@2020-11-01' existing = {
name: 'abc/def'
parent: mod
}

resource res2 'Microsoft.Network/virtualNetworks/subnets@2020-11-01' existing = {
name: 'res2'
parent: tenant()
}

output test string = res.id
"),
("mod.bicep", ""));

result.Should().HaveDiagnostics(new[]
{
("BCP036", DiagnosticLevel.Error, "The property \"parent\" expected a value of type \"Microsoft.Network/virtualNetworks\" but the provided value is of type \"module\"."),
("BCP036", DiagnosticLevel.Error, "The property \"parent\" expected a value of type \"Microsoft.Network/virtualNetworks\" but the provided value is of type \"tenant\"."),
});
}
}
}
}
10 changes: 9 additions & 1 deletion src/Bicep.Core/Semantics/Metadata/ResourceMetadataCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Immutable;
using System.Runtime.CompilerServices;
using Bicep.Core.Syntax;

namespace Bicep.Core.Semantics.Metadata
Expand All @@ -23,13 +24,15 @@ public ResourceMetadataCache(SemanticModel semanticModel)

protected override ResourceMetadata? Calculate(SyntaxBase syntax)
{
RuntimeHelpers.EnsureSufficientExecutionStack();

switch (syntax)
{
case ResourceAccessSyntax _:
case VariableAccessSyntax _:
{
var symbol = semanticModel.GetSymbolInfo(syntax);
if (symbol is DeclaredSymbol declaredSymbol)
if (symbol is DeclaredSymbol declaredSymbol && semanticModel.Binder.TryGetCycle(declaredSymbol) is null)
{
return this.TryLookup(declaredSymbol.DeclaringSyntax);
}
Expand All @@ -46,6 +49,11 @@ public ResourceMetadataCache(SemanticModel semanticModel)
break;
}

if (semanticModel.Binder.TryGetCycle(symbol) is not null)
{
break;
}

if (semanticModel.Binder.GetNearestAncestor<ResourceDeclarationSyntax>(syntax) is {} nestedParentSyntax)
{
// nested resource parent syntax
Expand Down
2 changes: 1 addition & 1 deletion src/Bicep.Core/TypeSystem/Az/AzResourceTypeProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ static TypeProperty UpdateFlags(TypeProperty typeProperty, TypePropertyFlags fla
// add the 'parent' property for child resource types that are not nested inside a parent resource
if (!typeReference.IsRootType && !flags.HasFlag(ResourceTypeGenerationFlags.NestedResource))
{
var parentType = LanguageConstants.CreateResourceScopeReference(ResourceScope.Resource);
var parentType = new ResourceParentType(typeReference);
var parentFlags = TypePropertyFlags.WriteOnly | TypePropertyFlags.DeployTimeConstant | TypePropertyFlags.DisallowAny | TypePropertyFlags.LoopVariant;

properties = properties.SetItem(LanguageConstants.ResourceParentPropertyName, new TypeProperty(LanguageConstants.ResourceParentPropertyName, parentType, parentFlags));
Expand Down
25 changes: 25 additions & 0 deletions src/Bicep.Core/TypeSystem/ResourceParentType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Linq;
using Bicep.Core.Extensions;
using Bicep.Core.Resources;

namespace Bicep.Core.TypeSystem
{
public class ResourceParentType : TypeSymbol
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anthony-c-martin I ended up adding a new type for the parent property and doing validation in TypeValidator. I tried to modify ResourceMetadataCache to let it emit diagnostics, but I didn't find a good way to return them to the caller other than adding a IDiagnostic property to ResourceMetadata (which feels a bit hacky since it's only added for an edge case).

{
public ResourceParentType(ResourceTypeReference childTypeReference)
: base(GetFullyQualifiedParentTypeName(childTypeReference))
{
this.ChildTypeReference = childTypeReference;
}

public ResourceTypeReference ChildTypeReference { get; }

public override TypeKind TypeKind => TypeKind.Resource;

private static string GetFullyQualifiedParentTypeName(ResourceTypeReference childTypeReference) =>
$"{childTypeReference.Namespace}/{childTypeReference.Types.Take(childTypeReference.Types.Length - 1).ConcatString("/")}";
}
}
38 changes: 21 additions & 17 deletions src/Bicep.Core/TypeSystem/TypeValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,72 +86,76 @@ public static bool AreTypesAssignable(TypeSymbol sourceType, TypeSymbol targetTy
return true;
}

switch (targetType)
switch (sourceType, targetType)
{
case AnyType _:
case (_, AnyType):
// values of all types can be assigned to the "any" type
return true;

case IScopeReference targetScope:
case (IScopeReference, IScopeReference):
// checking for valid combinations of scopes happens after type checking. this allows us to provide a richer & more intuitive error message.
return sourceType is IScopeReference;
return true;

case UnionType union when ReferenceEquals(union, LanguageConstants.ResourceOrResourceCollectionRefItem):
case (_, UnionType targetUnion) when ReferenceEquals(targetUnion, LanguageConstants.ResourceOrResourceCollectionRefItem):
return sourceType is IScopeReference || sourceType is ArrayType { Item: IScopeReference };

case TypeSymbol _ when sourceType is ResourceType sourceResourceType:
case (ResourceType sourceResourceType, ResourceParentType targetResourceParentType):
// Assigning a resource to a parent property.
return sourceResourceType.TypeReference.IsParentOf(targetResourceParentType.ChildTypeReference);

case (ResourceType sourceResourceType, _):
// When assigning a resource, we're really assigning the value of the resource body.
return AreTypesAssignable(sourceResourceType.Body.Type, targetType);

case TypeSymbol _ when sourceType is ModuleType sourceModuleType:
case (ModuleType sourceModuleType, _):
// When assigning a module, we're really assigning the value of the module body.
return AreTypesAssignable(sourceModuleType.Body.Type, targetType);

case StringLiteralType _ when sourceType is StringLiteralType:
case (StringLiteralType, StringLiteralType):
// The name *is* the escaped string value, so we must have an exact match.
return targetType.Name == sourceType.Name;

case StringLiteralType _ when sourceType is PrimitiveType:
case (PrimitiveType, StringLiteralType):
// We allow string to string literal assignment only in the case where the "AllowLooseStringAssignment" validation flag has been set.
// This is to allow parameters without 'allowed' values to be assigned to fields expecting enums.
// At some point we may want to consider flowing the enum type backwards to solve this more elegantly.
return sourceType.ValidationFlags.HasFlag(TypeSymbolValidationFlags.AllowLooseStringAssignment) && sourceType.Name == LanguageConstants.String.Name;

case PrimitiveType _ when sourceType is StringLiteralType:
case (StringLiteralType, PrimitiveType):
// string literals can be assigned to strings
return targetType.Name == LanguageConstants.String.Name;

case PrimitiveType _ when sourceType is PrimitiveType:
case (PrimitiveType, PrimitiveType):
// both types are primitive
// compare by type name
return string.Equals(sourceType.Name, targetType.Name, StringComparison.Ordinal);

case ObjectType _ when sourceType is ObjectType:
case (ObjectType, ObjectType):
// both types are objects
// this function does not implement any schema validation, so this is far as we go
return true;

case ArrayType _ when sourceType is ArrayType:
case (ArrayType, ArrayType):
// both types are arrays
// this function does not validate item types
return true;

case DiscriminatedObjectType _ when sourceType is DiscriminatedObjectType:
case (DiscriminatedObjectType, DiscriminatedObjectType):
// validation left for later
return true;

case DiscriminatedObjectType _ when sourceType is ObjectType:
case (ObjectType, DiscriminatedObjectType):
// validation left for later
return true;

case TypeSymbol _ when sourceType is UnionType sourceUnion:
case (UnionType sourceUnion, _):
// union types are guaranteed to be flat

// TODO: Replace with some sort of set intersection
// are all source type members assignable to the target type?
return sourceUnion.Members.All(sourceMember => AreTypesAssignable(sourceMember.Type, targetType) == true);

case UnionType targetUnion:
case (_, UnionType targetUnion):
// the source type should be a singleton type
Debug.Assert(!(sourceType is UnionType), "!(sourceType is UnionType)");

Expand Down