Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat: unknown statements #1794

Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Nov 17, 2021

Summary

This PR does the following:

  • created a new struct called ParseRecover where information the information needed for recovery are created passed to the parser;
  • unified the functions err_recover and err_recover_no_err in one single function;
  • the recovery logic is now encapsulated inside ParseRecover and the recovery logic is inside the recover function;
  • created new test cases to track errors that were not tracked before;
  • removed ERROR and added JS_UNKNOWN_STATEMENT in some particular case, specifically the parser has a function called stmt() that is called in those functions that tries to parse particular statements: if/else, do/while, while, for, etc., so every time this function fails, we emit an error;

Test Plan

cargo lint
cargo format
cargo test

@cloudflare-pages
Copy link

cloudflare-pages bot commented Nov 17, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7dc5762
Status: ✅  Deploy successful!
Preview URL: https://f9e6a741.tools-8rn.pages.dev

View logs

@yassere
Copy link
Contributor

yassere commented Nov 17, 2021

What’s the advantage of combining these parameters into a RecoveryBag struct? It feels like it’s adding more boilerplate at the error recovery call sites in the parser.

crates/rslint_parser/src/recovery_bag.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/recovery_bag.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/recovery_bag.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/recovery_bag.rs Outdated Show resolved Hide resolved
@ematipico
Copy link
Contributor Author

ematipico commented Nov 17, 2021

What’s the advantage of combining these parameters into a RecoveryBag struct? It feels like it’s adding more boilerplate at the error recovery call sites in the parser.

The parameters of the functions we use to recover from errors were becoming too many. Micha suggested to revisit it with a struct and I agree too. Also, it allows us to unify some logic in one single place and make the API better documented.

@ematipico ematipico force-pushed the feature/unknown-statement-nodes branch from 85fd6dd to e04b039 Compare November 17, 2021 17:22
crates/rslint_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I like the RecoveryBag and overall great improvements!

There are some cases where we now insert invalid children (e.g. expressions instead of statements).

Would you mind listing in the PR summary for parser rules your changing the error recovery?

crates/rslint_parser/src/parser.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/recovery_bag.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/recovery_bag.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/syntax/stmt.rs Outdated Show resolved Hide resolved
@ematipico ematipico force-pushed the feature/unknown-statement-nodes branch from e0abc47 to d3196eb Compare November 18, 2021 13:17
@github-actions
Copy link

github-actions bot commented Nov 18, 2021

Test262 comparison coverage results on windows-latest

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16771 16771 0
Failed 836 836 0
Panics 1 1 0
Coverage 95.25% 95.25% 0.00%

@ematipico
Copy link
Contributor Author

I reverted the changes I did around the parsing of switch. It fixed a infinite recursion inside the parser but it lowered the coverage, meaning that we'd need to spend more time around it.

@ematipico
Copy link
Contributor Author

There are some cases where we now insert invalid children (e.g. expressions instead of statements).

Errors, now fixed.

Would you mind listing in the PR summary for parser rules your changing the error recovery?

Done

crates/rslint_parser/src/parse_recoverer.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/parse_recoverer.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/parser.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Looks good. Please take a look at the comments from @yassere before merging.

@ematipico ematipico merged commit 15072f0 into feature/unknown-nodes-errors Nov 19, 2021
@ematipico ematipico deleted the feature/unknown-statement-nodes branch November 19, 2021 12:59
ematipico added a commit that referenced this pull request Nov 19, 2021
ematipico added a commit that referenced this pull request Nov 22, 2021
ematipico added a commit that referenced this pull request Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants