Skip to content
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.

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

Ensure parses of invalid inputs represent all tokens #3860

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Nov 8, 2022

Pull Request Description

Ensure all tokens from the input are represented in trees resulting from invalid inputs--tests now cover every reachable code line that creates an Invalid node. (Also implemented stricter validation, mainly of import/export statements.)

See: https://www.pivotaltracker.com/story/show/183405907

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@kazcw kazcw self-assigned this Nov 8, 2022
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 8, 2022
@kazcw kazcw marked this pull request as ready for review November 8, 2022 23:17
@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Nov 8, 2022
@mergify mergify bot merged commit e8f3ad3 into develop Nov 9, 2022
@mergify mergify bot deleted the wip/kw/parser/syntax-error-tokens branch November 9, 2022 02:57
test_invalid("polyglot java import");
test_invalid("from import all");
test_invalid("from Foo import all hiding");
test_invalid("from Foo import all hiding X.Y");
Copy link
Member

@JaroslavTulach JaroslavTulach Nov 9, 2022

Choose a reason for hiding this comment

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

Handling the previous tests by b8f9124 - the following tests are rather malformed_export ones, btw.

However there is no IR.Error.Syntax.InvalidExport, just InvalidImport, so I am using it in: 6df12fa as well. Probably using such reason may not matter much as it is mostly an internal object not shown to the user.

However the error messages are still "Imports must have a valid path." and if shown to users, they may get confused by that.

test_invalid("foreign 4 * 4");
test_invalid("foreign foo = \"4\"");
test_invalid("foreign js foo = 4");
}
Copy link
Member

Choose a reason for hiding this comment

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

mergify bot pushed a commit that referenced this pull request Nov 10, 2022
This PR mimics test cases from #3860 and makes sure `IR.Syntax.Error` is constructed at appropriate places rather than just yielding an `UnhandledEntity` exception.

# Important Notes
Merge before #3611 to minimize disruption when changing the parser.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants