Warn for #[unstable] on trait impls when it has no effect. #76538

m-ou-se · 2020-09-09T20:35:17Z

Earlier today I sent a PR with an #[unstable] attribute on a trait impl, but was informed that this attribute has no effect there. (comment: #76525 (comment), issue: #55436)

This PR adds a warning for this situation. Trait impl blocks with #[unstable] where both the type and the trait are stable will result in a warning:

warning: An `#[unstable]` annotation here has no effect. See issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information.
   --> library/std/src/panic.rs:235:1
    |
235 | #[unstable(feature = "integer_atomics", issue = "32976")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It detects three problems in the existing code:

A few RefUnwindSafe implementations for the atomic integer types in library/std/src/panic.rs. Example:

rust/library/std/src/panic.rs

Lines 235 to 236 in d92155b

#[unstable(feature = "integer_atomics", issue = "32976")]

impl RefUnwindSafe for atomic::AtomicI8 {}

An implementation of Error for LayoutErr in library/std/srd/error.rs:

rust/library/std/src/error.rs

Lines 392 to 397 in d92155b

    
           #[unstable( 
        
               feature = "allocator_api", 
        
               reason = "the precise API and guarantees it provides may be tweaked.", 
        
               issue = "32838" 
        
           )] 
        
           impl Error for LayoutErr {}

From implementations for Waker and RawWaker in library/alloc/src/task.rs. Example:

rust/library/alloc/src/task.rs

