Skip to content

rust-analyzer: fix parsing of trait bound polarity and for-binders #145199

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

Closed
wants to merge 1 commit into from

Conversation

npmccallum
Copy link
Contributor

The rustc AST allows both for<> binders and ? polarity modifiers in trait bounds, but they are parsed in a specific order and validated for correctness:

1. `for<>` binder is parsed first.
2. Polarity modifiers (`?`, `!`) are parsed second.
3. The parser validates that binders and polarity modifiers do not conflict:

```rust
if let Some(binder_span) = binder_span {
    match modifiers.polarity {
        BoundPolarity::Maybe(polarity_span) => {
            // Error: "for<...> binder not allowed with ? polarity"
        }
    }
}
```

This implies:

  • for<> ?Sized → Valid syntax. Invalid semantics.
  • ?for<> Sized → Invalid syntax.

However, rust-analyzer incorrectly had special-case logic that allowed ?for<> as valid syntax. This fix removes that incorrect special case, making rust-analyzer reject ?for<> Sized as a syntax error, matching rustc behavior.

This has caused confusion in other crates (such as syn) which rely on these files to implement correct syntax evaluation.

The rustc AST allows both `for<>` binders and `?` polarity
modifiers in trait bounds, but they are parsed in a specific
order and validated for correctness:

    1. `for<>` binder is parsed first.
    2. Polarity modifiers (`?`, `!`) are parsed second.
    3. The parser validates that binders and polarity modifiers
       do not conflict:

    ```rust
    if let Some(binder_span) = binder_span {
        match modifiers.polarity {
            BoundPolarity::Maybe(polarity_span) => {
                // Error: "for<...> binder not allowed with ? polarity"
            }
        }
    }
    ```

This implies:

- `for<> ?Sized` → Valid syntax. Invalid semantics.
- `?for<> Sized` → Invalid syntax.

However, rust-analyzer incorrectly had special-case logic that
allowed `?for<>` as valid syntax. This fix removes that incorrect
special case, making rust-analyzer reject `?for<> Sized` as a
syntax error, matching rustc behavior.

This has caused confusion in other crates (such as syn) which
rely on these files to implement correct syntax evaluation.
@rustbot
Copy link
Collaborator

rustbot commented Aug 10, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 10, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

@jieyouxu
Copy link
Member

This is a r-a-only change, so please instead submit this PR against the r-a repository.

@npmccallum
Copy link
Contributor Author

This is a r-a-only change, so please instead submit this PR against the r-a repository.

I don't think this is true. I think my commit message just triggered the bot. This is a change to the parser crate which is hosted in this repo. I'm not sure the best way to improve my commit message.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 10, 2025

I don't think this is true. I think my commit message just triggered the bot. This is a change to the parser crate which is hosted in this repo. I'm not sure the best way to improve my commit message.

To clarify, rust-analyzer is a subtree of this repository, but it is not a git submodule (because that has annoying UX issue). It is in fact sync'd between this repo and rust-analyzer with https://github.com/rust-lang/josh-sync. You can see an example sync such as #144887.

Please refer to https://rustc-dev-guide.rust-lang.org/external-repos.html?highlight=josh#external-dependencies-subtrees.

I.e. the bot message is correct.

@jieyouxu
Copy link
Member

In any case, I'll roll a rust-analyzer reviewer, if the r-a reviewer is fine reviewing/accepting it on the r-l/r repo side then disregard the above.

r? rust-analyzer

@rustbot rustbot added the T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue. label Aug 10, 2025
@ChayimFriedman2
Copy link
Contributor

Please send rust-analyzer-only PRs to https://github.com/rust-lang/rust-analyzer/.

@npmccallum
Copy link
Contributor Author

rust-lang/rust-analyzer#20417

@npmccallum npmccallum closed this Aug 10, 2025
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants