Uplifts and extends clippy::needless-maybe-sized into rustc

[CrooseGit]

Fixes #140962

Uplift the clippy::needless_maybe_sized lint into rustc as redundant_sizedness_bounds. This is useful for #144404 as it deals with redundant bound that would need to be addressed during migration.

---

redundant_sizedness_bounds

(warn-by-default)

The needless_maybe_sized lint checks that ?Sized bounds aren't redundant. This lint is extended to do the equivalent checks for the sizedness traits introduced by #144404 and thus renamed from needless_maybe_sized.

Example

// `T` cannot relaxed the `Size` bound because `Clone` requires it to be `Sized`. fn f<T: Clone + ?Sized>(t: &T) {}

Explanation

Relaxing a default Sized bound with ?Sized does nothing when there's another bound with a Sized supertrait (Clone in the example above)

---

@rustbot label: +I-lang-nominated
r? @lcnr
cc @flip1995

---

For Clippy:

changelog: Moves: Uplifted clippy::needless_maybe_sized into rustc as redundant_sizedness_bounds

[rustbot]

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[CrooseGit]

@davidtwco

[samueltardieu]

Reviewed only the Clippy part.

View changes since this review

[CrooseGit]

Thank you @samueltardieu for your feedback. Regarding the small changes to unrelated files, I believe that was my auto-formatter doing me a disservice, and I believe I have removed said changes from the commits.
I also believe I have addressed the other points you made, but do let me know if anything is amiss.
Thanks again.

[samueltardieu]

I also believe I have addressed the other points you made, but do let me know if anything is amiss.

I think you still have to remove src/tools/clippy/tests/ui/needless_maybe_sized.rs and src/tools/clippy/tests/ui/needless_maybe_sized.stderr.

View changes since this review

[CrooseGit]

As far as I can tell those files have already been removed, let me know if I'm missing anything. Thank you

needless_maybe_sized.stderr
needless_maybe_sized.rs

[davidtwco]

I think you still have to remove src/tools/clippy/tests/ui/needless_maybe_sized.rs and src/tools/clippy/tests/ui/needless_maybe_sized.stderr.

These have been removed as far as I can tell, they're just showing as moves in Git as similar tests have been added to the rustc ui test suite w/ some changes.

[samueltardieu]

I think you still have to remove src/tools/clippy/tests/ui/needless_maybe_sized.rs and src/tools/clippy/tests/ui/needless_maybe_sized.stderr.

These have been removed as far as I can tell, they're just showing as moves in Git as similar tests have been added to the rustc ui test suite w/ some changes.

Oh yes, the subpar GitHub diff UI shows this only at the point of destination, not at the point of origin.

[CrooseGit]

I have added a .fixed file to the tests

[tmandry]

@rfcbot fcp merge

[obi1kenobi]

I don't want to bikeshed: I like this lint, and I'd rather have the better version of this lint by any name sooner, than a perfectly-named lint later. But I'm currently working on making cargo-semver-checks detect the SemVer breakage analogue of this lint, and I have a concern about "unused".

I think it's at least a nice-to-have if related SemVer ("across time") and rustc/clippy ("single point in time") lints have similar names. While "unused" seems plausibly reasonable in the "point in time" setting, it would imply a "becomes unused" lint in the SemVer setting. That doesn't seem like the right word at all to me.

An example of the SemVer lint would catch e.g. a T: ?Sized + Foo where Foo in a later version starts implying Sized, thereby constraining T as well even though there was no syntactic change near T itself. The word "unused" usually appears in contexts like "unused import" and completely fails to communicate the seriousness of such a situation. If cargo-semver-checks tells the user: "your T now has an unused ?Sized bound," I'd be shocked if that made them realize they are about to make a major breaking change!

I think I'd rather say that ?Sized is inapplicable, or inaccurate, or that it's non-binding (as in, it fails to correctly describe the actual bounds of the API). I'm bad at naming things, and there are probably better words too. But "unused" doesn't feel like the right choice to me.

I'm open to making it more general, subject to perf being okay (it could be an awful lot of querying supertraits of supertraits of traits, etc).

I'm in the middle of prototyping something that does that kind of awful lot of querying, for rustdoc JSON and cargo-semver-checks: #143197 Maybe we should talk? :)

[traviscross]

An example of the SemVer lint would catch e.g. a T: ?Sized + Foo where Foo in a later version starts implying Sized, thereby constraining T...

If you could elaborate this example, that'd be helpful. In particular, why are we worried about the bound here rather than the change to the trait?

[bors]

☔ The latest upstream changes (presumably #147779) made this pull request unmergeable. Please resolve the merge conflicts.

[camsteffen]

I think the concern is that you may unknowingly start disallowing unsized types in your API when you intended to prevent that with ?Sized. So the severity of the lint is higher than a dead code lint. I don't know if we can actually do much better for the name. But maybe "reversed" or "overridden" or "negated" or "invalid" or "noop".

[obi1kenobi]

I think the concern is that you may unknowingly start disallowing unsized types in your API when you intended to prevent that with ?Sized. So the severity of the lint is higher than a dead code lint.

This is exactly it.

I'm concerned the user will see a rustc/clippy lints for needless_maybe_sized and go "ehh, not that big of a deal" and possibly #[allow] it. Then they'll see a cargo-semver-checks lint for became_needlessly_maybe_sized and go "still not a big deal." Then they'll ship SemVer breakage.

I like all names @camsteffen and I proposed better than "unused" because they signal a higher severity. I'd still avoid "noop" if possible because I think it has the same risk, just not as much of it.

[traviscross]

It's still not coming into focus for me. Is this the concern?

If the user has, in v1.1,

pub fn f<T: ?Sized>(_: &dyn Deref<Target = T>) {}

and then the user tries to ship, in v1.2,

pub trait Tr: Sized {} impl<T> Tr for T {} pub fn f<T: ?Sized + Tr>(_: &dyn Deref<Target = T>) {}

getting this warning,

warning: `?Sized` has no effect due to another bound --> src/main.rs | | pub fn f<T: ?Sized + Tr>(_: &dyn Deref<Target = T>) {} | ^^^^^^ help: remove this unused bound relaxation | = note: generic parameters are assumed to be `Sized` unless the bound is relaxed by writing `?Sized`, but here this has no effect as another bound implies `Sized` = note: `#[warn(unused_relaxed_bounds)]` on by default note: the bound that implies `Sized` is here --> src/main.rs | | pub fn f<T: ?Sized + Tr>(_: &dyn Deref<Target = T>) {} | ^^ note: this bound implies `Sized` | note: `Sized` is implied by `Tr` as `Sized` is a supertrait --> src/main.rs | | pub trait Tr: Sized {} | ^^^^^ note: `Sized` is a supertrait |

then the idea is that the user is going to reason, "I added a new T: Tr bound on the public function f, which would normally be breaking, but it's my trait, and I've implemented it for all T, so this should be OK. Rust is warning me that the ?Sized bound relaxation, which I put there to ensure that types that don't implement Sized are accepted, is now unused and is being ignored by the compiler when I wasn't getting that before, but that's probably OK because...? Well, I mean, Rust even suggested deleting the ?Sized, so it must be OK to delete, right?"

And then cargo-semver-check is going to report this as SemVer breakage, and the user will think, "no, it's OK, Rust told me that ?Sized was unused and suggested I delete the bound relaxation, so this must be OK."

Is that right, or no?

[traviscross]

As context, I take for granted that unused warnings in Rust must be taken seriously.

E.g., if we have,

pub trait Tr: Sized { fn f(self) -> u8 { 1 } } impl<T> Tr for T {}

and then somewhere else we write,

pub struct W<T>(T); impl<T> W<T> { fn f(&self) -> u8 { 2 } } fn main() { println!("{}", W(()).f()); }

thinking that, "obviously this should print 2; W<T>::f is an inherent method, and inherent methods take priority", or maybe just not noticing that trait or trait impl at all, and then we get,

warning: method `f` is never used --> src/main.rs:7:8 | 6 | impl<T> W<T> { | ------------ method in this implementation 7 | fn f(&self) -> u8 { 2 } | ^ | = note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default

that's a very serious warning. That causes me to sit up in my chair. I think, "if that code isn't being used, then what am I running? Not what I was expecting to run, obviously."

Any unused warning that I wasn't expecting does that for me, for this kind of reason. And so it's what I think about, in this case, when I think about how I'd react.

One ignores unexpected unused warnings in Rust at one's peril.

[camsteffen]

I don't really object to unused personally. I agree whatever lint should make you stop and consider. However, there is an unused lint group for lints with a certain semantic and this lint wouldn't fit in that group. Unused typically means "you wrote X and X is unreachable" rather than "you wrote code X which looks like it does Y but it actually does not".

Responding to your first example, FWIW, a more subtle story would be that you have T: Tr + ?Sized and there is no lint. You then add a supertrait Foo to Tr and Foo is Sized.

[traviscross]

Responding to your first example, FWIW, a more subtle story would be that you have T: Tr + ?Sized and there is no lint. You then add a supertrait Foo to Tr and Foo is Sized.

You've then made a breaking change to the trait. At that point, it seems like the use-site bound is the least of one's SemVer problems.

[obi1kenobi]

I agree whatever lint should make you stop and consider. However, there is an unused lint group for lints with a certain semantic and this lint wouldn't fit in that group. Unused typically means "you wrote X and X is unreachable" rather than "you wrote code X which looks like it does Y but it actually does not".

Strongly agreed with this. "Users should take lints seriously" I agree with, but that is not the same statement as "users take lints seriously" which I don't think follows or is accurate in general.

A great strength of Rust is setting up users to fall into the pit of success. Using unused in this lint does not feel like a pit of success choice.

We should also remember there are other analogous flavors of "you wrote code X which looks like it does Y but it actually does not." What we name this lint will set a precedent for name those future lints. One example with 'static bounds: playground

trait Tr: 'static {} #[allow(private_bounds)] pub trait Intermediate: Tr {} pub fn f<'a, T: Intermediate + 'a>(_: &'a str, _: &dyn Deref<Target = T>) {} 

Here, T: 'static implicitly because of T -> Intermediate -> Tr -> 'static but:

• The source code for f() makes it look like it only needs to be 'a.
• Rustdoc and docs.rs for f() make it look like it only needs to be 'a`.
• The same is true for both source code and rustdoc/docs.rs for Intermediate---neither mentions 'static.

This does not currently trigger any lint, neither in rustc nor in clippy. I believe it should. By precedent from here, would such a lint then be named unused_lifetime_bound?

If so, IMHO we'll end up dramatically redefining what the unused group of lints does. We'll do so in a step-by-step fashion, one lint at a time, which users will not notice until they get burned one day because their prior conditioning about the meaning and importance of unused lints.

Not a pit of success choice.

You've then made a breaking change to the trait. At that point, it seems like the use-site bound is the least of one's SemVer problems.

I agree, the breaking change is to the trait. But that can be viciously difficult to catch, and I believe we should offer as many opportunities as possible for users to spot and stop this problem. Consider this example: playground

trait Tr {} #[allow(private_bounds)] pub trait Intermediate: Tr {} pub fn f<T: ?Sized + Intermediate>(_: &dyn Deref<Target = T>) {} 

Let's assume there are suitable impls to allow downstream users to have access to both sized and unsized types that f() is callable with.

Furthermore, assume Tr, Intermediate, and f are all in separate files. For example, maybe Tr is in a mod not pub mod and is surrounded by other non-pub items.

Say one then changes Tr to trait Tr: Sized {}. This is undoubtedly a major breaking change, but one that's viciously difficult to notice---IMHO analogous to Send/Sync auto-trait changes:

• It's caused by a change to a non-pub item.
• The breakage happens far from the place where the change happened.
• It can happen through arbitrarily many layers of indirection, not just the one layer we have here.
• It's infectious: if one crate breaks SemVer this way, downstream crates depending on the same trait bound tree will also break---and in a way that's easy to misinterpret as the downstream crate breaking SemVer. To the users of downstream crates, it'll appear that post-cargo update there's now a bound-related compile error at the call site to the downstream crate, which will cause additional support and triage burden etc.

It's thus all too easy for a trait bound change on a private trait, in an entirely non-pub corner of one's crate, to escalate and ripple across the ecosystem.

SemVer breakage is a numbers game. IMHO we should play the numbers by doing our best to block that escalation path in as many places as possible. I believe this includes choosing a name for this lint that makes it harder to (mistakenly) ignore and just suppress away.

To see how users might not understand the broader context of a lint and suppress it without due care, we only have to look as far as that #[allow(private_bounds)]. To illustrate my point (i.e. not proposing we do this) imagine if that line instead said #[allow(all_private_traits_and_their_bounds_recursively_are_forever_public_api_here)] --- I think that would have also contributed to blocking this kind of escalation path.

[obi1kenobi]

I'm happy to answer questions and generally engage with the discussion, but I don't want to do so at the cost of shipping this lint at all. As I mentioned, my goal is not to bikeshed this :)

I feel I've made my case, and I hope you've found it persuasive. If not, that's fine! Consider my voice heard and, as far as I'm concerned, feel free to proceed!

[traviscross]

Thanks for those details and discussion. To answer your question, given this:

pub trait Tr: 'static {} pub fn f<'a, T: Tr + 'a>() {} // ^^ help: remove this bound

Yes, I would suggest that bound is unused. It can be removed from the program without effect; the compiler is free to ignore it. While one could equally describe it as "ineffectual", "pointless", "useless", "redundant", "superfluous", "needless", "inconsequential", "unnecessary", etc., these all seem like synonyms for unused in a Rust context. Other unused_* lints could equally use these synonyms.

In some cases where we use "unused" today, one could even argue for stronger terms like "contradictory" or "conflicting". E.g.:

#[unsafe(export_name = "foo")] //~ WARNING: unused attribute #[unsafe(export_name = "bar")] //~^ This could be made more subtle with `cfg_attr`, macros, etc. pub fn f() {}

Here, conflicting_* would be a reasonable name (similar to conflicting_repr_hints). Unlike with ?Sized + Sized, there is no semantically obvious way to combine these. That's why "conflicting" feels appropriate, to me, in the above, but not for ?Sized + Sized.

Broadly, I think the purpose of our lint names is to be as precise, concise, and consistent as possible, not to try to hit a particular scariness level. If we want to scare users, the lint output seems a better place to do that than the name.

All that said, if in discussion we turn out to not be happy with "unused", here are some other names that have occurred to me:

• subsumed_relaxed_bounds: The relaxed bound has been "subsumed" by a tighter bound, just as 'a is "subsumed" by 'static. There's maybe some tension in the relaxed_bounds case due to relaxation going in one direction from the default and then another bound going in the other direction, but we should probably consider this in the context of our "ask for what you need" direction in the hierarchy of Sized types work. I.e., the bound will be something like T: Pointee + Tr where trait Tr: MetaSized {}, and here it will be true that Pointee is subsumed by MetaSized.
• negated_relaxed_bounds: This hints specifically at the idea that the relaxed bound has been made ineffective.
• misleading_relaxed_bounds: This one I toyed with, but I don't really care for it, as it seems to just push the question down a level. "Why is it misleading? Because it has no effect." It seems better to try to speak to what's happening in the compiler, where possible, rather than to the speculated effect on the user's state of mind.

The "ask for what you need" hierarchy of Sized traits model does raise another question here for us. If we did have a separate lint for when a trait in a bound is already implied by being a supertrait of another trait in a bound, would we expect that to additionally fire given T: Pointee + Tr where Tr: MetaSized? It'd both be true that the bound relaxation has no effect and that a trait in the bound is already implied.

That's something to think about. Maybe, given our direction here, this new lint could itself be "subsumed".

[obi1kenobi]

Broadly, I think the purpose of our lint names is to be as precise, concise, and consistent as possible, not to try to hit a particular scariness level. If we want to scare users, the lint output seems a better place to do that than the name.

Ordinarily I would agree with this. But naming a lint unused_* to me implies that it also becomes part of the unused lint group, which is where the real problem lies IMHO. Every other lint in there is genuinely a fairly trivial concern, quite unlike the hazards that this issue is about.

We don't want #[allow(unused)] that was intended to en masse suppress a group of other lints to cause ineffective and confusing ?Sized bounds to remain unnoticed.

That's something to think about. Maybe, given our direction here, this new lint could itself be "subsumed".

I'm happy with "subsumed" and it genuinely feels like the best name of the lot. It could also be the name of a new lint group for all these lints, which I think would be a good outcome.

[camsteffen]

