Stream: t-compiler

Topic: #62194/#62696 - coherence "*stream" in private traits


Ariel Ben-Yehuda (Jul 30 2019 at 17:43, on Zulip):

Hi @Esteban Küber @nagisa

Ariel Ben-Yehuda (Jul 30 2019 at 17:43, on Zulip):

so my PR got broke by the changes in #62194, and I don't think the removal of the error makes things better

Ariel Ben-Yehuda (Jul 30 2019 at 17:44, on Zulip):

the fix is:

trait Private {
}

impl<T: Private> Send for T {
//~^ ERROR conflicting implementations of trait `std::marker::Send` for type `&_`
//~| note: downstream crates may implement trait `Private` for type `&_`
}

Where the note was removed if the trait is private

Ariel Ben-Yehuda (Jul 30 2019 at 17:44, on Zulip):

I think the error is more mysterious without the "semi-incorrect" note

Ariel Ben-Yehuda (Jul 30 2019 at 17:45, on Zulip):

plus, the PR (#62696) basically removed the notes from all coherence note tests, so now they don't actually test anything, which is definitely wrong

Ariel Ben-Yehuda (Jul 30 2019 at 18:08, on Zulip):

btw, @estebank, about the unwrap comment

Ariel Ben-Yehuda (Jul 30 2019 at 18:08, on Zulip):

You should use if let Some(id) = instead of unwrap(). Every unwrap is a potential ICE.

Ariel Ben-Yehuda (Jul 30 2019 at 18:09, on Zulip):

the code above does tcx.hir().as_local_hir_id(impl_def_id) several times, all of them with unwrap

Ariel Ben-Yehuda (Jul 30 2019 at 18:09, on Zulip):

therefore, it can't fail

Ariel Ben-Yehuda (Jul 30 2019 at 18:09, on Zulip):

adding if let Some(_) where the None case can't ever happen, with no comment that it can't ever happen, is bad practice

Ariel Ben-Yehuda (Jul 30 2019 at 18:14, on Zulip):

I would change the if impl_def_id.is_local() to be an if let Some(id) = tcx.hir().as_local_hir_id(impl_def_id). https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/coherence/builtin.rs#L80 does the same

Ariel Ben-Yehuda (Jul 30 2019 at 18:15, on Zulip):

or maybe do a let id = tcx.hir().as_local_hir_id(impl_def_id).unwrap(); just inside the if

Ariel Ben-Yehuda (Jul 30 2019 at 18:15, on Zulip):

depending on how I feel as_local_hir_id is normative for is_local

nagisa (Jul 30 2019 at 22:30, on Zulip):

/me shrugs. I’m strongly against compiler lying-but-not-really.

nagisa (Jul 30 2019 at 22:31, on Zulip):

plus, the PR (#62696) basically removed the notes from all coherence note tests, so now they don't actually test anything, which is definitely wrong

Hmm, I would not be surprised if the author did remove these by hand rather than by blessing.

Esteban Küber (Jul 30 2019 at 23:38, on Zulip):

/me shrugs. I’m strongly against compiler lying-but-not-really.

Could we reword the previous message to make it !lie?

nagisa (Jul 31 2019 at 00:21, on Zulip):

/me shrugs. I’m strongly against compiler lying-but-not-really.

Could we reword the previous message to make it !lie?

I would be fine with anything that does not say somebody could implement a private trait.

nagisa (Jul 31 2019 at 00:21, on Zulip):

I think this may actually be a limitation that we might be interested lifting in the future and there’s nothing really preventing this from working other than our implementation.

nagisa (Jul 31 2019 at 00:22, on Zulip):

If that is indeed true, perhaps wording the message in that sense would work

Ariel Ben-Yehuda (Aug 03 2019 at 13:28, on Zulip):

any ideas?

nagisa (Aug 03 2019 at 14:29, on Zulip):

Not really, no. We could revert the PR and reopen the issue with a note that we should think of an alternative wording for the message.

Ariel Ben-Yehuda (Aug 04 2019 at 16:28, on Zulip):

sounds like the best current idea to me. Any opinions @Esteban Küber ?

Esteban Küber (Aug 04 2019 at 16:51, on Zulip):

Sounds reasonable to me @Ariel Ben-Yehuda

Ariel Ben-Yehuda (Aug 04 2019 at 16:59, on Zulip):

@Esteban Küber https://github.com/rust-lang/rust/pull/63264

Last update: Nov 20 2019 at 01:15UTC