Fail model builds early when syntax errors occur

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

@@ -60,9 +60,6 @@ final class LoaderVisitor {
     private static final Logger LOGGER = Logger.getLogger(LoaderVisitor.class.getName());
     private static final TraitDefinition.Provider TRAIT_DEF_PROVIDER = new TraitDefinition.Provider();
 
-    /** Whether or not a call to {@link #onEnd()} has been made. */
-    private boolean calledOnEnd;
-
     /** Factory used to create traits. */
     private final TraitFactory traitFactory;
 
@@ -96,6 +93,9 @@ final class LoaderVisitor {
     /** Map of shape aliases to their alias. */
     private final Map<String, ShapeId> useShapes = new HashMap<>();
 
+    /** The result that is populated when onEnd is called. */
+    private ValidatedResult<Model> result;
+
     private static final class PendingTrait {
         final ShapeId id;
         final Node value;
@@ -278,7 +278,7 @@ public <T> Optional<T> getProperty(String property, Class<T> type) {
     }
 
     private void validateState(FromSourceLocation sourceLocation) {
-        if (calledOnEnd) {
+        if (result != null) {
             throw new IllegalStateException("Cannot call visitor method because visitor has called onEnd");
         }
     }
@@ -502,8 +502,10 @@ void onUseShape(ShapeId id, FromSourceLocation location) {
      * @return Returns the validated model result.
      */
     public ValidatedResult<Model> onEnd() {
-        validateState(SourceLocation.NONE);
-        calledOnEnd = true;
+        if (result != null) {
+            return result;
+        }
+
         Model.Builder modelBuilder = Model.builder().metadata(metadata);
 
         finalizeShapeTargets();
@@ -551,7 +553,9 @@ public ValidatedResult<Model> onEnd() {
 
         // Add any remaining built shapes.
         modelBuilder.addShapes(builtShapes.values());
-        return new ValidatedResult<>(modelBuilder.build(), events);
+        result = new ValidatedResult<>(modelBuilder.build(), events);
+
+        return result;
     }
 
     private void finalizeShapeTargets() {

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

@@ -45,7 +45,6 @@
 import software.amazon.smithy.model.validation.ValidationEvent;
 import software.amazon.smithy.model.validation.Validator;
 import software.amazon.smithy.model.validation.ValidatorFactory;
-import software.amazon.smithy.utils.ListUtils;
 
 /**
  * Assembles and validates a {@link Model} from documents, files, shapes, and
@@ -448,31 +447,27 @@ public ModelAssembler removeProperty(String setting) {
      *  and validation events.
      */
     public ValidatedResult<Model> assemble() {
+        LoaderVisitor visitor = createLoaderVisitor();
+
         try {
-            return doAssemble();
+            return doAssemble(visitor);
         } catch (SourceException e) {
-            return ValidatedResult.fromErrors(ListUtils.of(ValidationEvent.fromSourceException(e)));
+            visitor.onError(ValidationEvent.fromSourceException(e));
+            return visitor.onEnd();
         }
     }
 
-    private ValidatedResult<Model> doAssemble() {
+    private LoaderVisitor createLoaderVisitor() {
         if (traitFactory == null) {
             traitFactory = LazyTraitFactoryHolder.INSTANCE;
         }
 
-        LoaderVisitor visitor = new LoaderVisitor(traitFactory, properties);
-
-        // Load models first to ensure a version is set.
-        for (Map.Entry<String, Supplier<InputStream>> modelEntry : inputStreamModels.entrySet()) {
-            if (!ModelLoader.load(modelEntry.getKey(), modelEntry.getValue(), visitor)) {
-                LOGGER.warning(() -> "No ModelLoader was able to load " + modelEntry.getKey());
-            }
-        }
+        return new LoaderVisitor(traitFactory, properties);
+    }
 
-        if (!documentNodes.isEmpty()) {
-            for (Node node : documentNodes) {
-                ModelLoader.loadParsedNode(node, visitor);
-            }
+    private ValidatedResult<Model> doAssemble(LoaderVisitor visitor) {
+        if (!disablePrelude) {
+            mergeModelIntoVisitor(Prelude.getPreludeModel(), visitor);
         }
 
         shapes.forEach(visitor::onShape);
@@ -482,8 +477,14 @@ private ValidatedResult<Model> doAssemble() {
             mergeModelIntoVisitor(model, visitor);
         }
 
-        if (!disablePrelude) {
-            mergeModelIntoVisitor(Prelude.getPreludeModel(), visitor);
+        for (Node node : documentNodes) {
+            ModelLoader.loadParsedNode(node, visitor);
+        }
+
+        for (Map.Entry<String, Supplier<InputStream>> modelEntry : inputStreamModels.entrySet()) {
+            if (!ModelLoader.load(modelEntry.getKey(), modelEntry.getValue(), visitor)) {
+                LOGGER.warning(() -> "No ModelLoader was able to load " + modelEntry.getKey());
+            }
         }
 
         ValidatedResult<Model> modelResult = visitor.onEnd();

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

@@ -68,6 +68,11 @@ static boolean load(String filename, Supplier<InputStream> contentSupplier, Load
             } else {
                 return false;
             }
+        } catch (ModelSyntaxException e) {
+            // A syntax error in any model is going to really mess up the loading
+            // process. While we *could* tolerate syntax errors and move on, to the
+            // next model, doing so would likely emit many unintelligible errors.
+            throw e;
         } catch (SourceException e) {
             visitor.onError(ValidationEvent.fromSourceException(e));
             return true;

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

@@ -19,6 +19,7 @@
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -61,12 +62,10 @@ public void cannotMutateAfterOnEnd() {
     }
 
     @Test
-    public void cannotCallOnEndTwice() {
-        Assertions.assertThrows(IllegalStateException.class, () -> {
-            LoaderVisitor visitor = new LoaderVisitor(FACTORY);
-            visitor.onEnd();
-            visitor.onEnd();
-        });
+    public void callingOnEndTwiceIsIdempotent() {
+        LoaderVisitor visitor = new LoaderVisitor(FACTORY);
+
+        assertThat(visitor.onEnd(), is(visitor.onEnd()));
     }
 
     @Test

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

@@ -483,10 +483,14 @@ public void canLoadModelsFromJar() {
 
     @Test
     public void gracefullyParsesPartialDocuments() {
-        String document = "namespace foo.baz\nstring MyString\nstr";
+        String document = "namespace foo.baz\n"
+                          + "@required\n" // < this trait is invalid, but that's not validated due to the syntax error
+                          + "string MyString\n"
+                          + "str"; // < syntax error here
         ValidatedResult<Model> result = new ModelAssembler().addUnparsedModel("foo.smithy", document).assemble();
 
         assertTrue(result.isBroken());
+        assertThat(result.getValidationEvents(Severity.ERROR), hasSize(1));
         assertTrue(result.getResult().isPresent());
         assertTrue(result.getResult().get().getShape(ShapeId.from("foo.baz#MyString")).isPresent());
     }