Improve unknown annotation trait errors

When an annotation trait isn't defined in the model but SPI can be found
for it, the error message is about providing a null value for an
AnnotationTrait when an object is expected, and that's confusing. This
change addresses this by assuming that an unknown annotation trait is an
object since that is the most common case (and because annotation traits can
never be null). This doesn't end up resulting in a clean model validation
result, but it gives better error messages about the trait not being defined.

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);
+    }
 }