#[unsafe(export_name = "foo")] //~ WARNING: unused attribute #[unsafe(export_name = "bar")]

That's a good example. I feel convinced now that unused is actually not just for "trivial" concerns and may include cases of two things conflicting with each other and one wins out.

[obi1kenobi]

Given that it sounds like it was news to both of us that an ineffective unsafe is a lint in the unused group, I feel more convinced that that lint should also be renamed.

[nikomatsakis]

We discussed this in the lang-team meeting and concluded two things

• we all agree that T: ?Sized + Clone should lint, as the ?Sized is misleading
• but we were split on whether T: Pointee + Clone should lint.

In particular, other kinds of redundancy are not clearly worth linting over, for example, given trait Foo: Bar, T: Foo + Bar might be a problem, but maybe not, maybe the author wants to emphasize that they directly use methods from Bar as well as Foo.

The "double role" of Pointee here makes this case a bit subtle.

We were tempted to say: let's just do it for ?Sized, but @traviscross pointed out that the motivation of this PR is likely more to do with the second case, which raised the question: How important is this particular change to the overall hierarchy of sized traits? It seems like at most a "nice to have" and not essential, but maybe there is a connection we are not understanding.

[nikomatsakis]

Can somebody update as to this question so we can review again in next triage? Thanks.

[scottmcm]

Definitely a fan of marking bounds that "don't work", in some form.

That said, I'd trying to think of whether there's potentially any reasons that you might want to write something for explicitness even if it's arguably useless. Like T: SomeForeignTrait + Clone doesn't seem unreasonable to write, even if Clone is a supertrait of SomeForeignTrait. Because even if it's semver breaking to remove the supertrait, if I'm intentionally cloning it I might want to just have that there to see more easily in the code and/or the rustdoc. And if the crate does do a breaking change to remove a supertrait, it might be nice that when you do the major version bump that you still had the "unused" bound.

So I'm a fan of linting on the "conflict"-y cases like T: Clone + ?Sized where they're in a sense contradicting each other. Someone mentioned (forget who, sorry) that if we had negative bounds on negative impls, then T: Foo + !Foo is clearly worth linting.

As a result I'd be fine focusing to the ?Sized cases in this PR, and figure out some of the other more complicated cases (like whether we can meaningfully distinguish T: SomeForeignTrait + Clone from T: Copy + Clone) later in a different lint.

[traviscross]

For my part, I'm not sure I would be happy with linting on T: ?Sized + Clone if we weren't happy with linting on T: Pointee + Clone, and I'm not sure I'd be happy with linting on T: Pointee + Clone if we weren't happy with linting on T: Sized + Copy (where Copy: const Sized) or more generally on trait bounds that are unused due to being already implied by being a supertrait of another trait in the bound. From a user pointer of view, I'm not sure these will all really seem that different.

At the same time, I'm sympathetic to @scottmcm's point that writing out the subtrait explicitly in the bound can sometimes be desired to improve local reasoning or (as I'd maybe see it) as a static assertion.

[camsteffen]

We were tempted to say: let's just do it for ?Sized, but @traviscross pointed out that the motivation of this PR is likely more to do with the second case, ...

I don't think so? The PR description only contains the first case (?Sized + Clone). The clippy lint being uplifted only covers the first case. The other cases are thrown in as net new behavior AFAICT.

For my part, I'm not sure I would be happy with linting on T: ?Sized + Clone if we weren't happy with linting on T: Pointee + Clone,

I thought the former case is clearly more lintable since there is a possible conflict with the user's intention.

[traviscross]

For my part, I'm not sure I would be happy with linting on T: ?Sized + Clone if we weren't happy with linting on T: Pointee + Clone,

I thought the former case is clearly more lintable since there is a possible conflict with the user's intention.

There's a potential conflict with the user's intention with all three cases. In every case, the user is getting at least as much as the user asked for, but not as little as the user asked for with one of the bounds in isolation.

[camsteffen]

the user is getting at least as much as the user asked for

But I think it's quite plausible that a user will think ?Sized means must be allowed to be unsized. Personally I'm surprised it doesn't mean that, and the lint feels like correcting a mistake. Maybe my perception here is off?

[obi1kenobi]

Case in point: I've been working on SemVer for 3.5 years, and that was my expectation. I was shocked to find that ?Sized doesn't behave like that. I have to emphasize, I understand the logic and reasoning of why it works the way it does. But without reading about it in depth, I never would have assumed that's how it works. I think the lint should target users' possible misconceptions — we can't brush them away by saying "that's not a reasonable expectation" if that's genuinely how people think it works.

[traviscross]

...?Sized means must be allowed to be unsized. Personally I'm surprised it doesn't mean that, and the lint feels like correcting a mistake. Maybe my perception here is off?

In certain places in the language we add an implicit Sized bound, e.g. on generic parameters to functions -- but not on e.g. the item bounds of trait definitions, e.g. trait Tr: 'static, or on associated type bound syntax, e.g. T: Deref<Target: 'static>. This implicit Sized bound is exactly like writing Sized explicitly. In these places, ?Sized is allowed and means "don't add this implicit Sized bound". If you think about these bounds as desugaring to a language without default bounds, you can simply think of ?Sized as controlling whether Sized is emitted as a bound in this desugaring.

The ? bound relaxation syntax has always been a bit dissatisfying, including for the reasons you mention of intuition and comprehension. In the "hierarchy of Sized traits" (rust-lang/rfcs#3729) work, lang discussions have tended toward a direction of eliminating these ? relaxations. This is what we mean when we say, "ask for what you need". The appeal of this model is that asking for the minimum you need is the intended intuition for bounds in Rust.

In place of these ? relaxations would be a rule that certain magical traits, like Pointee or SizeOfVal, would relax the default trait bound implicitly (without writing ?Sized or ?SizeOf or whatnot). (Whether we'd look through other traits in the bound to find these as supertraits and relax the bound in that case is an open item; it has some appeal, but seems difficult.)

Of course, this then means that, in writing T: Pointee + Clone (or T: Tr + Clone where trait Tr: Pointee {} in a world where we do this transitively) that both 1) a bound relaxation gets ignored but also that 2) a bound is being ignored because it's implied by a supertrait of another trait in the bound, which is a general matter that can apply more broadly. It's not obvious that the unused relaxation would be the main bit of confusion. If you have a trait hierarchy A: B, B: C where C is the default bound, and you have some trait Tr: A, and you write T: B + Tr, then even though you're above the default bound, you could be forgiven for thinking, "I said T: B + ... -- why is my caller getting an error when passing a type that implements B?".

Since we're looking ahead to this likely model, this is part of the design challenge we're considering here. I mention this and elaborate it a bit as, unless one has been following our design discussions closely, the details of this consideration are likely not obvious.

[CrooseGit]

Upon reading the above discussion I am struggling to work out if a consensus has been reached.

As @traviscross pointed out there is currently an inconsistency in the patch. The lint description claims it lints for cases such as fn g<T: Default + Sized>(t: &T) {} where there is a Sized bound which is already implied by another bound.
This can be amended easily by either adding this lint, or removing the incorrect example.

Just to recap, currently the lint will pick up:

1. Sized? + <bound implying Sized>
2. MetaSized + <bound implying Sized>
3. PointeeSized + <bound implying Sized>
4. PointeeSized + <bound implying MetaSized>

Which I suppose can be generalised into two groups:

• Linting where there is a sizedness bound which is made redundant by another bound implying a greater degree of sizedness (Points 2 - 4).
• Linting where there is a maybe sized bound which is made redundant by another bound implying Sized. (Point 1)

If there is rough consensus, I would appreciate if someone could detail the required changes to this PR.
Thank you

[traviscross]

There was not a consensus. We're still working this one out.

[camsteffen]

In place of these ? relaxations would be a rule that certain magical traits, like Pointee or SizeOfVal, would relax the default trait bound implicitly

Thanks for explaining. I think then that Sized? + Clone and Pointee + Clone are pretty much equivalent cases that can be the same lint. You are potentially relaxing bounds but are not accomplishing that.

For Sized + Clone and other cases that are redundant but without any relaxation possibility, I still think that is a separate lint if not Clippy territory. Reaching for a comparison, I think it's kinda like Clippy's redundant_slicing. It's cleaning up redundant code, but probably not catching a mistake IMO. Personally, I don't think I'd mind having that lint enabled in rustc, but I wouldn't blame someone for calling that lint noise.