[v2] feat(diagnostics): Improve unexpected terminal diagnostic on identifiers#1848
[v2] feat(diagnostics): Improve unexpected terminal diagnostic on identifiers#1848teofr wants to merge 2 commits into
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR improves Solidity v2 parser diagnostics by reducing noise in UnexpectedTerminal expectation lists when unreserved keywords are likely being treated as identifiers, and updates diagnostics snapshot tests accordingly.
Changes:
- Adjusts parsing error expectation conversion to drop certain unreserved keyword terminals when
Identifieris among the expected terminals. - Adds/updates diagnostics snapshot test cases for
unexpected_terminal(including a new “expecting_identifier” scenario) and refreshes related expected outputs. - Updates the generated Rust test harness to run the split
unexpected_terminalsnapshot suites.
Reviewed changes
Copilot reviewed 3 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/solidity-v2/outputs/cargo/parser/src/parser/mod.rs | Implements the heuristic that filters unreserved keyword terminals out of expectation sets when Identifier is expected. |
| crates/solidity-v2/outputs/cargo/tests/src/diagnostics_output/snapshots.generated.rs | Updates generated snapshot test registration to run the new unexpected_terminal/* sub-suites. |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_terminal/semicolon/input.sol | Adds semicolon-based unexpected-terminal snapshot input. |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_terminal/semicolon/generated/solc/0.8.0-failure.txt | Adds expected solc diagnostic output snapshot for the new semicolon case. |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_terminal/semicolon/generated/slang/0.8.0-failure.txt | Adds expected slang diagnostic output snapshot for the new semicolon case. |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_terminal/expecting_identifier/input.sol | Adds new snapshot input targeting the “keyword-as-identifier” ambiguity (public used as a name). |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_terminal/expecting_identifier/generated/solc/0.8.0-failure.txt | Adds expected solc diagnostic output snapshot for the new “expecting_identifier” case. |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_terminal/expecting_identifier/generated/slang/0.8.0-failure.txt | Adds expected slang diagnostic output snapshot for the new “expecting_identifier” case. |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_terminal/generated/slang/0.8.0-failure.txt | Removes the prior combined unexpected-terminal slang snapshot output in favor of the split test directories. |
| crates/solidity-v2/testing/snapshots/diagnostics_output/syntax/unexpected_eof/generated/slang/0.8.0-failure.txt | Updates expected slang unexpected-EOF snapshot output (reflecting the filtered expectation set). |
| if expected.contains(&TerminalKind::Identifier) { | ||
| expected.remove(&TerminalKind::AtKeyword); | ||
| expected.remove(&TerminalKind::ErrorKeyword); | ||
| expected.remove(&TerminalKind::FromKeyword); | ||
| expected.remove(&TerminalKind::GlobalKeyword); |
|
Bencher Report| Branch | teofr/improve-identifier-diagnostic |
| Testbed | ci |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
teofr
force-pushed
the
teofr/improve-identifier-diagnostic
branch
from
5ce020f to
dff2b3f
Compare
ggiraldez
left a comment
ggiraldez
left a comment
There was a problem hiding this comment.
I think the change is good, but I have one minor concern with maintainability.
| expected.remove(&TerminalKind::AtKeyword); | ||
| expected.remove(&TerminalKind::ErrorKeyword); | ||
| expected.remove(&TerminalKind::FromKeyword); | ||
| expected.remove(&TerminalKind::GlobalKeyword); | ||
| expected.remove(&TerminalKind::LayoutKeyword); | ||
| expected.remove(&TerminalKind::RevertKeyword); | ||
| expected.remove(&TerminalKind::TransientKeyword); |
There was a problem hiding this comment.
Would it be possible to generate this list of unreserved keywords from the language definition? This is bound to get out of sync with it as the language evolves.
From a quick look at the templates, it should be easy to generate TerminalKind::is_unreserved_keyword() and then run
expected.retain(|t| !t.is_unreserved_keyword());
Certain keywords can be parsed as either the keyword itself, or as an identifier.
This PR tries to improve on the
UnexpectedTerminaldiagnostic when these keywords are likely expected only on their capacity as identifiers.This is a heuristic, so (as pointed by Copilot below) in some cases omitting certain terminals is not perfect, but I think it's worth it in the general case.
Check the difference on the newly added test between the first and second commit for an example.