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

Implement PartialOrd for Cursor #1236

merged 3 commits into from Oct 20, 2022

Implement PartialOrd for Cursor #1236

merged 3 commits into from Oct 20, 2022


Copy link

@CAD97 CAD97 commented Oct 20, 2022

New version of #1196 now that #1223 has changed token buffers to a flat layout, making the comparison trivial to implement.

As additional motivation and since it was first discovered in #1196, fixes #1235. The issue has nothing to do with eq comparing the scope pointer; in fact, the involved pointers actually have the same scope. Properly fixing the issue requires the ability to compare cursors in order to detect the case where the verbatim end is inside of a None-delimited group we previously would have stepped over.

I removed the check for scope equality here, as it is much more useful to be able to be able to compare cursors which don't necessarily have the same scope end (e.g. to determine which of multiple speculative parses got the furthest). Additionally, we actually now rely on comparing two cursors at the same location with different scopes in verbatim::between treating them as equal, as we explicitly enter the None-delimited group which the end cursor implicitly entered.

If exposing this is undesirable, we can reintroduce the scope check to Cursor comparison (treating cursors not with the same scope as unordered and nonequal) and introduce a Cursor::position(&self) -> impl Ord which can be used to order cursor's position ignoring the scope. (This would also provide a nice place to document how cursors are ordered.)

I'm happy to implement + document that alternative approach if it's desired, but since just dropping the scope-matters in comparison is simpler, I'm providing this solution first.

Fixes dtolnay#1235. As seen in that issue, a syntax node can actually cross the
border of a None-delimited group, due to such groups being transparent
to the parser in most cases. In the cases that this can occur, the
presence of the group is known to be semantically irrelevant, so we just
explicitly ignore the presence of the group.

It's important that we only ignore None-delimited groups when the
verbatim end requires entering the group, as such groups can have semantic
impact (such as grouping binary operator chains) in other positions.
Copy link

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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


@dtolnay dtolnay merged commit 2b3f742 into dtolnay:master Oct 20, 2022
@CAD97 CAD97 deleted the cursor-cmp branch October 20, 2022 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

Parser crash in Item::Verbatim static when sliced by None-delimited group
2 participants