Skip to content

Assert trees are valid, fill in missing tests #1808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 5, 2023
Merged

Assert trees are valid, fill in missing tests #1808

merged 1 commit into from
Jun 5, 2023

Conversation

milesziemer
Copy link
Contributor

All TreeTypeTests now expect the entire tree to be valid. This helps us find ambiguity and inconsistencies in the parser for cases when parsing a child tree is invalid only due to the context of the parent. Fixes minor bugs that caused trees to be invalid:

  • enumMember test didn't have a BR after VALUE_ASSIGNMENT

Fills in missing tests for:

  • Operations
  • Block apply statements
  • Entity type name
  • Aggregate type name

Other minor changes and improvements:

  • Adds util methods for parsing optional for-resource, mixins, and value assignment productions.
  • Adds util method for parsing any shape type and name.
  • Updates peekPastSpaces to not skip the current token, in case it is not SP. This simplifies parsing value assignments, and has more intuitive behavior.
  • Removes peekPastWs, which was only used for TRAIT_BODY_VALUE, inlining the implementation. peekPastWs skipped the current token like peekPastSpaces, which is necessary for TRAIT_BODY_VALUE.
  • Add ENTITY_TYPE_NAME production.
  • Updates name of some productions to be consistent with grammar.
  • Fixed a few places where TOKEN was parsed as IDENTIFIER.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@milesziemer milesziemer requested a review from a team as a code owner June 5, 2023 14:23
@milesziemer milesziemer force-pushed the add-tree-is-valid-assertions branch from d428c7e to f1144bc Compare June 5, 2023 14:26
mtdowling
mtdowling reviewed Jun 5, 2023
View reviewed changes
smithy-syntax/src/main/java/software/amazon/smithy/syntax/TreeType.java Outdated
@@ -486,8 +469,7 @@ void parse(CapturingTokenizer tokenizer) {
@Override
void parse(CapturingTokenizer tokenizer) {
tokenizer.withState(this, () -> {
tokenizer.expect(IdlToken.IDENTIFIER);
tokenizer.next(); // skip "input"
IDENTIFIER.parse(tokenizer); // skip "input"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is %s"input" in the grammar, which would be a TOKEN, right? See also output and errors below this.

@milesziemer
Assert trees are valid, fill in missing tests
9a95cec 
All TreeTypeTests now expect the entire tree to be valid. This
helps us find ambiguity and inconsistencies in the parser for
cases when parsing a child tree is invalid only due to the
context of the parent. Fixes minor bugs that caused trees to be
invalid:
- enumMember test didn't have a BR after VALUE_ASSIGNMENT

Fills in missing tests for:
- Operations
- Block apply statements
- Entity type name
- Aggregate type name

Other minor changes and improvements:
- Adds util methods for parsing optional for-resource, mixins,
and value assignment productions.
- Adds util method for parsing any shape type and name.
- Updates peekPastSpaces to not skip the current token, in case
it is not SP. This simplifies parsing value assignments, and has
more intuitive behavior.
- Removes peekPastWs, which was only used for TRAIT_BODY_VALUE,
inlining the implementation. peekPastWs skipped the current token
like peekPastSpaces, which is necessary for TRAIT_BODY_VALUE.
- Add ENTITY_TYPE_NAME production.
- Updates name of some productions to be consistent with grammar.
- Fixed a few places where TOKEN was parsed as IDENTIFIER.
@milesziemer milesziemer force-pushed the add-tree-is-valid-assertions branch from f1144bc to 9a95cec Compare June 5, 2023 15:28
@milesziemer milesziemer merged commit c1bd97f into smithy-lang:token-tree Jun 5, 2023
mtdowling pushed a commit that referenced this pull request Jun 8, 2023
@milesziemer @mtdowling
Assert trees are valid, fill in missing tests (#1808)
0e7d66a 
All TreeTypeTests now expect the entire tree to be valid. This
helps us find ambiguity and inconsistencies in the parser for
cases when parsing a child tree is invalid only due to the
context of the parent. Fixes minor bugs that caused trees to be
invalid:
- enumMember test didn't have a BR after VALUE_ASSIGNMENT

Fills in missing tests for:
- Operations
- Block apply statements
- Entity type name
- Aggregate type name

Other minor changes and improvements:
- Adds util methods for parsing optional for-resource, mixins,
and value assignment productions.
- Adds util method for parsing any shape type and name.
- Updates peekPastSpaces to not skip the current token, in case
it is not SP. This simplifies parsing value assignments, and has
more intuitive behavior.
- Removes peekPastWs, which was only used for TRAIT_BODY_VALUE,
inlining the implementation. peekPastWs skipped the current token
like peekPastSpaces, which is necessary for TRAIT_BODY_VALUE.
- Add ENTITY_TYPE_NAME production.
- Updates name of some productions to be consistent with grammar.
- Fixed a few places where TOKEN was parsed as IDENTIFIER.
mtdowling pushed a commit that referenced this pull request Jun 16, 2023
@milesziemer @mtdowling
Assert trees are valid, fill in missing tests (#1808)
559f8e1 
All TreeTypeTests now expect the entire tree to be valid. This
helps us find ambiguity and inconsistencies in the parser for
cases when parsing a child tree is invalid only due to the
context of the parent. Fixes minor bugs that caused trees to be
invalid:
- enumMember test didn't have a BR after VALUE_ASSIGNMENT

Fills in missing tests for:
- Operations
- Block apply statements
- Entity type name
- Aggregate type name

Other minor changes and improvements:
- Adds util methods for parsing optional for-resource, mixins,
and value assignment productions.
- Adds util method for parsing any shape type and name.
- Updates peekPastSpaces to not skip the current token, in case
it is not SP. This simplifies parsing value assignments, and has
more intuitive behavior.
- Removes peekPastWs, which was only used for TRAIT_BODY_VALUE,
inlining the implementation. peekPastWs skipped the current token
like peekPastSpaces, which is necessary for TRAIT_BODY_VALUE.
- Add ENTITY_TYPE_NAME production.
- Updates name of some productions to be consistent with grammar.
- Fixed a few places where TOKEN was parsed as IDENTIFIER.
mtdowling pushed a commit that referenced this pull request Jun 19, 2023
@milesziemer @mtdowling
Assert trees are valid, fill in missing tests (#1808)
c394686 
All TreeTypeTests now expect the entire tree to be valid. This
helps us find ambiguity and inconsistencies in the parser for
cases when parsing a child tree is invalid only due to the
context of the parent. Fixes minor bugs that caused trees to be
invalid:
- enumMember test didn't have a BR after VALUE_ASSIGNMENT

Fills in missing tests for:
- Operations
- Block apply statements
- Entity type name
- Aggregate type name

Other minor changes and improvements:
- Adds util methods for parsing optional for-resource, mixins,
and value assignment productions.
- Adds util method for parsing any shape type and name.
- Updates peekPastSpaces to not skip the current token, in case
it is not SP. This simplifies parsing value assignments, and has
more intuitive behavior.
- Removes peekPastWs, which was only used for TRAIT_BODY_VALUE,
inlining the implementation. peekPastWs skipped the current token
like peekPastSpaces, which is necessary for TRAIT_BODY_VALUE.
- Add ENTITY_TYPE_NAME production.
- Updates name of some productions to be consistent with grammar.
- Fixed a few places where TOKEN was parsed as IDENTIFIER.
syall pushed a commit to Xtansia/smithy that referenced this pull request Aug 11, 2023
@milesziemer
Assert trees are valid, fill in missing tests (smithy-lang#1808)
d898d37 
All TreeTypeTests now expect the entire tree to be valid. This
helps us find ambiguity and inconsistencies in the parser for
cases when parsing a child tree is invalid only due to the
context of the parent. Fixes minor bugs that caused trees to be
invalid:
- enumMember test didn't have a BR after VALUE_ASSIGNMENT

Fills in missing tests for:
- Operations
- Block apply statements
- Entity type name
- Aggregate type name

Other minor changes and improvements:
- Adds util methods for parsing optional for-resource, mixins,
and value assignment productions.
- Adds util method for parsing any shape type and name.
- Updates peekPastSpaces to not skip the current token, in case
it is not SP. This simplifies parsing value assignments, and has
more intuitive behavior.
- Removes peekPastWs, which was only used for TRAIT_BODY_VALUE,
inlining the implementation. peekPastWs skipped the current token
like peekPastSpaces, which is necessary for TRAIT_BODY_VALUE.
- Add ENTITY_TYPE_NAME production.
- Updates name of some productions to be consistent with grammar.
- Fixed a few places where TOKEN was parsed as IDENTIFIER.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtdowling mtdowling mtdowling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@milesziemer @mtdowling