Dart: allow non-const constructible defaults to work with 'Positional…

…Defaults' (#1632) ------ Motivation ------ When fields with default values were defined in a struct and their types did not provide 'const' constructors, when such fields were used in combination with 'PositionalDefaults', then the generated code did not compile. ------ Designed solution ------ In order to be able to generate the code, which compiles when 'PositionalDefaults' annotation is used together with default values of types that do not provide const constructors, the optional values have been used. When Gluecodium encounters a type that does not provide const constructor it makes it optional in positional defaults constructor and uses null as default. Later, on initializer list it checks if user provided value different than null and if not, then initializes the field with the actual default value. This way the limitation related to const constructors is surpassed. However, if the user wants to have nullable type that does not provide const constructor and uses default value different than null, then the behavior may be surprising. Therefore, the validator was implemented for Dart to prevent users from such usage. ------ Contents of change ------ 1. New smoke and functional tests, which confirm invalid behavior. 2. Implementation of designed fix. 3. New functional tests, which confirm that the fix works as expected. 4. Validator of default values to prevent unexpected behavior + unit tests. Signed-off-by: Patryk Wrobel <[email protected]>
heremaps · Jan 14, 2025 · eff29f1 · eff29f1
1 parent 2528ae3
commit eff29f1
Show file tree

Hide file tree

Showing 11 changed files with 698 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## Unreleased
 ### Bug fixes:
+ * Dart: fixed a bug related to compilation error caused by usage of `@PositionalDefaults` and default value for a field that uses type, which does not provide const constructor.
  * Dart: disabled generation of `invalid_use_of_visible_for_testing_member` warning from the generated code for a hierarchy of classes.
  * Dart: removed generation of redundant import for constants declared with external types. The redundant import caused linter warnings.
  * Dart: removed generation of redundant `default` clause in exhaustive switch-cases for enumerations. The redundant `default` caused linter warnings.

diff --git a/docs/lime_attributes.md b/docs/lime_attributes.md
@@ -131,6 +131,12 @@ name was defined through command-line parameters. If the tag is not present, the
 positional parameters in Dart. Can only be applied to a struct that has at least one field with a default value. The
 positional defaults constructor will be generated with a `@Deprecated` annotation, if _DeprecationMessage_ is
 specified.
+> **Important:** if the constructor used for the default value is non-const, then the generated constructor will use
+> optional value to prevent compilation error. However, this implies certain limitation for default values of nullable
+> types, which do not provide const constructor (i.a. Blob type or custom structures that are not annotated as `@Immutable`).
+>
+> If the field is nullable and its type does not provide const constructor then the only default value that is accepted
+> can be null. If any other value is used then the generator will raise validation error.
 * **Attribute** **=** **"**_Annotation_**"**: marks an element to be marked with the given annotation in Dart
 generated code. _Annotation_ does not need to be prepended with `@`. _Annotation_ can contain parameters, e.g.
 `@Dart(Attribute="Deprecated(\"It's deprecated.\")")`. If some of the parameters are string literals, their enclosing

diff --git a/functional-tests/functional/dart/test/Defaults_test.dart b/functional-tests/functional/dart/test/Defaults_test.dart
@@ -18,6 +18,7 @@
 //
 // -------------------------------------------------------------------------------------------------
 
+import 'dart:typed_data';
 import "package:test/test.dart";
 import "package:functional/test.dart";
 import "../test_suite.dart";
@@ -103,6 +104,48 @@ void main() {
     expect(result.intField, 13);
     expect(result.stringField, "foobar");
   });
