Port clippy lint `redundant_field_names` to compiler by fee1-dead · Pull Request...

Copy link

Member

rust-highfive commented on Jul 27

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive commented on Jul 27

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

rust-highfive commented on Jul 27

Warning

• These commits modify compiler targets. (See the Target Tier Policy.)

This comment has been hidden.

camsteffen commented on Jul 28

I think we should remove the lint from Clippy in the same PR.

The lint really should be named redundant_field_initializers or field_init_same_name or field_init_repeats_name.

Are we okay with uplifting this, warn by default, with the possibility of enhancing it if we allow Struct { &foo }? (I think that's fine).

Can we put this in the nonstandard-style group? It seems like rustc is lacking a "style" group for slightly more picky lints. Someone will say that such lints belong in Clippy, but I'm not so sure. This lint feels like it could be in rustc.

If this lint triggers in macros for $a: $b, that's probably a deal-breaker. Maybe we can detect cases like $a: $a.

fee1-dead commented on Jul 30

Can we put this in the nonstandard-style group?

I don't think so because AFAIK that group is only used for lints with nonstandard naming conventions.

This comment has been hidden.

bors commented on Jul 31

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

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

bors commented on Aug 2

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

This comment has been hidden.

camsteffen commented on Sep 12

Clippy changes LGTM. I just have a concern regarding macros.

This comment has been hidden.

fee1-dead commented on Sep 12

I don't have bors permissions ATM. @cjgillot can you approve it please? Thanks.

workingjubilee commented on Sep 12 •

Why are we uplifting this lint? It does not fit the usual pattern of uplifted lints (e.g. strict correctness criteria) and there is no motivation discussed in the PR or commits. The feature it lints for use of was added in 2016 October to nightly and stabilized in 2017 February. This will add a warning to code that was simply correct before that, and is in any case probably not doing any harm by omitting a bit of syntactic sugar.

This fits the definition of an allowed-by-default lint, but I do not think that is what you want.

fee1-dead commented on Sep 12 •

@joshtriplett said in this zulip thread that he would like to see clippy lints uplifted.

I do see a small problem for projects that want to maintain a quite low msrv (<1.17.0). But I don't know about whether there are projects that need to maintain a msrv this ancient.

To quote the rustc-dev-guide:

Diagnostic Levels

Guidelines for different diagnostic levels:

• error: [...]

• warning: [...] Some examples of when it is appropriate to issue a warning:

  • [...]
  • Unnecessary syntax that can be removed without affecting the semantics of the code. For example, unused code, or unnecessary unsafe.
  • [...]

Note that in a section below it says the warn lint level is equivalent to the warning diagnostic level.

I don't know about the strict correctness criteria for lints, can you tell me where and what it is?

workingjubilee commented on Sep 12

This could be considered in the same category, but it is not an instance of code that seems like it was intended to be used, or reflective of a misunderstanding as with a false invocation of unsafe, it is simply a bit of repetition. This has usually not merited a lint in rustc, and indeed the language actually favors repetition as with e.g. trailing commas where it would make things easier to rename and refactor... as might be a sensible choice here.

Many lints have been uplifted from clippy in the past, and I would honestly be happy to see more. But what I mean is that previously many of the lints that were uplifted from clippy were correctness lints (of which there are even now many that seem like good candidates) and were both motivated and granted on such a reason. Most of the interest I have seen expressed that aim, and your excerpt comes right next to these quotes:

Care should be taken when adding warnings to avoid warning fatigue, and avoid false-positives where there really isn't a problem with the code.

Code that is very likely to be incorrect, dangerous, or confusing, but the language technically allows

Yes, there are projects that have an MSRV below 1.17: serde is one of them.

Mark-Simulacrum commented on Sep 12

@joshtriplett said in this zulip thread that he would like to see clippy lints uplifted.

I am pretty sure that the interpretation here is not that all clippy lints should be uplifted, but rather that approval of lints for uplift via targeted PRs or T-lang MCPs is much more likely to be a successful path toward a decision, as opposed to the prior art at the time of that comment, which was an issue proposing lots of lints to be moved.

Personally, I tend to agree with @workingjubilee that it feels like this lint definitely belongs either in clippy or as an allow-by-default lint; it's primarily a stylistic lint, and doesn't necessarily mean that all people will want to write their code like this. I'd personally be frustrated when seeing a warning on this pattern because the code is equally correct either way and not really significantly more verbose or anything like that.

camsteffen commented on Sep 14

Without a lint category, it doesn't really make sense to uplift. I think rustc should have an allow-by-default category for lints like this:

• clearly more idiomatic
• purely stylistic, non-critical, low-risk
• lints usage of a language feature or prelude items (generally not std API usage)
• the scope of the lint is clear and simple with no perceivable future enhancements

Some candidates: needless_return, write_with_newline, writeln_empty_string, redundant_pattern, redundant_static_lifetimes, single_match, unused_unit

There is a very large gap between rustc lints and Clippy lints in terms of stability, false positive rate, lint pickiness, opinionatedness. Of course some of the "blame" falls squarely on Clippy to fix bugs, remove un-useful lints, etc. But Clippy will likely never have the level of stability that rustc has as it is sort of a playground for new lints. A rustc lint category can bridge that gap a bit for people who want rustc to be just a little more picky, but in a highly stable way.

workingjubilee commented on Sep 20 •

I understand Clippy has a lot of highly experimental lints, but are we sure there isn't any chance that Clippy could make the first move on growing such a "low stress" mode where it just emits warnings on lints with stronger consensus behind them? It would seem an appropriate "proof of concept" for the idea, at least.

Clippy is often run in CI for Rust projects and if it denies something then CI doesn't pass. And a lot of projects run -D warnings in CI, so if Clippy is pretty unstable on warnings then that is probably a bit of a problem, too. It may be a good idea to more actively rein that in somewhat, or at least use the nursery category a bit more freely when it becomes apparent a lint is more half-baked than expected.

bors commented on Sep 26

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

nikomatsakis commented 16 days ago

@rfcbot fcp close

We discussed in the @rust-lang/lang meeting today. The consensus of the meeting was that we ought to close this PR. We generally prefer to limit uplift to lints that fix known bugs and have a very limited set of "false warnings". This case seems to be a conventions question and we would prefer to keep such work within clippy.

Thanks @fee1-dead for raising the question!

rfcbot commented 16 days ago •

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

nikomatsakis commented 16 days ago

One side note: My actual preference would be to remove most style lints but to have rustfmt silently remove and edit cases like this. That way we don't bother our users with it but we also see the edits applied consistently across Rust code. I think that would be best discussed with the @rust-lang/wg-rustfmt team, though, and perhaps even wants an RFC for broader discussion (I think it's a bit of a policy change from how rustfmt does things now).

