Improve unknown annotation trait errors

smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelParser.java

@@ -692,12 +692,20 @@ private Node coerceTraitValue(
             ShapeId traitId, Node value, boolean isAnnotation, Function<ShapeId, ShapeType> typeProvider) {
         if (isAnnotation && value.isNullNode()) {
             ShapeType targetType = typeProvider.apply(traitId);
-            if (targetType != null) {
-                if (targetType == ShapeType.STRUCTURE || targetType == ShapeType.MAP) {
-                    return new ObjectNode(Collections.emptyMap(), value.getSourceLocation());
-                } else if (targetType == ShapeType.LIST || targetType == ShapeType.SET) {
-                    return new ArrayNode(Collections.emptyList(), value.getSourceLocation());
-                }
+            if (targetType == null || targetType == ShapeType.STRUCTURE || targetType == ShapeType.MAP) {
+                // The targetType == null condition helps mitigate a confusing
+                // failure mode where a trait isn't defined in the model, but a
+                // TraitService is found as a service provider for the trait.
+                // If the TraitService creates an annotation trait, then using null
+                // instead of object results in a failure about passing null for an
+                // annotation trait, and that's confusing because the actual error
+                // message should be about the missing trait definition. Because the
+                // vast majority of annotation traits are modeled as objects, this
+                // makes the assumption that the value is an object (which addresses
+                // the above failure case).
+                return new ObjectNode(Collections.emptyMap(), value.getSourceLocation());
+            } else if (targetType == ShapeType.LIST || targetType == ShapeType.SET) {
+                return new ArrayNode(Collections.emptyList(), value.getSourceLocation());
             }
         }
 

smithy-model/src/test/java/software/amazon/smithy/model/loader/IdlModelLoaderTest.java

@@ -34,7 +34,9 @@
 import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.traits.DocumentationTrait;
+import software.amazon.smithy.model.traits.DynamicTrait;
 import software.amazon.smithy.model.traits.StringTrait;
+import software.amazon.smithy.model.traits.TraitFactory;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidatedResult;
 import software.amazon.smithy.model.validation.ValidatedResultException;
@@ -182,4 +184,43 @@ public void properlyLoadsOperationsWithUseStatements() {
         assertThat(identifiers.get("s"), equalTo(ShapeId.from("smithy.api#String")));
         assertThat(identifiers.get("x"), equalTo(ShapeId.from("smithy.example.other#X")));
     }
+
+    @Test
+    public void addressesConfusingEdgeCaseForUnknownTraits() {
+        TraitFactory defaultFactory = TraitFactory.createServiceFactory();
+
+        // This trait factory ensures that the trait under test is coerced to
+        // an object instead of a null. The definition of @foo can't be found,
+        // but we know it's an annotation trait, so it's coerced to the most
+        // common annotation trait type (an object). This makes the error
+        // message for annotation traits that have no trait definition but do
+        // have TraitService SPI more intelligible.
+        TraitFactory factory = (id, target, value) -> {
+            if (id.equals(ShapeId.from("smithy.example#foo"))) {
+                assertThat("Did not coerce the unknown annotation trait to an object", value.isObjectNode(), is(true));
+                return Optional.of(new DynamicTrait(id, value));
+            } else {
+                return defaultFactory.createTrait(id, target, value);
+            }
+        };
+
+        List<ValidationEvent> events = Model.assembler()
+                .traitFactory(factory)
+                .addUnparsedModel("foo.smithy", "namespace smithy.example\n@foo\nstring MyString\n")
+                .assemble()
+                .getValidationEvents();
+
+        // Ensure that there is also an event that the @foo trait couldn't be found.
+        // This is a bit overkill, but ensures that it works end-to-end.
+        for (ValidationEvent event : events) {
+            if (event.getShapeId().filter(id -> id.equals(ShapeId.from("smithy.example#MyString"))).isPresent()) {
+                if (event.getMessage().contains("Unable to resolve trait")) {
+                    // Just break out of the test early when it's found. We're done.
+                    return;
+                }
+            }
+        }
+
+        Assertions.fail("Did not find expected unknown trait event: " + events);
+    }
 }