From 148466a1a9e86035b02de88d94a64ed6169859f3 Mon Sep 17 00:00:00 2001 From: Jonny Eskew Date: Wed, 13 Mar 2024 13:28:01 -0400 Subject: [PATCH] Preserve distinction between `{}` and `object` in compiled JSON (#13597) Resolves #13556 Given an object type declaration with no catchall schema, the type checker will issue warnings if a property whose name does not match any property declarations is supplied. ```bicep param example { foo: string } = { bar: 'string' } // <-- Issues a BCP037 warning because there is no known `bar` property @sealed() param sealedExample { foo: string } = { bar: 'string' } // <-- Issues a BCP037 error because there is no known `bar` property and the object is sealed ``` This means that given an object type declaration of `{}`, all supplied properties will trigger a BCP037 warning because an empty object was expected. This behaves differently from the untyped object `object`, which matches without warning against any object. The distinction between `{}` and `object` was getting dropped when templates were compiled to JSON, however, because the compiler wasn't including any information in the type definition that would allow the two to be distinguished; instead, they would both be compiled to `{"type": "object"}`. This PR updates the compiler to include an empty `properties` constraint on object type declarations that include no properties so that it can tell the difference between `{}` and `object` when loading or importing from JSON modules. ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/13597) --- .../ScenarioTests.cs | 35 +++++++++++++++ .../UserDefinedTypeTests.cs | 12 +++++ .../baselines/TypeDeclarations_LF/main.json | 5 ++- .../TypeDeclarations_LF/main.sourcemap.bicep | 3 ++ .../main.symbolicnames.json | 5 ++- .../ArmTemplateSemanticModelTests.cs | 24 ++++++++++ src/Bicep.Core/Emit/TemplateWriter.cs | 5 +-- .../TypeSystem/ArmTemplateTypeLoader.cs | 44 ++++++++++--------- 8 files changed, 107 insertions(+), 26 deletions(-) diff --git a/src/Bicep.Core.IntegrationTests/ScenarioTests.cs b/src/Bicep.Core.IntegrationTests/ScenarioTests.cs index a4499325869..2c75eb37675 100644 --- a/src/Bicep.Core.IntegrationTests/ScenarioTests.cs +++ b/src/Bicep.Core.IntegrationTests/ScenarioTests.cs @@ -5898,4 +5898,39 @@ public void Test_Issue13534() ("BCP036", DiagnosticLevel.Warning, """The property "gateway" expected a value of type "string" but the provided value is of type "object"."""), }); } + + // https://github.com/Azure/bicep/issues/13556 + [TestMethod] + public void Distinction_between_empty_and_untyped_objects_should_survive_compilation_to_JSON() + { + var originalTypesTemplate = """ + @export() + type emptyObject = {} + + @export() + type untypedObject = object + """; + + var compiledTypesTemplate = CompilationHelper.Compile(originalTypesTemplate).Template; + compiledTypesTemplate.Should().NotBeNull(); + + var result = CompilationHelper.Compile( + ("types.bicep", originalTypesTemplate), + ("types.json", compiledTypesTemplate!.ToString()), + ("main.bicep", """ + import * as fromBicep from 'types.bicep' + import * as fromJson from 'types.json' + + param a fromBicep.emptyObject = {fizz: 'buzz'} + param b fromBicep.untypedObject = {foo: 'bar'} + param c fromJson.emptyObject = {snap: 'crackle'} + param d fromJson.untypedObject = {wishy: 'washy'} + """)); + + result.ExcludingLinterDiagnostics().Should().HaveDiagnostics(new[] + { + ("BCP037", DiagnosticLevel.Warning, """The property "fizz" is not allowed on objects of type "{ }". No other properties are allowed."""), + ("BCP037", DiagnosticLevel.Warning, """The property "snap" is not allowed on objects of type "{ }". No other properties are allowed."""), + }); + } } diff --git a/src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs b/src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs index 8ce07bbcc15..db805d1d18e 100644 --- a/src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs +++ b/src/Bicep.Core.IntegrationTests/UserDefinedTypeTests.cs @@ -450,6 +450,18 @@ public void Warning_should_be_shown_when_setting_unknown_properties_on_unsealed_ }); } + [TestMethod] + public void Empty_object_should_be_distinguishable_from_untyped_object_in_compiled_JSON() + { + var result = CompilationHelper.Compile(""" + type emptyObject = {} + type untypedObect = object + """); + + result.Template.Should().HaveValueAtPath("definitions.emptyObject.properties", new JObject()); + result.Template.Should().NotHaveValueAtPath("definitions.untypedObject.properties"); + } + [TestMethod] public void Error_should_be_shown_when_setting_unknown_properties_that_do_not_match_additional_properties_type() { diff --git a/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.json b/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.json index 72de3aa1780..006b6248a95 100644 --- a/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.json +++ b/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.json @@ -6,7 +6,7 @@ "_generator": { "name": "bicep", "version": "dev", - "templateHash": "7210691896706139249" + "templateHash": "8162009081861809153" } }, "definitions": { @@ -194,6 +194,7 @@ }, "stringStringDictionary": { "type": "object", + "properties": {}, "additionalProperties": { "type": "string" } @@ -665,6 +666,7 @@ }, "discriminatedUnionInlineAdditionalProps1": { "type": "object", + "properties": {}, "additionalProperties": { "type": "object", "discriminator": { @@ -682,6 +684,7 @@ }, "discriminatedUnionInlineAdditionalProps2": { "type": "object", + "properties": {}, "additionalProperties": { "type": "object", "discriminator": { diff --git a/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.sourcemap.bicep b/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.sourcemap.bicep index 6e62da3fb61..efc1e67760d 100644 --- a/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.sourcemap.bicep +++ b/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.sourcemap.bicep @@ -305,6 +305,7 @@ type tupleSecondItem = tuple[1] type stringStringDictionary = { //@ "stringStringDictionary": { //@ "type": "object", +//@ "properties": {}, //@ }, *: string //@ "additionalProperties": { @@ -903,6 +904,7 @@ type discriminatorUnionAsPropertyType = { type discriminatedUnionInlineAdditionalProps1 = { //@ "discriminatedUnionInlineAdditionalProps1": { //@ "type": "object", +//@ "properties": {}, //@ }, @discriminator('type') *: typeA | typeB @@ -925,6 +927,7 @@ type discriminatedUnionInlineAdditionalProps1 = { type discriminatedUnionInlineAdditionalProps2 = { //@ "discriminatedUnionInlineAdditionalProps2": { //@ "type": "object", +//@ "properties": {}, //@ }, @discriminator('type') *: (typeA | typeB)? diff --git a/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.symbolicnames.json b/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.symbolicnames.json index 72de3aa1780..006b6248a95 100644 --- a/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.symbolicnames.json +++ b/src/Bicep.Core.Samples/Files/baselines/TypeDeclarations_LF/main.symbolicnames.json @@ -6,7 +6,7 @@ "_generator": { "name": "bicep", "version": "dev", - "templateHash": "7210691896706139249" + "templateHash": "8162009081861809153" } }, "definitions": { @@ -194,6 +194,7 @@ }, "stringStringDictionary": { "type": "object", + "properties": {}, "additionalProperties": { "type": "string" } @@ -665,6 +666,7 @@ }, "discriminatedUnionInlineAdditionalProps1": { "type": "object", + "properties": {}, "additionalProperties": { "type": "object", "discriminator": { @@ -682,6 +684,7 @@ }, "discriminatedUnionInlineAdditionalProps2": { "type": "object", + "properties": {}, "additionalProperties": { "type": "object", "discriminator": { diff --git a/src/Bicep.Core.UnitTests/Semantics/ArmTemplateSemanticModelTests.cs b/src/Bicep.Core.UnitTests/Semantics/ArmTemplateSemanticModelTests.cs index b886b1bc1c3..09ae4fa12a0 100644 --- a/src/Bicep.Core.UnitTests/Semantics/ArmTemplateSemanticModelTests.cs +++ b/src/Bicep.Core.UnitTests/Semantics/ArmTemplateSemanticModelTests.cs @@ -514,6 +514,30 @@ public void Resource_derivation_metadata_causes_tagged_union_variant_to_be_loade fallbackType.Properties["name"].TypeReference.Type.Should().Be(TypeFactory.CreateStringLiteralType("default")); } + [TestMethod] + public void Empty_properties_constraint_causes_loaded_object_to_use_FallbackProperty_flag_on_additional_properties() + { + var parameterType = GetLoadedParameterType(""" + { + "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#", + "contentVersion": "1.0.0.0", + "languageVersion": "2.0", + "resources": {}, + "parameters": { + "emptyObject": { + "type": "object", + "properties": {} + } + } + } + """, + "emptyObject"); + + var objectParameterType = parameterType.Should().BeAssignableTo().Subject; + objectParameterType.AdditionalPropertiesType.Should().Be(LanguageConstants.Any); + objectParameterType.AdditionalPropertiesFlags.Should().HaveFlag(TypePropertyFlags.FallbackProperty); + } + private static TypeSymbol GetLoadedParameterType(string jsonTemplate, string parameterName) { var model = LoadModel(jsonTemplate); diff --git a/src/Bicep.Core/Emit/TemplateWriter.cs b/src/Bicep.Core/Emit/TemplateWriter.cs index 7f85765eede..ea452e0cec6 100644 --- a/src/Bicep.Core/Emit/TemplateWriter.cs +++ b/src/Bicep.Core/Emit/TemplateWriter.cs @@ -745,10 +745,7 @@ private ObjectExpression GetTypePropertiesForObjectType(ObjectTypeExpression exp propertySchemata.Add(ExpressionFactory.CreateObjectProperty(property.PropertyName, propertySchema, property.SourceSyntax)); } - if (propertySchemata.Any()) - { - properties.Add(ExpressionFactory.CreateObjectProperty("properties", ExpressionFactory.CreateObject(propertySchemata, expression.SourceSyntax))); - } + properties.Add(ExpressionFactory.CreateObjectProperty("properties", ExpressionFactory.CreateObject(propertySchemata, expression.SourceSyntax))); if (expression.AdditionalPropertiesExpression is { } addlPropsType) { diff --git a/src/Bicep.Core/TypeSystem/ArmTemplateTypeLoader.cs b/src/Bicep.Core/TypeSystem/ArmTemplateTypeLoader.cs index d203b2ef15c..d3caee09c86 100644 --- a/src/Bicep.Core/TypeSystem/ArmTemplateTypeLoader.cs +++ b/src/Bicep.Core/TypeSystem/ArmTemplateTypeLoader.cs @@ -243,11 +243,11 @@ private static TypeSymbol GetObjectType(SchemaValidationContext context, ITempla return new DiscriminatedObjectType(string.Join(" | ", TypeHelper.GetOrderedTypeNames(variants)), flags, discriminator.PropertyName.Value, variants); } - return GetObjectType(context, schemaNode.Properties.CoalesceEnumerable(), schemaNode.Required?.Value, schemaNode.AdditionalProperties, flags); + return GetObjectType(context, schemaNode.Properties, schemaNode.Required?.Value, schemaNode.AdditionalProperties, flags); } private static TypeSymbol GetObjectType(SchemaValidationContext context, - IEnumerable> properties, + IEnumerable>? properties, IEnumerable? requiredProperties, TemplateBooleanOrSchemaNode? additionalProperties, TypeSymbolValidationFlags flags) @@ -255,26 +255,30 @@ private static TypeSymbol GetObjectType(SchemaValidationContext context, ITempla var requiredProps = requiredProperties is not null ? ImmutableHashSet.CreateRange(requiredProperties) : null; ObjectTypeNameBuilder nameBuilder = new(); - List propertyList = new(); + List? propertyList = null; ITypeReference? additionalPropertiesType = LanguageConstants.Any; TypePropertyFlags additionalPropertiesFlags = TypePropertyFlags.FallbackProperty; - foreach (var (propertyName, schema) in properties) + if (properties is not null) { - // depending on the language version, either only properties included in schemaNode.Required are required, - // or all of them are (but some may be nullable) - var required = context.TemplateLanguageVersion?.HasFeature(TemplateLanguageFeature.NullableParameters) == true - || (requiredProps?.Contains(propertyName) ?? false); - var propertyFlags = required ? TypePropertyFlags.Required : TypePropertyFlags.None; - var description = schema.Metadata?.Value is JObject metadataObject && - metadataObject.TryGetValue(LanguageConstants.MetadataDescriptionPropertyName, out var descriptionToken) && - descriptionToken is JValue { Value: string descriptionString } - ? descriptionString - : null; - - var (type, typeName) = GetDeferrableTypeInfo(context, schema); - propertyList.Add(new(propertyName, type, propertyFlags, description)); - nameBuilder.AppendProperty(propertyName, typeName); + propertyList = new(); + foreach (var (propertyName, schema) in properties) + { + // depending on the language version, either only properties included in schemaNode.Required are required, + // or all of them are (but some may be nullable) + var required = context.TemplateLanguageVersion?.HasFeature(TemplateLanguageFeature.NullableParameters) == true + || (requiredProps?.Contains(propertyName) ?? false); + var propertyFlags = required ? TypePropertyFlags.Required : TypePropertyFlags.None; + var description = schema.Metadata?.Value is JObject metadataObject && + metadataObject.TryGetValue(LanguageConstants.MetadataDescriptionPropertyName, out var descriptionToken) && + descriptionToken is JValue { Value: string descriptionString } + ? descriptionString + : null; + + var (type, typeName) = GetDeferrableTypeInfo(context, schema); + propertyList.Add(new(propertyName, type, propertyFlags, description)); + nameBuilder.AppendProperty(propertyName, typeName); + } } if (additionalProperties is not null) @@ -293,12 +297,12 @@ private static TypeSymbol GetObjectType(SchemaValidationContext context, ITempla } } - if (propertyList.Count == 0 && additionalProperties is null) + if (propertyList is null && additionalProperties is null) { return flags.HasFlag(TypeSymbolValidationFlags.IsSecure) ? LanguageConstants.SecureObject : LanguageConstants.Object; } - return new ObjectType(nameBuilder.ToString(), flags, propertyList, additionalPropertiesType, additionalPropertiesFlags); + return new ObjectType(nameBuilder.ToString(), flags, propertyList.CoalesceEnumerable(), additionalPropertiesType, additionalPropertiesFlags); } private class SansMetadata : ITemplateSchemaNode