Lines 36 to 37 in d92155b

    
           #[unstable(feature = "wake_trait", issue = "69912")] 
        
           impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for Waker {

Case 3 interesting: It has a bound with an #[unstable] trait (W: Wake), so appears to have not much effect on stable code. It does however break similar blanket implementations. It would also have immediate effect if Wake was implemented for any stable type. (Which is not the case right now, but there are no warnings in place to prevent it.) Whether this case is a problem or not is not clear to me. If it isn't, adding a simple c.visit_generics(..); to this PR will stop the warning for this case.

rust-highfive · 2020-09-09T20:35:21Z

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2020-09-09T20:55:37Z

I this PR exists, then stability checking has been broken at some point.
Stability checking is supposed to either require the #[unstable] attribute (if the item it's applied to is reachable), or produce an error (the attribute is not applicable to this item).

...unless #[unstable] is required, but it was already inherited from a parent module, so writing it on the item itself is not necessary.

Anyway, stability attributes on impls are actually required if not inherited, and very much make sense at least for documentation, and there were always plans to implement stability checking for impls some day, but it's hard and not top priority.

petrochenkov · 2020-09-09T20:58:03Z

I don't think complicating stability checking is the right trade off here, it already accumulated quite a bit of technical debt over the years.

m-ou-se · 2020-09-09T21:03:00Z

#[unstable] for impl Trait for Type blocks has never worked. (#55436)

It's easy to make the mistake of adding a #[unstable] trait impl that's effectively stable. This has happened by accident. It's also easy to forget to stabilize one when the relevant type or trait gets stabilized, as has happened for both LayoutErr and RefUnwindSafe. This warning prevents this. Seems well worth it.

lcnr · 2020-09-10T06:05:36Z

I have already both made this mistake myself and saw a few other PRs which did the same, so 👍 on adding a warning here.
The implementation also looks good to me.

@m-ou-se Considering that you mentioned that this detected 3 mistakes, how does this pass CI without having to fix any of them? Am I missing something here?

m-ou-se · 2020-09-10T07:32:06Z

@m-ou-se Considering that you mentioned that this detected 3 mistakes, how does this pass CI without having to fix any of them? Am I missing something here?

They're just warnings right now. So they do appear in the output, but don't fail the CI:

They should be errors instead, like the other messages about stability attributes.

I'm not sure how to solve case 3 though.

lcnr · 2020-09-10T07:39:58Z

We can maybe change this to an unstable lint and explicitly allow it in case 3 🤔

m-ou-se · 2020-09-10T07:55:23Z

I turned them into errors.

The first case is fixed using

#[stable(feature = "alloc_layout", since = "1.28.0")]

The second case is fixed using

#[stable(feature = "integer_atomics_stable", since = "1.34.0")]

Might be good if someone can verify these. I copied the stability attribute from the types, and checked that the (stable) traits and these ('unstable') trait impls already existed at the time the types were stabilized.

The third case still needs a solution. A lint with #[allow(..)] sounds like it could work. Will look into that.

m-ou-se · 2020-09-10T09:22:59Z

Any idea where would be the best place to call register_lints for this lint? Nothing in compiler/rustc_passes registers any lints, but only uses bulitin lints. But adding this to the builtin lints doesn't seem like a good idea, as it has no use outside of the standard libraries.

At first rustc_lints::internal seemed like a good place to put this lint, but these lints are only enabled while compiling rustc, not while compiling std.

lcnr · 2020-09-10T10:20:08Z

hmm, maybe adding it to builtin_lints and only running it if the staged_api feature is active?

I think that's fine even if we don't really need this outside of std

m-ou-se · 2020-09-10T19:41:37Z

Added it to the builtin lints as a rustc:: tool lint, just like the internal lints. That way it doesn't show up in -W help and an #[allow()] attribute for it doesn't trigger a unknown lint error (which happens when bootstrapping rustc+std, using the current beta).

Also added a test, as the 'test cases' in std are now fixed.

library/alloc/src/task.rs

lcnr · 2020-09-11T06:31:25Z

an #[allow()] attribute for it doesn't trigger a unknown lint error

Does this mean that the following test compiles rn?

#![allow(rustc::ineffective_unstable_trait_impl)]…
fn main() {}

I think it would be good to add staged_api as a feature gate for this so that users can't reference this lint.

It should hopefully work by just adding a line similar to

rust/compiler/rustc_session/src/lint/builtin.rs

Line 535 in 94b4de0

@feature_gate = sym::unsafe_block_in_unsafe_fn;

m-ou-se · 2020-09-11T10:16:28Z

Does this mean that the following test compiles rn?

Yes. Every rustc:: lint is ignored when unknown, it seems.

All other internal lints also compile just fine like that:

#![allow(rustc::usage_of_qualified_ty)]
fn main() {}

Tool lints (with ::) don't support @feature_gate. If I turn it into a regular lint, it'll show up in -W help even though it should be internal only, and the annotations I added for Wake will need to be changed to #[cfg_attr(not(bootstrap), allow(...))] to stop the bootstrap/beta rustc from complaining about it until the next release cycle.

lcnr · 2020-09-11T11:35:01Z

hmm, ok 👍

Then r=me with the merge commit removed. @petrochenkov do you still have some concerns or questions about this?

These impls were effectively stable. #[unstable] had no effect here, since both RefUnwindSafe and these types were already stable. These effectively became stable as soon as the types became stable, which was in 1.34.0.

This impl was effectively stable. #[unstable] had no effect here, since both Error and LayoutErr were already stable. This effectively became stable as soon as LayoutErr became stable, which was in 1.28.0.

petrochenkov · 2020-09-11T14:11:44Z

r? @petrochenkov

petrochenkov · 2020-09-11T19:19:42Z

Ok, the previous time I didn't look at the code and misinterpreted what this PR does.
This lint is more against outdated #[unstable] attributes, rather anything else.

We have some conventions regarding lint names (https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#lints), and regarding diagnostic messages ("An #[unstable] annotation here ..." -> "an #[unstable] annotation here ...") which this lint doesn't match, but it doesn't matter much since all of this is internal and unstable anyway.

r? @lcnr

lcnr · 2020-09-11T20:07:25Z

@bors r+

bors · 2020-09-11T20:07:27Z

📌 Commit 14cc177 has been approved by lcnr

…ess-unstable-trait-impl, r=lcnr Warn for #[unstable] on trait impls when it has no effect. Earlier today I sent a PR with an `#[unstable]` attribute on a trait `impl`, but was informed that this attribute has no effect there. (comment: rust-lang#76525 (comment), issue: rust-lang#55436) This PR adds a warning for this situation. Trait `impl` blocks with `#[unstable]` where both the type and the trait are stable will result in a warning: ``` warning: An `#[unstable]` annotation here has no effect. See issue rust-lang#55436 <rust-lang#55436> for more information. --> library/std/src/panic.rs:235:1 | 235 | #[unstable(feature = "integer_atomics", issue = "32976")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` --- It detects three problems in the existing code: 1. A few `RefUnwindSafe` implementations for the atomic integer types in `library/std/src/panic.rs`. Example: https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/panic.rs#L235-L236 2. An implementation of `Error` for `LayoutErr` in `library/std/srd/error.rs`: https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/error.rs#L392-L397 3. `From` implementations for `Waker` and `RawWaker` in `library/alloc/src/task.rs`. Example: https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/alloc/src/task.rs#L36-L37 Case 3 interesting: It has a bound with an `#[unstable]` trait (`W: Wake`), so appears to have much effect on stable code. It does however break similar blanket implementations. It would also have immediate effect if `Wake` was implemented for any stable type. (Which is not the case right now, but there are no warnings in place to prevent it.) Whether this case is a problem or not is not clear to me. If it isn't, adding a simple `c.visit_generics(..);` to this PR will stop the warning for this case.

bors · 2020-09-12T11:20:09Z

⌛ Testing commit 14cc177 with merge 3697de21c220ab1024b31a801b3a2b88a531111d...

bors · 2020-09-12T12:06:06Z

💔 Test failed - checks-actions

m-ou-se · 2020-09-12T12:11:54Z

broken_heart Test failed - checks-actions

Uh, that failure seems unrelated. ui/autoref-autoderef/autoderef-and-borrow-method-receiver.rs failed with no output and exit code 0xc0000005 (access violation). But that test is just some dead code and fn main() {}.

lcnr · 2020-09-12T15:24:14Z

yeah, let's hope this one was spurious

@bors retry

bors · 2020-09-12T18:01:40Z

⌛ Testing commit 14cc177 with merge 9891908...

bors · 2020-09-12T19:56:03Z

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing 9891908 to master...

rust-highfive assigned lcnr Sep 9, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2020

lcnr reviewed Sep 11, 2020

View reviewed changes

library/alloc/src/task.rs Show resolved Hide resolved

lcnr approved these changes Sep 11, 2020

View reviewed changes

m-ou-se added 7 commits September 11, 2020 13:36

Warn for #[unstable] on trait impls when it has no effect.

1854f8b

Turn useless #[unstable] attributes into errors.

e5c645f

Mark RefUnwindSafe impls for stable atomic types as stable.

f6fbf66

These impls were effectively stable. #[unstable] had no effect here, since both RefUnwindSafe and these types were already stable. These effectively became stable as soon as the types became stable, which was in 1.34.0.

Mark Error impl for LayoutErr as stable.

cf8e5d1

This impl was effectively stable. #[unstable] had no effect here, since both Error and LayoutErr were already stable. This effectively became stable as soon as LayoutErr became stable, which was in 1.28.0.

Turn unstable trait impl error into a lint, so it can be disabled.

89fb34f

Allow unstable From impl for [Raw]Waker.

471fb62

Add test for unstable trait impl lint.

1c1bfba

m-ou-se force-pushed the check-useless-unstable-trait-impl branch from 4fef1bc to 1c1bfba Compare September 11, 2020 11:37

rust-highfive assigned petrochenkov and unassigned lcnr Sep 11, 2020

rust-highfive assigned lcnr and unassigned petrochenkov Sep 11, 2020

Improve ineffective_unstable_trait_impl error message.

14cc177

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2020

Dylan-DPC-zz mentioned this pull request Sep 12, 2020

Rollup of 11 pull requests #76621

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 12, 2020

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2020

bors merged commit 9891908 into rust-lang:master Sep 12, 2020

rustbot added this to the 1.48.0 milestone Sep 12, 2020

bors mentioned this pull request Sep 12, 2020

added a lint against function item references #76269

Merged

jyn514 mentioned this pull request Jan 15, 2021

Don't make tools responsible for checking unknown and renamed lints #80524

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn for #[unstable] on trait impls when it has no effect. #76538

Warn for #[unstable] on trait impls when it has no effect. #76538

m-ou-se commented Sep 9, 2020 •

edited

Loading

rust-highfive commented Sep 9, 2020

petrochenkov commented Sep 9, 2020

petrochenkov commented Sep 9, 2020

m-ou-se commented Sep 9, 2020

lcnr commented Sep 10, 2020

m-ou-se commented Sep 10, 2020

lcnr commented Sep 10, 2020 •

edited

Loading

m-ou-se commented Sep 10, 2020

m-ou-se commented Sep 10, 2020 •

edited

Loading

lcnr commented Sep 10, 2020 •

edited

Loading

m-ou-se commented Sep 10, 2020

lcnr commented Sep 11, 2020

m-ou-se commented Sep 11, 2020 •

edited

Loading

lcnr commented Sep 11, 2020

petrochenkov commented Sep 11, 2020

petrochenkov commented Sep 11, 2020

lcnr commented Sep 11, 2020

bors commented Sep 11, 2020

bors commented Sep 12, 2020

bors commented Sep 12, 2020

m-ou-se commented Sep 12, 2020

lcnr commented Sep 12, 2020

bors commented Sep 12, 2020

bors commented Sep 12, 2020

	#[unstable(feature = "integer_atomics", issue = "32976")]
	impl RefUnwindSafe for atomic::AtomicI8 {}

	#[unstable(
	feature = "allocator_api",
	reason = "the precise API and guarantees it provides may be tweaked.",
	issue = "32838"
	)]
	impl Error for LayoutErr {}

	#[unstable(feature = "wake_trait", issue = "69912")]
	impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for Waker {

Warn for #[unstable] on trait impls when it has no effect. #76538

Warn for #[unstable] on trait impls when it has no effect. #76538

Conversation

m-ou-se commented Sep 9, 2020 • edited Loading

rust-highfive commented Sep 9, 2020

petrochenkov commented Sep 9, 2020

petrochenkov commented Sep 9, 2020

m-ou-se commented Sep 9, 2020

lcnr commented Sep 10, 2020

m-ou-se commented Sep 10, 2020

lcnr commented Sep 10, 2020 • edited Loading

m-ou-se commented Sep 10, 2020

m-ou-se commented Sep 10, 2020 • edited Loading

lcnr commented Sep 10, 2020 • edited Loading

m-ou-se commented Sep 10, 2020

lcnr commented Sep 11, 2020

m-ou-se commented Sep 11, 2020 • edited Loading

lcnr commented Sep 11, 2020

petrochenkov commented Sep 11, 2020

petrochenkov commented Sep 11, 2020

lcnr commented Sep 11, 2020

bors commented Sep 11, 2020

bors commented Sep 12, 2020

bors commented Sep 12, 2020

m-ou-se commented Sep 12, 2020

lcnr commented Sep 12, 2020

bors commented Sep 12, 2020

bors commented Sep 12, 2020

m-ou-se commented Sep 9, 2020 •

edited

Loading

lcnr commented Sep 10, 2020 •

edited

Loading

m-ou-se commented Sep 10, 2020 •

edited

Loading

lcnr commented Sep 10, 2020 •

edited

Loading

m-ou-se commented Sep 11, 2020 •

edited

Loading