Add more error recovery support and tests

Add error recovery to control statements and metadata. Ensure that invalid identifiers are turned into ERROR tokens instead of throwing. Error recovery will also consider "$" a recovery token.
smithy-lang · Apr 18, 2023 · 6e89ada · 6e89ada
1 parent 2a00227
commit 6e89ada
Show file tree

Hide file tree

Showing 8 changed files with 211 additions and 54 deletions.
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelLoader.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelLoader.java
@@ -232,44 +232,48 @@ private void parseControlSection() {
         Set<CharSequence> definedKeys = new HashSet<>();
 
         while (tokenizer.getCurrentToken() == IdlToken.DOLLAR) {
-            tokenizer.next();
-            tokenizer.expect(IdlToken.IDENTIFIER, IdlToken.STRING);
-            String key = tokenizer.internString(tokenizer.getCurrentTokenStringSlice());
+            try {
+                tokenizer.next();
+                tokenizer.expect(IdlToken.IDENTIFIER, IdlToken.STRING);
+                String key = tokenizer.internString(tokenizer.getCurrentTokenStringSlice());
 
-            tokenizer.next();
-            tokenizer.skipSpaces();
-            tokenizer.expect(IdlToken.COLON);
-            tokenizer.next();
-            tokenizer.skipSpaces();
+                tokenizer.next();
+                tokenizer.skipSpaces();
+                tokenizer.expect(IdlToken.COLON);
+                tokenizer.next();
+                tokenizer.skipSpaces();
 
-            if (!definedKeys.add(key)) {
-                throw syntax(format("Duplicate control statement `%s`", key));
-            }
+                if (!definedKeys.add(key)) {
+                    throw syntax(format("Duplicate control statement `%s`", key));
+                }
 
-            Node value = IdlNodeParser.expectAndSkipNode(tokenizer, resolver);
+                Node value = IdlNodeParser.expectAndSkipNode(tokenizer, resolver);
 
-            switch (key) {
-                case "version":
-                    onVersion(value);
-                    break;
-                case "operationInputSuffix":
-                    operationInputSuffix = value.expectStringNode().getValue();
-                    break;
-                case "operationOutputSuffix":
-                    operationOutputSuffix = value.expectStringNode().getValue();
-                    break;
-                default:
-                    emit(ValidationEvent.builder()
-                                 .id(Validator.MODEL_ERROR)
-                                 .sourceLocation(value)
-                                 .severity(Severity.WARNING)
-                                 .message(format("Unknown control statement `%s` with value `%s",
-                                                 key, Node.printJson(value)))
-                                 .build());
-                    break;
-            }
+                switch (key) {
+                    case "version":
+                        onVersion(value);
+                        break;
+                    case "operationInputSuffix":
+                        operationInputSuffix = value.expectStringNode().getValue();
+                        break;
+                    case "operationOutputSuffix":
+                        operationOutputSuffix = value.expectStringNode().getValue();
+                        break;
+                    default:
+                        emit(ValidationEvent.builder()
+                                     .id(Validator.MODEL_ERROR)
+                                     .sourceLocation(value)
+                                     .severity(Severity.WARNING)
+                                     .message(format("Unknown control statement `%s` with value `%s",
+                                                     key, Node.printJson(value)))
+                                     .build());
+                        break;
+                }
 
-            tokenizer.expectAndSkipBr();
+                tokenizer.expectAndSkipBr();
+            } catch (ModelSyntaxException e) {
+                errorRecovery(e);
+            }
         }
     }
 
@@ -293,19 +297,23 @@ private void onVersion(Node value) {
 
     private void parseMetadataSection() {
         while (tokenizer.doesCurrentIdentifierStartWith('m')) {
-            tokenizer.expectCurrentLexeme("metadata");
-            tokenizer.next(); // skip "metadata"
-            tokenizer.expectAndSkipSpaces();
-            tokenizer.expect(IdlToken.IDENTIFIER, IdlToken.STRING);
-            String key = tokenizer.internString(tokenizer.getCurrentTokenStringSlice());
-            tokenizer.next();
-            tokenizer.skipSpaces();
-            tokenizer.expect(IdlToken.EQUAL);
-            tokenizer.next();
-            tokenizer.skipSpaces();
-            Node value = IdlNodeParser.expectAndSkipNode(tokenizer, resolver);
-            operations.accept(new LoadOperation.PutMetadata(modelVersion, key, value));
-            tokenizer.expectAndSkipBr();
+            try {
+                tokenizer.expectCurrentLexeme("metadata");
+                tokenizer.next(); // skip "metadata"
+                tokenizer.expectAndSkipSpaces();
+                tokenizer.expect(IdlToken.IDENTIFIER, IdlToken.STRING);
+                String key = tokenizer.internString(tokenizer.getCurrentTokenStringSlice());
+                tokenizer.next();
+                tokenizer.skipSpaces();
+                tokenizer.expect(IdlToken.EQUAL);
+                tokenizer.next();
+                tokenizer.skipSpaces();
+                Node value = IdlNodeParser.expectAndSkipNode(tokenizer, resolver);
+                operations.accept(new LoadOperation.PutMetadata(modelVersion, key, value));
+                tokenizer.expectAndSkipBr();
+            } catch (ModelSyntaxException e) {
+                errorRecovery(e);
+            }
         }
     }
 
@@ -474,7 +482,7 @@ private void errorRecovery(ModelSyntaxException e) {
         }
 
         // Here we do rudimentary error recovery to attempt to make sense of the remaining model.
-        // We do this by scanning tokens until we find the next identifier, docs, or trait at the start of a line.
+        // We do this by scanning tokens until we find the next "$", identifier, docs, or trait at the start of a line.
         // The model is still invalid and will fail to validate, but things like IDEs should still be able to do
         // things like goto definition.
         emit(ValidationEvent.fromSourceException(e));
@@ -490,7 +498,15 @@ private void errorRecovery(ModelSyntaxException e) {
 
     // These tokens are good signals that the next shape is starting.
     private boolean isErrorRecoveryToken(IdlToken token) {
-        return token == IdlToken.IDENTIFIER || token == IdlToken.DOC_COMMENT || token == IdlToken.AT;
+        switch (token) {
+            case IDENTIFIER:
+            case DOC_COMMENT:
+            case AT:
+            case DOLLAR:
+                return true;
+            default:
+                return false;
+        }
     }
 
     private boolean parseShapeDefinition(List<IdlTraitParser.Result> traits, boolean hasDocComment) {

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlTokenizer.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlTokenizer.java
@@ -750,9 +750,15 @@ private IdlToken parseNumber() {
     }
 
     private IdlToken parseIdentifier() {
-        ParserUtils.consumeIdentifier(parser);
+        try {
+            ParserUtils.consumeIdentifier(parser);
+            currentTokenType = IdlToken.IDENTIFIER;
+        } catch (RuntimeException e) {
+            currentTokenType = IdlToken.ERROR;
+            currentTokenError = e.getMessage();
+        }
         currentTokenEnd = parser.position();
-        return currentTokenType = IdlToken.IDENTIFIER;
+        return currentTokenType;
     }
 
     private IdlToken parseString() {

diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/IdlModelLoaderTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/IdlModelLoaderTest.java
@@ -16,6 +16,7 @@
 package software.amazon.smithy.model.loader;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
@@ -34,6 +35,7 @@
 import org.opentest4j.AssertionFailedError;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.SourceLocation;
+import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.shapes.EnumShape;
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.OperationShape;
@@ -45,6 +47,7 @@
 import software.amazon.smithy.model.shapes.StructureShape;
 import software.amazon.smithy.model.traits.DocumentationTrait;
 import software.amazon.smithy.model.traits.DynamicTrait;
+import software.amazon.smithy.model.traits.ExternalDocumentationTrait;
 import software.amazon.smithy.model.traits.StringTrait;
 import software.amazon.smithy.model.traits.Trait;
 import software.amazon.smithy.model.traits.TraitFactory;
@@ -341,9 +344,9 @@ public void setsCorrectLocationForEnum() {
     }
 
     @Test
-    public void doesBasicErrorRecovery() {
+    public void doesBasicErrorRecoveryToTrait() {
         ValidatedResult<Model> result = Model.assembler()
-                .addImport(getClass().getResource("error-recovery.smithy"))
+                .addImport(getClass().getResource("error-recovery/to-trait.smithy"))
                 .assemble();
 
         assertThat(result.isBroken(), is(true));
@@ -356,8 +359,6 @@ public void doesBasicErrorRecovery() {
         assertThat(model.getShape(ShapeId.from("smithy.example#MyInteger")).isPresent(), is(false));
         assertThat(model.getShape(ShapeId.from("smithy.example#MyInteger2")).isPresent(), is(true));
 
-        System.out.println(result.getValidationEvents());
-
         boolean foundSyntax = false;
         boolean foundTrait = false;
         for (ValidationEvent e : result.getValidationEvents()) {
@@ -373,4 +374,103 @@ public void doesBasicErrorRecovery() {
         assertThat(foundSyntax, is(true));
         assertThat(foundTrait, is(true));
     }
+
+    @Test
+    public void doesBasicErrorRecoveryToDocs() {
+        ValidatedResult<Model> result = Model.assembler()
+                .addImport(getClass().getResource("error-recovery/to-docs.smithy"))
+                .assemble();
+
+        assertThat(result.isBroken(), is(true));
+        assertThat(result.getResult().isPresent(), is(true));
+
+        Model model = result.getResult().get();
+
+        ShapeId myInteger = ShapeId.from("smithy.example#MyInteger");
+        assertThat(model.getShape(myInteger).isPresent(), is(true));
+        // Ensure recovery happened on the docs trait, capturing it on the shape.
+        assertThat(model.expectShape(myInteger).hasTrait(DocumentationTrait.class), is(true));
+
+        boolean foundSyntax = false;
+        boolean foundTrait = false;
+        for (ValidationEvent e : result.getValidationEvents()) {
+            if (e.getSeverity() == Severity.ERROR && e.getMessage().contains("Syntax error at line 6, column 28")) {
+                foundSyntax = true;
+            }
+            if (e.getSeverity() == Severity.ERROR && e.getMessage().contains("Unable to resolve trait")) {
+                foundTrait = true;
+            }
+        }
+
+        assertThat(foundSyntax, is(true));
+        assertThat(foundTrait, is(true));
+    }
+
+    @Test
+    public void doesBasicErrorRecoveryToIdentifier() {
+        ValidatedResult<Model> result = Model.assembler()
+                .addImport(getClass().getResource("error-recovery/to-identifier.smithy"))
+                .assemble();
+
+        assertThat(result.isBroken(), is(true));
+        assertThat(result.getResult().isPresent(), is(true));
+
+        Model model = result.getResult().get();
+
+        ShapeId myString = ShapeId.from("smithy.example#MyString");
+        assertThat(model.getShape(myString).isPresent(), is(true));
+        assertThat(model.expectShape(myString).hasTrait(ExternalDocumentationTrait.class), is(false));
+
+        boolean foundSyntax = result.getValidationEvents().stream()
+                .anyMatch(e -> e.getSeverity() == Severity.ERROR
+                               && e.getMessage().contains("Syntax error at line 6, column 28"));
+
+        assertThat(foundSyntax, is(true));
+    }
+
+    @Test
+    public void doesBasicErrorRecoveryToControl() {
+        ValidatedResult<Model> result = Model.assembler()
+                .addImport(getClass().getResource("error-recovery/to-dollar.smithy"))
+                .assemble();
+
+        assertThat(result.isBroken(), is(true));
+        assertThat(result.getResult().isPresent(), is(true));
+
+        Model model = result.getResult().get();
+
+        assertThat(model.getShape(ShapeId.from("smithy.example#MyInteger")).isPresent(), is(true));
+
+        boolean foundSyntax = result.getValidationEvents().stream()
+                .anyMatch(e -> e.getSeverity() == Severity.ERROR
+                               && e.getMessage().contains("Syntax error at line 1, column 5"));
+
+        assertThat(foundSyntax, is(true));
+    }
+
+    @Test
+    public void doesBasicErrorRecoveryInMetadata() {
+        ValidatedResult<Model> result = Model.assembler()
+                .addImport(getClass().getResource("error-recovery/in-metadata.smithy"))
+                .assemble();
+
+        assertThat(result.isBroken(), is(true));
+        assertThat(result.getResult().isPresent(), is(true));
+
+        Model model = result.getResult().get();
+
+        // Ensure the value keys were parsed and invalid keys were not.
+        assertThat(model.getMetadata().keySet(), containsInAnyOrder("valid1", "valid2", "valid3", "valid4"));
+        assertThat(model.getMetadata().get("valid1"), equalTo(Node.from("ok")));
+        assertThat(model.getMetadata().get("valid2"), equalTo(Node.from("ok")));
+        assertThat(model.getMetadata().get("valid3"), equalTo(Node.from("ok")));
+        assertThat(model.getMetadata().get("valid4"), equalTo(Node.from("ok")));
+
+        // Find all four invalid metadata keys.
+        long foundSyntax = result.getValidationEvents().stream()
+                .filter(e -> e.getSeverity() == Severity.ERROR && e.getMessage().contains("Syntax error"))
+                .count();
+
+        assertThat(foundSyntax, equalTo(4L));
+    }
 }
diff --git a/.../src/test/resources/software/amazon/smithy/model/loader/error-recovery/in-metadata.smithy b/.../src/test/resources/software/amazon/smithy/model/loader/error-recovery/in-metadata.smithy
@@ -0,0 +1,10 @@
+$version: "2.0"
+
+metadata invalid1 ==
+metadata valid1 = "ok"
+metadata invalid2 = {"bad"}
+metadata valid2 = "ok"
+metadata invalid3 = ]
+metadata valid3 = "ok"
+metadata invalid4 =
+metadata valid4 = "ok"
diff --git a/...odel/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-docs.smithy b/...odel/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-docs.smithy
@@ -0,0 +1,11 @@
+$version: "2.0"
+
+namespace smithy.example
+
+// Parse error here.
+@externalDocumentation(foo:)
+
+// Then recover on the line below.
+/// Hello
+@unknown
+integer MyInteger
diff --git a/...el/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-dollar.smithy b/...el/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-dollar.smithy
@@ -0,0 +1,6 @@
+$foo
+$version: "2.0"
+// ^ Recover here
+namespace smithy.example
+
+integer MyInteger
diff --git a/...rc/test/resources/software/amazon/smithy/model/loader/error-recovery/to-identifier.smithy b/...rc/test/resources/software/amazon/smithy/model/loader/error-recovery/to-identifier.smithy
@@ -0,0 +1,8 @@
+$version: "2.0"
+
+namespace smithy.example
+
+// Parse error here.
+@externalDocumentation(foo:)
+string MyString
+// ^ Recover here
diff --git a/...smithy/model/loader/error-recovery.smithy → ...del/loader/error-recovery/to-trait.smithy b/...smithy/model/loader/error-recovery.smithy → ...del/loader/error-recovery/to-trait.smithy