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

Reject generic self types using impl defid: do not merge. #130120

Conversation

adetaylor
Copy link
Contributor

The RFC for arbitrary self types v2 declares that we should reject "generic" self types. This commit does so.

The definition of "generic" was unclear in the RFC, but has been explored in
#129147
and the conclusion is that "generic" means any self type which is a type parameter defined on the method itself, or references to such a type.

This approach was chosen because other definitions of "generic" don't work. Specifically,

  • we can't filter out generic type arguments, because that would filter out Rc and all the other types of smart pointer we want to support;
  • we can't filter out all type params, because Self itself is a type param, and because existing Rust code depends on other type params declared on the type (as opposed to the method).

This PR is a second attempt at achieving this, based on producing a new well-formedness checking context which is only aware of the type params of the impl block, not of the method itself.

It doesn't currently work because it turns out we do need some method params under some circumstances, but raising it for completeness.

This PR adds lots of extra tests to arbitrary-self-from-method-substs. Most of these are ways to trigger a "type mismatch" error which

// FIXME(arbitrary_self_types): We probably should limit the
hopes can be minimized by filtering out generics in this way. We remove a FIXME from confirm.rs suggesting that we make this change. It's still possible to cause type mismatch errors, and a subsequent PR may be able to improve diagnostics in this area, but it's harder to cause these errors without contrived uses of the turbofish.

This is a part of the arbitrary self types v2 project, rust-lang/rfcs#3519
#44874

r? @wesleywiser

The RFC for arbitrary self types v2 declares that we should reject
"generic" self types. This commit does so.

The definition of "generic" was unclear in the RFC, but has been
explored in
rust-lang#129147
and the conclusion is that "generic" means any `self` type which
is a type parameter defined on the method itself, or references
to such a type.

This approach was chosen because other definitions of "generic"
don't work. Specifically,
* we can't filter out generic type _arguments_, because that would
  filter out Rc<Self> and all the other types of smart pointer
  we want to support;
* we can't filter out all type params, because Self itself is a
  type param, and because existing Rust code depends on other
  type params declared on the type (as opposed to the method).

This PR is a second attempt at achieving this, based on producing a new
well-formedness checking context which is only aware of the type params
of the impl block, not of the method itself.

It doesn't currently work because it turns out we do need some method
params under some circumstances, but raising it for completeness.

This PR adds lots of extra tests to arbitrary-self-from-method-substs.
Most of these are ways to trigger a "type mismatch" error which
https://github.com/rust-lang/rust/blob/9b82580c7347f800c2550e6719e4218a60a80b28/compiler/rustc_hir_typeck/src/method/confirm.rs#L519
hopes can be minimized by filtering out generics in this way.
We remove a FIXME from confirm.rs suggesting that we make this change.
It's still possible to cause type mismatch errors, and a subsequent
PR may be able to improve diagnostics in this area, but it's harder
to cause these errors without contrived uses of the turbofish.

This is a part of the arbitrary self types v2 project,
rust-lang/rfcs#3519
rust-lang#44874

r? @wesleywiser
@adetaylor
Copy link
Contributor Author

@rustbot label +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 8, 2024
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 8, 2024
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling cfg-if v1.0.0
   Compiling adler v1.0.2
   Compiling rustc-demangle v0.1.24
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error[E0658]: `Box<Node<T>, A>` cannot be used as the type of `self` without the `arbitrary_self_types` feature
    |
    |
160 |     fn into_element<A: Allocator>(self: Box<Self, A>) -> T {
    |
    = note: see issue #44874 <https://github.com/rust-lang/rust/issues/44874> for more information
    = help: add `#![feature(arbitrary_self_types)]` to the crate attributes to enable
    = note: this compiler was built on 2024-09-08; consider upgrading it if it is out of date
    = note: this compiler was built on 2024-09-08; consider upgrading it if it is out of date
    = help: consider changing to `self`, `&self`, `&mut self`, `self: Box<Self>`, `self: Rc<Self>`, `self: Arc<Self>`, or `self: Pin<P>` (where P is one of the previous types except `Self`)

error[E0658]: `Box<[T], A>` cannot be used as the type of `self` without the `arbitrary_self_types` feature
    |
    |
497 |     pub fn into_vec<A: Allocator>(self: Box<Self, A>) -> Vec<T, A> {
    |
    = note: see issue #44874 <https://github.com/rust-lang/rust/issues/44874> for more information
    = help: add `#![feature(arbitrary_self_types)]` to the crate attributes to enable
    = note: this compiler was built on 2024-09-08; consider upgrading it if it is out of date
    = note: this compiler was built on 2024-09-08; consider upgrading it if it is out of date
    = help: consider changing to `self`, `&self`, `&mut self`, `self: Box<Self>`, `self: Rc<Self>`, `self: Arc<Self>`, or `self: Pin<P>` (where P is one of the previous types except `Self`)
For more information about this error, try `rustc --explain E0658`.
error: could not compile `alloc` (lib) due to 2 previous errors
Build completed unsuccessfully in 0:05:16
  local time: Sun Sep  8 22:03:52 UTC 2024

@fmease fmease marked this pull request as ready for review September 9, 2024 05:35
@compiler-errors compiler-errors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2024
@fmease fmease marked this pull request as draft September 11, 2024 09:47
@fmease
Copy link
Member

fmease commented Sep 11, 2024

(oops, I didn't mean to mark it as ready for review ^^)

@bors
Copy link
Contributor

bors commented Oct 23, 2024

☔ The latest upstream changes (presumably #132027) made this pull request unmergeable. Please resolve the merge conflicts.

@wesleywiser
Copy link
Member

@adetaylor Should I still review this or has this been superseded by #130098?

@adetaylor
Copy link
Contributor Author

If you are happy with the approach in #130098 then yes I think this is probably irrelevant now (the pros and cons were discussed here).

@adetaylor adetaylor closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

7 participants