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

Take into account negative impls in "trait item not found" suggestions #79790

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

LeSeulArtichaut
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut commented Dec 7, 2020

This removes the suggestion to implement a trait for a type when that type already has a negative implementation for the trait, and replaces it with a note to point out that the trait is explicitely unimplemented, as suggested by @scottmcm.

Helps with #79683.

r? @scottmcm do you want to review this?

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2020
Comment on lines 1178 to 1181
let (potential_candidates, explicitly_negative) = if param_type.is_some() {
// Negative bounds are not supported
(candidates, Vec::new())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually sort of a shortcut, consider the following code:

#![feature(negative_impls)]

trait Foo {
    fn foo(&self);
}

trait Bar {}

impl<T: Foo> !Bar for T {}

fn foo<T: Bar>(x: T) {
    x.foo()
}

Which gives the following error:

error[E0599]: no method named `foo` found for type parameter `T` in the current scope
  --> main.rs:12:7
   |
12 |     x.foo()
   |       ^^^ method not found in `T`
   |
   = help: items from traits can only be used if the type parameter is bounded by the trait
help: the following trait defines an item `foo`, perhaps you need to restrict type parameter `T` with it:
   |
11 | fn foo<T: Foo + Bar>(x: T) {
   |        ^^^^^^^^

error: aborting due to previous error

So I think this could be improved but I think it's reasonable to do it in a follow-up PR since this PR does not change the current diagnostic.

@LeSeulArtichaut
Copy link
Contributor Author

My PR also doesn't work in:

#![feature(negative_impls)]

trait Bar {
    fn bar(&self);
}

impl<T: Copy> !Bar for T {}

fn foo(x: ()) {
    x.bar()
}

fn main() {}
error[E0599]: no method named `bar` found for unit type `()` in the current scope
  --> main.rs:10:7
   |
10 |     x.bar()
   |       ^^^ method not found in `()`
   |
   = help: items from traits can only be used if the trait is implemented and in scope
note: `Bar` defines an item `bar`, perhaps you need to implement it
  --> test.rs:3:1
   |
3  | trait Bar {

@LeSeulArtichaut LeSeulArtichaut added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. F-negative_impls #![feature(negative_impls)] S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2020
@LeSeulArtichaut
Copy link
Contributor Author

r? @lcnr

@rust-highfive rust-highfive assigned lcnr and unassigned scottmcm Dec 8, 2020
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

lgtm

one additional small nit would be to use

match explicitly_negative/potential_candidates {
    [] => {},
    [trait_info] => ...
    slice => ...
}

instead of the current if not_empty { if let ... { ... } }

compiler/rustc_typeck/src/check/method/suggest.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/method/suggest.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented Dec 16, 2020

r=me

@bors delegate+

@bors
Copy link
Contributor

bors commented Dec 16, 2020

✌️ @LeSeulArtichaut can now approve this pull request

@LeSeulArtichaut
Copy link
Contributor Author

Thanks for your review @lcnr!
@bors r=lcnr

@bors
Copy link
Contributor

bors commented Dec 16, 2020

📌 Commit cfc38d2 has been approved by lcnr

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 16, 2020
Take into account negative impls in "trait item not found" suggestions

This removes the suggestion to implement a trait for a type when that type already has a negative implementation for the trait, and replaces it with a note to point out that the trait is explicitely unimplemented, as suggested by `@scottmcm.`

Helps with rust-lang#79683.

r? `@scottmcm` do you want to review this?
@bors
Copy link
Contributor

bors commented Dec 16, 2020

⌛ Testing commit cfc38d2 with merge 0d7c7ea307a01cf5e08d93bca34e168d5e122e9e...

@Dylan-DPC-zz
Copy link

failed in rollup log

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 16, 2020
@LeSeulArtichaut
Copy link
Contributor Author

@Dylan-DPC How is this PR causing that "could not parse code block as Rust code" error?

@bors
Copy link
Contributor

bors commented Dec 16, 2020

☀️ Try build successful - checks-actions
Build commit: 0d7c7ea307a01cf5e08d93bca34e168d5e122e9e (0d7c7ea307a01cf5e08d93bca34e168d5e122e9e)

@LeSeulArtichaut
Copy link
Contributor Author

I don't know where this try build comes from, but based on this I will take the liberty of accepting this again.
@bors r=lcnr

@bors
Copy link
Contributor

bors commented Dec 16, 2020

📌 Commit cfc38d2 has been approved by lcnr

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2020
@Dylan-DPC-zz
Copy link

I don't know but the error was tough to debug and this looked like t he closest PR :D

@LeSeulArtichaut
Copy link
Contributor Author

I'd guess the failure comes from #79816 which IIUC creates this new lint?

@Dylan-DPC-zz
Copy link

yeah the test failure now on that PR, confirms it. sorry about the mess :P

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 16, 2020
Take into account negative impls in "trait item not found" suggestions

This removes the suggestion to implement a trait for a type when that type already has a negative implementation for the trait, and replaces it with a note to point out that the trait is explicitely unimplemented, as suggested by `@scottmcm.`

Helps with rust-lang#79683.

r? `@scottmcm` do you want to review this?
@bors
Copy link
Contributor

bors commented Dec 17, 2020

⌛ Testing commit cfc38d2 with merge a6491be...

@bors
Copy link
Contributor

bors commented Dec 17, 2020

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing a6491be to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 17, 2020
@bors bors merged commit a6491be into rust-lang:master Dec 17, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 17, 2020
@LeSeulArtichaut LeSeulArtichaut deleted the issue-79683 branch December 17, 2020 06:03
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 C-enhancement Category: An issue proposing an enhancement or a PR with one. F-negative_impls #![feature(negative_impls)] merged-by-bors This PR was explicitly merged by bors. 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.

8 participants