+  _testSuite.test("Check positional defaults for non-const constructible types", () {
+    // Case 1: all defaults.
+    final first = PosDefaultStructWithCustomStructsFields();
+    expect(first.nonConstCtorField0.intField, 42);
+    expect(first.nonConstCtorField1.someField1.intField, 42);
+    expect(first.nonConstCtorField2.stringField, "Some string");
+    expect(first.nonConstCtorField3.nullableListField, null);
+    expect(first.nonConstCtorField5, Uint8List.fromList([]));
+    expect(first.nonConstCtorField6, Uint8List.fromList([222, 173, 190, 239]));
+    expect(first.nonConstCtorField7, null);
+
+    // Case 2: custom values.
+    final second = PosDefaultStructWithCustomStructsFields(
+      // Fields with const constructors.
+      AnotherImmutableStructWithDefaults.withDefaults(),
+      null,
+      [],
+      null,
+      0,
+      0.0,
+      null,
+      null,
+      // Fields without const constructor.
+      StructWithAllDefaults(21, "ABC"),
+      PosDefaultStructWithFieldUsingImmutableStruct(),
+      SomeMutableCustomStructWithDefaults(21, "Another string", [7, 7, 7]),
+      StructWithNullableCollectionDefaults(),
+      StructWithAllDefaults(44, "DEF"),
+      Uint8List.fromList([1, 2, 3]),
+      Uint8List.fromList([4, 5, 6]),
+      Uint8List.fromList([7, 8, 9])
+    );
+
+    expect(second.nonConstCtorField0.intField, 21);
+    expect(second.nonConstCtorField1.someField1.intField, 42);
+    expect(second.nonConstCtorField2.stringField, "Another string");
+    expect(second.nonConstCtorField3.nullableListField, null);
+    expect(second.nonConstCtorField4?.stringField, "DEF");
+    expect(second.nonConstCtorField5, Uint8List.fromList([1, 2, 3]));
+    expect(second.nonConstCtorField6, Uint8List.fromList([4, 5, 6]));
+    expect(second.nonConstCtorField7, Uint8List.fromList([7, 8, 9]));
+  });
   _testSuite.test("Check positional enumerator defaults", () {
     final result = StructWithEnums();
 

diff --git a/functional-tests/functional/input/lime/PositionalDefaults.lime b/functional-tests/functional/input/lime/PositionalDefaults.lime
@@ -72,6 +72,36 @@ struct PosDefaultStructWithFieldUsingImmutableStruct {
     someField1: AnotherImmutableStructWithDefaults = {}
 }
 
+@Dart(PositionalDefaults)
+@Java(Skip) @Swift(Skip)
+struct SomeMutableCustomStructWithDefaults {
+    intField: Int = 77
+    stringField: String = "Some string"
+    listField: List<Int> = [1, 2, 3]
+}
+
+@Dart(PositionalDefaults)
+@Java(Skip) @Swift(Skip)
+struct PosDefaultStructWithCustomStructsFields {
+    constCtorField0: AnotherImmutableStructWithDefaults = {}
+    constCtorField1: AnotherImmutableStructWithDefaults? = {}
+    constCtorField2: List<String> = ["abc", "def", "ghi"]
+    constCtorField3: Map<String, String>? = null
+    constCtorField4: Int = 77
+    constCtorField5: Double = 77.77
+    constCtorField6: AnotherImmutableStructWithDefaults? = {}
+    constCtorField7: AnotherImmutableStructWithDefaults? = null
+
+    nonConstCtorField0: StructWithAllDefaults = {}
+    nonConstCtorField1: PosDefaultStructWithFieldUsingImmutableStruct = {}
+    nonConstCtorField2: SomeMutableCustomStructWithDefaults = {}
+    nonConstCtorField3: StructWithNullableCollectionDefaults = {}
+    nonConstCtorField4: StructWithAllDefaults? = null
+    nonConstCtorField5: Blob = []
+    nonConstCtorField6: Blob = [222, 173, 190, 239]
+    nonConstCtorField7: Blob? = null
+}
+
 @Dart(PositionalDefaults)
 @Java(Skip) @Swift(Skip)
 struct PosDefaultsWithDuration {

diff --git a/gluecodium/src/main/java/com/here/gluecodium/generator/dart/DartDefaultValuesValidator.kt b/gluecodium/src/main/java/com/here/gluecodium/generator/dart/DartDefaultValuesValidator.kt
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2016-2024 HERE Europe B.V.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ * License-Filename: LICENSE
+ */
+
+package com.here.gluecodium.generator.dart
+
+import com.here.gluecodium.common.LimeLogger
+import com.here.gluecodium.model.lime.LimeAttributeType.DART
+import com.here.gluecodium.model.lime.LimeAttributeType.IMMUTABLE
+import com.here.gluecodium.model.lime.LimeAttributeValueType
+import com.here.gluecodium.model.lime.LimeBasicType
+import com.here.gluecodium.model.lime.LimeElement
+import com.here.gluecodium.model.lime.LimeStruct
+import com.here.gluecodium.model.lime.LimeValue
+
+class DartDefaultValuesValidator(private val logger: LimeLogger) {
+    fun validate(referenceMap: Map<String, LimeElement>): Boolean {
+        val allStructs = referenceMap.values.filterIsInstance<LimeStruct>()
+        return !allStructs.map { validatePositionalDefaults(it) }.contains(false)
+    }
+
+    private fun validatePositionalDefaults(limeStruct: LimeStruct): Boolean {
+        if (!limeStruct.attributes.have(DART, LimeAttributeValueType.POSITIONAL_DEFAULTS)) {
+            return true
+        }
+
+        var result = true
+        for (field in limeStruct.initializedFields) {
+            // Types other than BLOB and custom LimeStructs provide const constructors.
+            val basicType = field.typeRef.type as? LimeBasicType
+            if (field.typeRef.type !is LimeStruct && (basicType?.typeId != LimeBasicType.TypeId.BLOB)) {
+                continue
+            }
+
+            // Custom immutable types provide const constructors.
+            if (field.typeRef.type.attributes.have(IMMUTABLE)) {
+                continue
+            }
+
+            // If the type does not provide const constructor, then generated 'PositionalDefaults' constructor
+            // will utilize optional field to avoid compilation errors.
+            // However, if the user uses 'nullable type' and wants to set value that is different from 'null' as the
+            // default one, then if somebody passed 'null' to the generated constructor, then it wouldn't be honored.
+            //
+            // Do not allow users to do that.
+            if (field.typeRef.isNullable && field.defaultValue!! !is LimeValue.Null) {
+                logger.error(field, "$NULLABLE_ERROR_REASON; please check $DOCS_LINK")
+                result = false
+            }
+        }
+
+        return result
+    }
+
+    companion object {
+        const val NULLABLE_ERROR_REASON =
+            "For '@PositionalDefaults' structs, nullable fields that do not provide" +
+                " const constructors must use 'null' default value"
+        const val DOCS_LINK = "https://github.com/heremaps/gluecodium/blob/master/docs/lime_attributes.md#dart-specific-attributes"
+    }
+}
diff --git a/gluecodium/src/main/java/com/here/gluecodium/generator/dart/DartGenerator.kt b/gluecodium/src/main/java/com/here/gluecodium/generator/dart/DartGenerator.kt
@@ -122,10 +122,12 @@ internal class DartGenerator : Generator {
         val dartNameResolver = DartNameResolver(ffiReferenceMap, nameRules, limeLogger, commentsProcessor)
         val ffiNameResolver = FfiNameResolver(ffiReferenceMap, nameRules, internalPrefix)
 
-        val validationResult =
+        val overloadsValidationResult =
             DartOverloadsValidator(dartNameResolver, limeLogger, overloadsWerror)
                 .validate(dartFilteredModel.referenceMap.values)
-        if (!validationResult) {
+        val defaultValuesValidationResult =
+            DartDefaultValuesValidator(limeLogger).validate(dartFilteredModel.referenceMap)
+        if (!overloadsValidationResult || !defaultValuesValidationResult) {
             throw GluecodiumExecutionException("Validation errors found, see log for details.")
         }
 

diff --git a/gluecodium/src/main/java/com/here/gluecodium/generator/dart/DartGeneratorPredicates.kt b/gluecodium/src/main/java/com/here/gluecodium/generator/dart/DartGeneratorPredicates.kt
@@ -25,9 +25,11 @@ import com.here.gluecodium.generator.common.CommonGeneratorPredicates
 import com.here.gluecodium.model.lime.LimeAttributeType.DART
 import com.here.gluecodium.model.lime.LimeAttributeType.IMMUTABLE
 import com.here.gluecodium.model.lime.LimeAttributeValueType.POSITIONAL_DEFAULTS
+import com.here.gluecodium.model.lime.LimeBasicType
 import com.here.gluecodium.model.lime.LimeContainer
 import com.here.gluecodium.model.lime.LimeElement
 import com.here.gluecodium.model.lime.LimeExternalDescriptor
+import com.here.gluecodium.model.lime.LimeField
 import com.here.gluecodium.model.lime.LimeNamedElement
 import com.here.gluecodium.model.lime.LimeStruct
 import com.here.gluecodium.model.lime.LimeType
@@ -45,6 +47,23 @@ internal class DartGeneratorPredicates(
             "allFieldsCtorIsPublic" to { limeStruct: Any ->
                 limeStruct is LimeStruct && allFieldsCtorIsPublic(limeStruct)
             },
+            "fieldHasConstCtor" to { limeField: Any ->
+                if (limeField is LimeField) {
+                    when (limeField.typeRef.type) {
+                        is LimeBasicType -> {
+                            val basicType = limeField.typeRef.type as LimeBasicType
+                            basicType.typeId != LimeBasicType.TypeId.BLOB
+                        }
+                        is LimeStruct -> limeField.typeRef.type.attributes.have(IMMUTABLE)
+                        else -> true
+                    }
+                } else {
+                    false
+                }
+            },
+            "fieldHasDefaultValue" to { limeField: Any ->
+                limeField is LimeField && limeField.defaultValue != null
+            },
             "isInternal" to { element: Any ->
                 when (element) {
                     // Dart has no type nesting, so all types are "outside" and have to check for an internal outer type.

diff --git a/gluecodium/src/main/resources/templates/dart/DartStructConstructors.mustache b/gluecodium/src/main/resources/templates/dart/DartStructConstructors.mustache
@@ -28,8 +28,27 @@
 {{/instanceOf}}{{!!
 }}  {{#if attributes.immutable}}const {{/if}}{{resolveName}}{{#if external.dart.converter}}Internal{{/if}}({{!!
   }}{{#uninitializedFields}}{{resolveName typeRef}} {{resolveName}}, {{/uninitializedFields}}{{!!
-  }}[{{#initializedFields}}{{resolveName typeRef}} {{resolveName}} = {{>constPrefix}}{{resolveName defaultValue}}{{#if iter.hasNext}}, {{/if}}{{/initializedFields}}])
-    : {{#fields}}{{resolveName "visibility"}}{{resolveName}} = {{resolveName}}{{#if iter.hasNext}}, {{/if}}{{/fields}};
+  }}[{{#initializedFields}}{{!!
+}}{{#ifPredicate "fieldHasConstCtor"}}{{!!
+}}{{resolveName typeRef}} {{resolveName}} = {{>constPrefix}}{{resolveName defaultValue}}{{#if iter.hasNext}}, {{/if}}{{!!
+}}{{/ifPredicate}}{{!!
+}}{{#unlessPredicate "fieldHasConstCtor"}}{{!!
+}}{{resolveName typeRef.asNullable}} {{resolveName}} = null{{#if iter.hasNext}}, {{/if}}{{!!
+}}{{/unlessPredicate}}{{!!
+}}{{/initializedFields}}])
+    : {{#fields}}{{!!
+}}{{#ifPredicate "fieldHasDefaultValue"}}{{!!
+}}{{#ifPredicate "fieldHasConstCtor"}}{{!!
+}}{{resolveName "visibility"}}{{resolveName}} = {{resolveName}}{{#if iter.hasNext}}, {{/if}}{{!!
+}}{{/ifPredicate}}{{!!
+}}{{#unlessPredicate "fieldHasConstCtor"}}{{!!
+}}{{resolveName "visibility"}}{{resolveName}} = {{resolveName}} ?? {{resolveName defaultValue}}{{#if iter.hasNext}}, {{/if}}{{!!
+}}{{/unlessPredicate}}{{!!
+}}{{/ifPredicate}}{{!!
+}}{{#unlessPredicate "fieldHasDefaultValue"}}{{!!
+}}{{resolveName "visibility"}}{{resolveName}} = {{resolveName}}{{#if iter.hasNext}}, {{/if}}{{!!
+}}{{/unlessPredicate}}{{!!
+}}{{/fields}};
 {{/if}}{{!!
 
 }}{{#unless attributes.dart.positionalDefaults initializedFields}}{{!!