One side note: My actual preference would be to remove most style lints but to have rustfmt silently remove and edit cases like this. That way we don't bother our users with it but we also see the edits applied consistently across Rust code. I think that would be best discussed with the @rust-lang/wg-rustfmt team, though, and perhaps even wants an RFC for broader discussion (I think it's a bit of a policy change from how rustfmt does things now).

fwiw we do already have a number of these covered via config options in rustfmt (e.g. I gather use_field_init_shorthand could be of use in this case), but they're often turned off by default.

As you alluded to, we don't have the flexibility today to intentionally change rustfmt in such a way that an updated version introduces formatting changes. However, there's at least a couple changes I think we need to try to get pushed through anyway to get rid of some odd formatting behavior that's no longer necessary, and that could potentially provide a window for the some of the other changes you have in mind too (if we do end up introducing formatting changes then they'd ideally come in one batch so users can rustup update && cargo fmt once and be done with it).

Will likely be a topic of discussion at an upcoming dev tools meeting

rfcbot commented 9 days ago

This is now entering its final comment period, as per the review above.

scottmcm commented 9 days ago

I'm totally ok with this staying in clippy, so I've 'd here.

That said, I wonder if there's a more restricted subset of this that's unambiguous enough to have in rustc, like how unused_parens doesn't actually lint on all unnecessary parens -- we don't lint (1 * 2) + 3, for example.

For example, I might not say that rustc should lint on

   Foo {
       x: foo(),
       y: y,
       z: bar(),
   }

but I would be happy for it to lint on

    Foo { x: x, y: y, z: z }

(like one might see in Foo::new)