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

Negative trait impls show as "found implementations" in error messages #79458

Closed
scottmcm opened this issue Nov 27, 2020 · 7 comments · Fixed by #79606
Closed

Negative trait impls show as "found implementations" in error messages #79458

scottmcm opened this issue Nov 27, 2020 · 7 comments · Fixed by #79606
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. F-negative_impls #![feature(negative_impls)] T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

scottmcm commented Nov 27, 2020

(Spotted in https://users.rust-lang.org/t/the-trait-clone-is-not-implemented-for-mut-t/51989?u=scottmcm)

I tried this code https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=6862c5bd66f1fcdf55fd725a93baffcd:

#[derive(Clone)]
struct Foo<'a, T> {
    x: &'a mut T,
}

Which of course fails to compile, but it gives the following confusing help:

  = help: the following implementations were found:
            <&T as Clone>
            <&mut T as Clone>

The library now has impl<'_, T> !Clone for &'_ mut T, so it looks like this error-generation code path needs to be updated to account for that.

Ideally it'd take advantage of the explicit negative here to say that it's not clone and has been promised that it never will be, but at the very least is shouldn't be showing <&mut T as Clone> which makes it look like it's actually Clone.

cc #68318

@scottmcm scottmcm added C-bug Category: This is a bug. F-negative_impls #![feature(negative_impls)] labels Nov 27, 2020
@oli-obk oli-obk added the A-diagnostics Area: Messages for errors, warnings, and lints label Nov 27, 2020
@ThePuzzlemaker
Copy link
Contributor

@rustbot claim

@ThePuzzlemaker
Copy link
Contributor

ThePuzzlemaker commented Nov 27, 2020

While <&mut T as !Clone> (or just not showing it) makes sense to me, I feel that something like <&mut T as !Clone>, meaning that &mut T can never be Clone would be better to new users.

@ThePuzzlemaker
Copy link
Contributor

ThePuzzlemaker commented Nov 27, 2020

Actually, <&mut T as !Clone> would probably not be the best (or easiest to implement afaik) as it's not technically valid syntax, so trying to implement it in the pretty printer, as I originally tried, would break a few things. It'd be better if we either:

  • just ignore implementations with negative polarity or
  • use impl Trait for Ty syntax e.g. impl<'a> !Clone for &'a mut T and impl<'a> Clone for &'a T for the diagnostic.

@ThePuzzlemaker
Copy link
Contributor

Updating on this: I think that ignoring negative polarity implementations would be the best solution right now (especially as I'm still a new-ish contributor). In the future, it may be helpful to implement something like you suggested, but that would require a lot more changes to the compiler than is feasible for me to do right now.

@ThePuzzlemaker
Copy link
Contributor

ThePuzzlemaker commented Nov 29, 2020

I'm going to make some cleanups to my changes, then I'll draft a PR.
Update: looks like my changes caused quite a few tests to fail (which absolutely should not have happened), so I'm trying to re-implement it a different way that should hopefully not do that.

@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 30, 2020
@scottmcm
Copy link
Member Author

@ThePuzzlemaker Splitting it into two changes sounds good to me. Not showing the incorrect one is definitely the higher priority change.

@ThePuzzlemaker
Copy link
Contributor

ThePuzzlemaker commented Dec 1, 2020

Turns out some of those test failures actually were correct (as when this bug was introduced, someone must have run x.py test with --bless which unintentionally caused those failures), so I'm going to re-run tests again with my original changes then see which ones are valid (hopefully, all of them). After that, I'll add a regression test.

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-bug Category: This is a bug. F-negative_impls #![feature(negative_impls)] T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants