generalize "incoherent impls" impl for user defined types

[lcnr]

To allow the move of trait Error into core.

continues the work from #94963, finishes rust-lang/compiler-team#487

r? @petrochenkov cc @yaahc

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[lcnr]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 03113b4344ffb6d8525f324973b749e1c8de50a9 with merge 6dd2f7a9b86fcbc5879bd666c301ed939021bc4f...

[jackh726]

Is there a reason (other than maybe perf) to not just always assemble potentially incoherent impls, instead of adding a new rustc_has_incoherent_inherent_impls? I guess it allows us to "lock down" the types a bit more too - but these are unstable attributes anyways.

Three other thoughts:

Should rustc_allow_incoherent_impl be on the impl itself? Seems better than writing on every function.

In your previous PR, you added rustc_coherence_is_core. Why not just use the same mechanism of rustc_allow_incoherent_impl?

This somewhat reminds me of some of the discussions around arbitrary self types (#44874) and whether those methods get registered on the "Self" type, even if that type isn't a type you control (like Box<Foo>).

[bors]

☀️ Try build successful - checks-actions
Build commit: 6dd2f7a9b86fcbc5879bd666c301ed939021bc4f (6dd2f7a9b86fcbc5879bd666c301ed939021bc4f)

[rust-timer]

Queued 6dd2f7a9b86fcbc5879bd666c301ed939021bc4f with parent 3bfeffd, future comparison URL.

[rust-timer]

Finished benchmarking commit (6dd2f7a9b86fcbc5879bd666c301ed939021bc4f): comparison url.

Summary:

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	0	1	0	1
mean2	N/A	N/A	-0.4%	N/A	-0.4%
max	N/A	N/A	-0.4%	N/A	-0.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩

[lcnr]

Is there a reason (other than maybe perf) to not just always assemble potentially incoherent impls, instead of adding a new rustc_has_incoherent_inherent_impls? I guess it allows us to "lock down" the types a bit more too - but these are unstable attributes anyways.

Yeah, I want to "lock down" which types can have incoherent impls. It doesn't matter too much as the impls also have to be explicitly mention that they are incoherent, but 🤷 considering that adding this attribute is pretty trivial it seems worth it to me.

Three other thoughts:

Should rustc_allow_incoherent_impl be on the impl itself? Seems better than writing on every function.

I mentioned that in #94963 (comment). The idea is that adding incoherent impls should only be a last resort and avoided if possible. Having the attribute on assoc items instead of whole impls prevents people from accidentally adding functions to existing impls. E.g. the f32 impl in alloc instead of core.

In your previous PR, you added rustc_coherence_is_core. Why not just use the same mechanism of rustc_allow_incoherent_impl?

don't fully get what you mean here? Why we don't use rustc_allow_incoherent_impl in core as well? Also from #94963 (comment): core should be thought of as owning builtin types, so adding inherent impls for them there is absolutely fine and shouldn't require some additional mental overhead.

[petrochenkov]

I also think #[rustc_has_incoherent_inherent_impls] is redundant, even given the arguments from #94963 (comment).

It may be potentially useful for performance though, due to skipping assemble_inherent_candidates_for_incoherent_tys for all other types.
If think if a perf run show no difference then it can be removed.

[lcnr]

hmm, I considered removing it after your comment, but it feels pretty valuable to me to immediately know which types have incoherent inherent impls, because these types are somewhat special from my pov.

I don't expect there to be a perf difference depending on our approach here, considering that assemble_inherent_candidates_for_incoherent_ty is also trivial.

@rust-lang/libs-api given that you have some type/trait object which needs incoherent inherent impls what's your preferred solution here:

Solution 1

do not mark the types in any way, mark all incoherent impls with #[rustc_allow_incoherent_impl].

Looking at the library changes in this PR, this would amount to removing the #[lang = "CStr"] attribute from struct CStr without adding any replacement. So we would not have to add the #[rustc_has_incoherent_inherent_impls] attribute.

Solution 2

Add an attribute to types and traits which may have incoherent impls,mark all incoherent impls with #[rustc_allow_incoherent_impl].

This is currently implemented in this PR and is pretty verbose.

[yaahc]

@lcnr the most important property is that it be hard / impossible to accidentally add any new incoherent impls. I can't really think of any realistic situations where the attribute on the type would help prevent a mistake. The best I can come up with is someone mistakenly believing that a type already allows incoherent impls, and so allowing a new incoherent impl on the basis of that, but not actually checking the original type.

That said, I think I'd prefer the verbosity still, if only because it is just one more barrier making it more annoying to add incoherent impls, which we already are fairly grumpy about needing in the first place.

[yaahc]

@lcnr I brought this up with the libs-api team during our meeting today and they agreed with my conclusion, we'd prefer the more verbose option, if we later have a good justification for simplify it we can do so then, but for now we want to default to a higher barrier to entry for using this feature, with the side benefit of improved search-ability being appreciated.

[lcnr]

I still want to change the help messages depending on whether rustc_attrs is enabled or not.

Think that the current state is pretty good, don't think there's a much better way to write this which keeps the different error messages depending on rustc_attrs.

@rustbot ready

[petrochenkov]

to write this which keeps the different error messages depending on rustc_attrs

That's exactly what I wanted to avoid though.
#[feature] attributes are not supposed to change any behavior, only to enable/disable stability errors.
So if I see anything other than a feature_err(...) inside a tcx.features().something condition, it immediately rings a bell.

[petrochenkov]

@bors r+
rustc_typeck is not a crate that I care about enough to argue about #96520 (comment).

[bors]

📌 Commit 321162b has been approved by petrochenkov

[bors]

⌛ Testing commit 321162b with merge 3a79d200fc88150033665314bfa213e641dd989b...

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[jackh726]

#[feature] attributes are not supposed to change any behavior, only to enable/disable stability errors. So if I see anything other than a feature_err(...) inside a tcx.features().something condition, it immediately rings a bell.

@petrochenkov I understand your reasoning here, but there are already a number of features that don't follow these. Off the top of my head, I can name nll, generic_associated_types_extended, and type_alias_impl_trait (I think). I also think that this difference is more justified than any of those features given that rustc_attrs is perma-unstable and we really don't want to be suggesting it (though the suggestion is nice for when it is enabled internally). Just my 2 cents.

[bors]

⌛ Testing commit 321162b with merge 74cea9f...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 74cea9f to master...

[rust-timer]

Finished benchmarking commit (74cea9f): comparison url.

Summary:

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regression found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	1	0	0	0
mean2	N/A	1.4%	N/A	N/A	N/A
max	N/A	1.4%	N/A	N/A	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