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

v2: modest parser improvements #934

Merged
merged 7 commits into from
Feb 14, 2024
Merged

Conversation

sbillig
Copy link
Collaborator

@sbillig sbillig commented Sep 25, 2023

  • Require comma between enum variants (and allow single-line enum def)
  • Add SyntaxKind for function signature
  • Add SyntaxKind for trait "signature" (the part of a trait definition before {)
  • Add SyntaxKind for trait impl "signature"
    ... ?

@sbillig
Copy link
Collaborator Author

sbillig commented Dec 8, 2023

@Y-Nak I'm inclined to merge these few simple changes as-is.

I'm currently experimenting with the trait TryParse idea and converting parse errors to enums, but will do that in a separate PR.

@sbillig
Copy link
Collaborator Author

sbillig commented Dec 9, 2023

By "as-is" I don't mean that I won't make requested changes, of course, just that I'll do further parser restructuring separately.

Comment on lines 545 to 557
loop {
match parser.current_kind() {
Some(kind) if kind.is_modifier_head() => {
if allow_modifier {
let (_, modifier_checkpoint) =
parser.parse(ItemModifierScope::default(), None);
checkpoint.get_or_insert(modifier_checkpoint);
} else {
parser
.unexpected_token_error("modifier is not allowed in this block", None);
}
}
_ => break,
Copy link
Member

Choose a reason for hiding this comment

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

This loop will introduce a false negative when a modifier is duplicated. e.g.,

pub pub fn foo()

/// Proceeds the parser to the recovery token of the current scope.
pub fn recover(&mut self) {
/// Consumes tokens until a recovery token is found, and reports an error on any
/// unexpexted tokens.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// unexpexted tokens.
/// unexpected tokens.

typo

@Y-Nak
Copy link
Member

Y-Nak commented Jan 5, 2024

I left two nits. I feel it'd be better to continue the discussion on #972. Please feel free to merge the PR after the nits are resolved.

@sbillig
Copy link
Collaborator Author

sbillig commented Jan 27, 2024

@Y-Nak I decided to apply one more change to this PR. Previously, x > > 1 and x + = 1 would be accepted as >> and += operations; I think it's better to require that there be no whitespace in the operator, and I think my implementation is the least-bad option to solve this. (The other obvious option is to add +=, >>, etc as lexer tokens and allow splitting >> tokens into two)

@sbillig sbillig merged commit 574c1f0 into ethereum:fe-v2 Feb 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants