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 index 187edded716..e6c53157969 100644 --- 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 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); + } } } @@ -415,51 +423,92 @@ private void parseApplyStatement() { private void parseFirstShapeStatement(SourceLocation possibleDocCommentLocation) { if (tokenizer.getCurrentToken() != IdlToken.EOF) { - if (tokenizer.doesCurrentIdentifierStartWith('a')) { - parseApplyStatement(); - } else { - List traits; - boolean hasDocComment = tokenizer.getCurrentToken() == IdlToken.DOC_COMMENT; - - if (possibleDocCommentLocation == null) { - traits = IdlTraitParser.parseDocsAndTraitsBeforeShape(tokenizer, resolver); + try { + if (tokenizer.doesCurrentIdentifierStartWith('a')) { + parseApplyStatement(); } else { - // In this case, this is the first shape encountered for a model file that doesn't have any - // use statements. We need to take the previously skipped documentation comments to parse - // potential use statements and apply them to this first shape. - String docLines = tokenizer.removePendingDocCommentLines(); - traits = IdlTraitParser.parseDocsAndTraitsBeforeShape(tokenizer, resolver); - // Note that possibleDocCommentLocation is just a mark of where docs _could be_. - if (docLines != null) { - hasDocComment = true; - traits.add(new IdlTraitParser.Result(DocumentationTrait.ID.toString(), - new StringNode(docLines, possibleDocCommentLocation), - IdlTraitParser.TraitType.DOC_COMMENT)); + List traits; + boolean hasDocComment = tokenizer.getCurrentToken() == IdlToken.DOC_COMMENT; + + if (possibleDocCommentLocation == null) { + traits = IdlTraitParser.parseDocsAndTraitsBeforeShape(tokenizer, resolver); + } else { + // In this case, this is the first shape encountered for a model file that doesn't have any + // use statements. We need to take the previously skipped documentation comments to parse + // potential use statements and apply them to this first shape. + String docLines = tokenizer.removePendingDocCommentLines(); + traits = IdlTraitParser.parseDocsAndTraitsBeforeShape(tokenizer, resolver); + // Note that possibleDocCommentLocation is just a mark of where docs _could be_. + if (docLines != null) { + hasDocComment = true; + traits.add(new IdlTraitParser.Result(DocumentationTrait.ID.toString(), + new StringNode(docLines, possibleDocCommentLocation), + IdlTraitParser.TraitType.DOC_COMMENT)); + } } - } - if (parseShapeDefinition(traits, hasDocComment)) { - parseShape(traits); + if (parseShapeDefinition(traits, hasDocComment)) { + parseShape(traits); + } } + } catch (ModelSyntaxException e) { + errorRecovery(e); } } } private void parseSubsequentShapeStatements() { - while (tokenizer.getCurrentToken() != IdlToken.EOF) { - if (tokenizer.doesCurrentIdentifierStartWith('a')) { - parseApplyStatement(); - } else { - List traits; - boolean hasDocComment = tokenizer.getCurrentToken() == IdlToken.DOC_COMMENT; - traits = IdlTraitParser.parseDocsAndTraitsBeforeShape(tokenizer, resolver); - if (parseShapeDefinition(traits, hasDocComment)) { - parseShape(traits); + while (tokenizer.hasNext()) { + try { + if (tokenizer.doesCurrentIdentifierStartWith('a')) { + parseApplyStatement(); + } else { + List traits; + boolean hasDocComment = tokenizer.getCurrentToken() == IdlToken.DOC_COMMENT; + traits = IdlTraitParser.parseDocsAndTraitsBeforeShape(tokenizer, resolver); + if (parseShapeDefinition(traits, hasDocComment)) { + parseShape(traits); + } } + } catch (ModelSyntaxException e) { + errorRecovery(e); } } } + private void errorRecovery(ModelSyntaxException e) { + if (!tokenizer.hasNext()) { + throw 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. + // 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)); + + do { + // Always skip the current token since it was the one that failed. + IdlToken token = tokenizer.next(); + if (tokenizer.getCurrentTokenColumn() == 1 && isErrorRecoveryToken(token)) { + break; + } + } while (tokenizer.hasNext()); + } + + // These tokens are good signals that the next shape is starting. + private boolean isErrorRecoveryToken(IdlToken token) { + switch (token) { + case IDENTIFIER: + case DOC_COMMENT: + case AT: + case DOLLAR: + return true; + default: + return false; + } + } + private boolean parseShapeDefinition(List traits, boolean hasDocComment) { if (tokenizer.getCurrentToken() != IdlToken.EOF) { // Continue to parse if not at the end of the file. 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 index e451570d31e..35b79896f9a 100644 --- 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 @@ -321,7 +321,7 @@ public IdlToken next() { switch (c) { case SimpleParser.EOF: if (currentTokenType == IdlToken.EOF) { - throw new NoSuchElementException("Expected another token but traversed beyond EOF"); + throw new NoSuchElementException("Expected another token but reached EOF"); } currentTokenEnd = parser.position(); return currentTokenType = IdlToken.EOF; @@ -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 index cf4f5ff9470..e457b638936 100644 --- 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; @@ -339,4 +342,135 @@ public void setsCorrectLocationForEnum() { assertThat(fooBarMember.getSourceLocation().getLine(), is(7)); assertThat(fooBarMember.getSourceLocation().getColumn(), is(5)); } + + @Test + public void doesBasicErrorRecoveryToTrait() { + ValidatedResult result = Model.assembler() + .addImport(getClass().getResource("error-recovery/to-trait.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#MyString")).isPresent(), is(true)); + assertThat(model.getShape(ShapeId.from("smithy.example#MyFooIsBroken")).isPresent(), is(false)); + assertThat(model.getShape(ShapeId.from("smithy.example#MyInteger")).isPresent(), is(false)); + assertThat(model.getShape(ShapeId.from("smithy.example#MyInteger2")).isPresent(), 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 9, column 9: Expected COLON(':') but found IDENTIFIER('MyInteger')")) { + 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 doesBasicErrorRecoveryToDocs() { + ValidatedResult 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 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 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 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/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/in-metadata.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/in-metadata.smithy new file mode 100644 index 00000000000..c6eccd88141 --- /dev/null +++ b/smithy-model/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/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-docs.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-docs.smithy new file mode 100644 index 00000000000..d9dd5af6773 --- /dev/null +++ b/smithy-model/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/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-dollar.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-dollar.smithy new file mode 100644 index 00000000000..db0a334bb6f --- /dev/null +++ b/smithy-model/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/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-identifier.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-identifier.smithy new file mode 100644 index 00000000000..86b545ecbab --- /dev/null +++ b/smithy-model/src/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/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-trait.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-trait.smithy new file mode 100644 index 00000000000..5ac7655d45f --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/error-recovery/to-trait.smithy @@ -0,0 +1,13 @@ +$version: "2.0" + +namespace smithy.example + +string MyString + +structure MyFooIsBroken { +// The parser will keep trying to parse here, assuming integer is a key and needs to be followed by ":". +integer MyInteger + +// When the above fails, error recovery kicks in, looking for the next token at the start of the line. +@unknown +integer MyInteger2