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

Explain why Self is invalid in generic parameters #90022

Merged
merged 1 commit into from
Dec 5, 2021

Conversation

hkmatsumoto
Copy link
Member

Close #89985.

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2021
@hkmatsumoto hkmatsumoto added the A-diagnostics Area: Messages for errors, warnings, and lints label Oct 18, 2021
@hkmatsumoto hkmatsumoto force-pushed the self-upper-as-generic-parameter branch from 3127f9b to cc572c7 Compare October 18, 2021 16:40
@hkmatsumoto hkmatsumoto added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 18, 2021
.emit();

// FIXME - try to conitnue parsing other generics?
return Ok((None, TrailingToken::None));
Copy link
Contributor

Choose a reason for hiding this comment

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

This should likely be something like:

    Ok(GenericParam {
            ident: Ident::new(kw::SelfUpper, this.prev_token.span),
            id: ast::DUMMY_NODE_ID,
            attrs: vec![],
            bounds: vec![], //maybe even parse/skip bounds?
            kind: GenericParamKind::Type { default: None },
            is_placeholder: false,
        })

so that we still take Self as a type param (although this might actually bring worse diagnostics if valid uses of Self are seen as the param 🤔).

Copy link
Member Author

Choose a reason for hiding this comment

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

(although this might actually bring worse diagnostics if valid uses of Self are seen as the param thinking).

I wonder if there could be a valid use of Self as a type parameter. Yes, one could write let a: Option<Self> = ... in assoc items, but that is a generic argument and will be parsed by a function other than parse_generic_params.

... so that we still take Self as a type param

Recovering by treating as if Self is a valid ident emits these diagnostics below, which I'm not very satisfied with; given Self is unusable here, parameter `Self` is never used sounds like we should use parameter Self, which isn't right.

error: unexpected keyword `Self` in generic parameters
  --> $DIR/keyword-self-as-type-param.rs:3:12
   |
LL | struct Foo<Self>(Self);
   |            ^^^^
   |
   = note: you cannot use `Self` as a generic parameter because it is reserved for associated items

error[E0392]: parameter `Self` is never used
  --> $DIR/keyword-self-as-type-param.rs:3:12
   |
LL | struct Foo<Self>(Self);
   |            ^^^^ unused parameter

The best-ish solution I come up with is that, when we see Self, emit the diagnostic and skip Self and the following comma as if the generic param never existed.

@hkmatsumoto hkmatsumoto force-pushed the self-upper-as-generic-parameter branch 2 times, most recently from e39e41e to d0a35c2 Compare October 19, 2021 13:29
@rust-log-analyzer

This comment has been minimized.

@hkmatsumoto hkmatsumoto force-pushed the self-upper-as-generic-parameter branch from d0a35c2 to a72dd4a Compare October 19, 2021 14:03
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2021
@jackh726
Copy link
Member

jackh726 commented Dec 4, 2021

@bors r+

This seems reasonable.

@bors
Copy link
Contributor

bors commented Dec 4, 2021

📌 Commit a72dd4a has been approved by jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 4, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2021
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#89642 (environ on macos uses directly libc which has the correct signature.)
 - rust-lang#90022 (Explain why `Self` is invalid in generic parameters)
 - rust-lang#90023 (Postpone the evaluation of constant expressions that depend on inference variables)
 - rust-lang#91215 (Implement VecDeque::retain_mut)
 - rust-lang#91355 (std: Stabilize the `thread_local_const_init` feature)
 - rust-lang#91528 (LLVM support .insn directive)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 29fe57d into rust-lang:master Dec 5, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 5, 2021
@hkmatsumoto hkmatsumoto deleted the self-upper-as-generic-parameter branch December 5, 2021 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add note on attempts of using Self as a normal generic arg
8 participants