Clippy lint against, or some other way of detecting, default binding modes — Take 2

21

Haven't read all of it yet, but there is a clippy lint now named pattern_type_mismatch that will guard against default binding mode usage.

Edit: I named it as such to increase the chances of it getting into clippy (Addendum: by making it more acceptable to those worried about it being used simply out of protest). Unfortunately for that reason it doesn't mention binding modes or anything like that. Maybe someone would like to PR an addition of the terms so one can find it.

Edit 2: Link to the lint.

12 Likes
22

Wow, nice work, that was fast!

23

It's been there a while and is already stable, it also took me a bit to get it together :slight_smile:

I didn't advertise it widely though. I thought I published it on the binding modes thread that was active at the time but I can't find it again.

11 Likes
24

Edit: removed some text saying I was disappointed in this approach because I misunderstood what phaylon meant here: they clarified that they did this to try and move past the controversy. I do still wish that it had been done differently, as I'll explain below, but I'll avoid language that asserts a value judgement. I apologize to the folks involved here for using language that implied any kind of value judgement.

I wish there had been more attempts to connect this to the previous discussion. Ironically, this is a good name for the lint, and overall that PR assuages most of my concerns from the last time this was proposed (which the OP here has mislabeled as "without proper consideration": I was pretty clear about what we needed to make something a lint, and those things were not provided). This PR was made with the knowledge that there was a previous rejected discussion on this. It addressed some of the points in that discussion, but didn't link to it as much, effectively wiping the slate clean. There was a reasonable reason for that, and I don't fault the parties involved, but I do wish in the future we avoid any kind of "wipe-the-slate-clean" dynamics.

Here's why I find the lint as landed to be actually okay:

Firstly, it's a restriction lint, which has an overall much weaker bar: restriction lints are ones where we are explicitly not making any judgement call on the quality of the code. There are some risks associated with widespread usage of restriction lints in the ecosystem, and we have to better understand that, but overall we're far more open to those lints.

Secondly, pattern_type_mismatch, to me, seems way better motivated here than the original issue from ages ago (or even most of this thread). It's not "this feature sucks", it talks about the actual underlying thing to be linted about, and motivates it way better. Maybe not enough, but definitely much more.

I wish the approach had connected more to the older conversations because connecting it to the older conversation would have enabled us to have a proper discussion about what the precise bounds of restriction lints are (I personally am slightly in favor on such lints being okay as restriction lints, but I would like to have that discussion), and instead this kind of "slipped through". Additionally it also helps normalize "ask for your feature enough times until you encounter the maintainer willing to accept it", which also isn't great.

12 Likes
25

Honest question: was the OP not seen as a restriction lint? If not, what was it seen as? And why?

Having never used clippy, I'm completely oblivious to clippy's various types of lints, so I have no idea how things are classified. But this paragraph makes it sound like "restriction lint" is the magic password* to having a fruitful discussion here, which I suspect many people (including myself) weren't fully aware of. People know what lints are, and they might know what kind lints they want, but they may not have the jargon to initiate the discussion "correctly." Or at least that's me.

I'd like to understand this part better because if I ever propose a clippy lint, I'd like to know which technical phrases I should use or avoid.

*I use "magic password" in a lighthearted and good faith type of way, not in a mean spirited way.

10 Likes
26

Bear in mind, the original issue was filed three years ago, back when we hadn't fully figured this out (the clippy 1.0 RFC was fleshed out that same year).

But yes, by default we don't think about restriction lints unless someone makes it clear what they're looking for is a restriction lint. Restriction lints are kinda like an extra part of clippy, not part of our main goals, and thus not something we usually think about. This doesn't mean lint-proposers need to know the words "restriction lint", but it's usually reasonably obvious and usually if someone proposes a lint which is of the form "I have such and such reason for disabling it on my crates" we mention restriction lints and explain what they mean, to see if that's what the user wants.

But the main reason I rejected the original issue was that it did not really explain why the code could be bad beyond "people don't like it". pattern_type_mismatch's PR does this acceptably well.

And ultimately, a clippy request to disable a feature immediately after it lands is ... not kosher either IMO. Such a thing should be a part of that RFC itself, and if it doesn't make it into the RFC, tough luck. There's this trend of people who don't like a proposed feature mobbing the RFC in unconstructive ways, then mobbing the tracking issue, then filing issues about tempering it, then spilling over to clippy to try to "fix" it, and it's honestly tiring to deal with. I don't think this is constructive at all, does not achieve the goals of the process, and just makes it more stressful for everyone involved. By putting my foot down on that particular aspect of it I'm attempting to avoid setting up an "asshole filter" where the only times changes happen are ones where people are willing to repeatedly boundary-push until they get their way.

10 Likes
27

For reference, here is the original proposed PR; if its intentions were seen other than as written then I apologise for any lack of clarity and would welcome suggestions of wording changes to any future such proposals.

1 Like
28

So it looks like the documentation of the clippy lint avoids the phrase "binding modes", but the PR proposing it did not. On the contrary, the PR explicitly stated its intent to "avoid[] mentioning default binding modes" so that "noone should be inclined to activate the lint simply because they vaguely remember past discussions".

@phaylon, your previous post in this thread made it sound like you avoided mentioning binding modes in the PR in order to, essentially, sneak the feature into clippy.

5 Likes
29

Just to be sure I understand, you're saying that's the reasoning for rejecting lints involving recently implemented or stabilized features, and not that the mobbing behaviour is what happened here, correct?

(I read the RFC thread and tracking thread and didn't see any discussion I would consider combative or uncivil. Also of note is that most discussion about unexpected and unresolved behaviour starts post-stabilization-merge, about a week before the feature hits stable.)

30

Since I was the original reviewer of the PR, I also want to comment on this. I read through the whole thread and hope that I don't forget anything. I want to address these things:

  • Clippy is not the tool to reverse rustc features
  • The pattern_type_mismatch lint and why I agreed to allow it in Clippy
  • The restriction group (briefly)

Clippy is not the tool to reverse rustc features

I completely agree with @Manishearth here. Clippy is not a tool you should (ab)use to implement a lint, reversing a feature, just because you don't like it. I myself also closed some Issues exactly for this reason already. Rust has the RFC process and once a feature made it through the RFC process, the majority agreed that it is a good feature people should have.

I personally think this is definitely a good feature, because it makes the language easier to write for beginners and therefore lowers the level of entry. Pretty much the first reason @skade brought up.

The pattern_type_mismatch lint

If I agree that Clippy isn't a tool for doing such things, why did I allow this lint to get into Clippy then?

I didn't see this lint as "it should disable this feature" but rather as a lint that can help teaching what actually happens when writing a match expression. And the lint in its current state does this really well IMO.

From the start it was clear that this lint should go into the restriction group and is not meant for broader use. You can enable it, if you want to teach someone about the intricacies of pattern matching in Rust. Or, if for some reason, your codebase has a correctness reason, why you cannot use default binding modes. I doubt that this happens often though.

@phaylon put much work into the suggestions and the documentation to explain on a beginner level, how bindings work and what should be added to always adhere to the underlying type. This is also something I put a really high bar on, since I would definitely have disagreed with adding a lint that just says "this uses default binding mode!".

That being said, if you want to enable this lint in your codebase, I can't stop you from doing so. But I also won't help you explain newcomers, why they aren't allowed to use a convenience feature in your codebase.

The restriction group

I don't want to go into details here and open a big discussion about Clippy lint groups. We want to write a Clippy book this year, where we can discuss the definitions of groups in more detail (internally and with the community)

I just want to rehash what @Manishearth already said about the restriction group with my own understanding and my own words. This is not a group where you can just open a PR with any lint that you care about and it will get automatically merged, because "it's just restriction".

The restriction group is for lints, that only apply in really specialized codebases with a specific goal in mind. And as the group name says, they will "restrict" the language in a way, that does not make sense for almost all codebases. Otherwise it would be pedantic or even in one of the warn-by-default groups.

As for this lint: if you do a workshop on Rust and want to make an example crate about the intricacies of pattern matching, using this lint might help you. In any other case, don't use this lint.

(Clippy has a lint, that warns-by-default if you write #[warn(clippy::restriction)])

14 Likes
31

Yes, though I definitely do recall seeing some of that kind of mobbing behavior around this when the original feature landed. But my rejection of the original clippy issue was not related to whether or not there was mobbing behavior.

2 Likes
33

I'm not saying, it should be used to teach a different style, but rather that it can be used to better explain pattern matching and how it works under the hood.

I agree, but this also goes for every other lint group of Clippy. We'll focus on making things like this more clear cut in Clippy (according to our roadmap). This will also include to revisit already existing lints and re-classify/remove them (tracked here: [Roadmap] Lint groups · Issue #6626 · rust-lang/rust-clippy · GitHub, not started).

I strongly disagree here. Clippy is a tool that should be used especially by beginners, so adding something addressing the needs of beginners is a strong argument IMO (though it should be added as allow-by-default).

But this isn't exactly the argument for this lint. This lint is rather targeted at experts, that want to show beginners how pattern matching works in Rust under the hood. And I think this lints actually illustrates, why you want to have default binding modes: ergonomics.

Rust beginners, who want to get a deeper understanding on how the language works on real code examples. I myself really started using Rust when default binding modes were already a thing, so I didn't know how exactly pattern matching worked and thought it was just magic. This lint would've helped me understand what's going on.

I probably should have done this. But as stated before, the discussions about this all took place before I was involved in the Rust ecosystem, so I was unaware of this. IMO this lint doesn't hurt to have as a restriction lint. From the Clippy README.md:

The lint list also contains "restriction lints", which are for things which are usually not considered "bad", but may be useful to turn on in specific cases. These should be used very selectively, if at all.

I think this applies for this lint.

3 Likes
34

Just to be clear: I fundamentally question the reasoning that this method works on beginners and there was no good case made for this.

35

I really appreciate this capacity of linting... to teach myself, let alone others!

In general, I wish there was a flag that would highlight how various features of the compiler “are working for me”. I suspect that understanding/reminding accordingly would accelerate my seeing how the same rules are at play always. E.g., the conceptual jump required to learn the move semantics of a primitive vs other types.

That is Rust 101, however, I suspect a “Notice feature x flag” would be useful for more advanced concepts. In other words, not a message with a “good or bad” connotation that comes with Error or Warning, but more of an FYI.

1 Like
36

Personally, I'm disappointed in the premise of this argument.

I don't have a huge stake in the outcome. Admittedly, I never liked default binding modes. But I'm not interested in using this lint – in part because I appreciate the benefits of a shared community code style, in part because, well, I don't normally use Clippy at all.

But I believe that I should have the choice of what code style to enforce in my own code. Even if the restriction I want to add is utterly distasteful, even if it seems to makes code both more confusing and more bug-prone – yes, even if it's something like this, it's ultimately a subjective decision I should get to make.

After all, how do we justify code style rules? Usually, it's in large part on aesthetic grounds, which are in the eye of the beholder. Beyond that, they may claim to increase readability or prevent bugs, but that depends on the person reading the code or writing the bugs. There may be some way to empirically measure their effectiveness… but only at a large scale, with great difficulty. Nobody is doing that.

And when the objection to a rule is that it's relitigating a settled issue… well, perhaps I should stick with the community consensus if I want to contribute code to the community, but sometimes I just want to write code for myself.

You might retort: "Sure, feel free to write your own personal linter and enforce whatever rules you want. But Clippy maintainers aren't obligated to write your lint, or maintain it."

True, but Clippy is in a unique position. It has a sort of privileged access to rustc, in the sense of being allowed to use unstable compiler APIs on stable releases. If I wanted to write my own linter, I would have to use it with nightly builds. Personally I don't mind that, but clearly quite a lot of people do. It seems unfair to force people to do that just because they disagree with the Clippy maintainers' judgement.

I guess that if I were in that situation, I could propose that my hypothetical alternate linter be included in rustup as its own separate component. But that seems pretty silly. Such a proposal would likely be rejected, entirely reasonably: there aren't many rustup components, and this doesn't clear the bar for adding a new one.

Perhaps someday this can be solved by rustc having a stable analysis API. In the mean time, I personally think Clippy should be more willing to accept PRs despite policy disagreements, as long as they have high technical quality and don't present a large maintenance burden.

15 Likes
37

If you create your own lint tool it will have the same access to rustc as Clippy. And you can also use your own lint tool to check crates compiled with stable rustc. The only difference is that it will not be shipped by rustup, but rather has to be installed with cargo install like for example the before mentioned cargo-inspect.

Even though it is very rare, lints (requests) or changes to lints were rejected because of this very reason and this will also be a reason to reject lints in the future if there is a really good argument to do so.

1 Like
38

cargo-inspect does require nightly, though from a brief inspection it doesn't actually use rustc APIs like Clippy does. All it does is run rustc as an external command with -Z unpretty=hir or another -Z unpretty option, then perform some text processing on the result (diffing and syntax highlighting). It requires nightly because -Z options aren't available on stable.

A third-party tool can't directly access a stable compiler's APIs like clippy can, because they are gated on #![feature(rustc_private)] (which Clippy uses).

Well, it can access them if you set RUSTC_BOOTSTRAP. Or the documentation could ask users to build with stable but lint with nightly.

Perhaps those options are good enough. Honestly, they both sound decent to me. But the same options existed for Clippy itself before it started being distributed with rustup. The standard advice was, "If you're worried about nightly being unstable, it's still fine to use it for Clippy, since you're only running that locally. You can still use stable to generate the binary you actually ship." But for whatever reasons, users didn't like those options, and there was much celebration when Clippy was finally available on stable.

One of those reasons, especially for less experienced Rust users, was simply the stigma of using nightly at all. That said, I suppose there were also concrete usability issues which Clippy could have solved another way (and which an alternate linter could solve another way). To quote the announcement of clippy being added as a rustup component:

No more nightly updates that break clippy. No more manually cargo install clippy.

Breakage from nightly updates would be less of a concern for a tool that only had a few lints. On the other hand, it might be more practical for the tool to be a fork of Clippy with added lints. It's already annoying to compile code twice, once with rustc and once with clippy-driver (which is why I don't use it). Three times would be worse, so it would be better to combine Clippy's lints and the extra lints. If the tool were a fork of Clippy, though, it would probably have similar issues with frequent breakage. But I can imagine ways to avoid this, like pinning to specific nightly versions and having a custom install script that used rustup to grab the correct nightly before doing the equivalent of cargo install. Or the lint tool could just distribute its own binaries, some way or other.

As for cargo install, well, it is painful to deal with, but that's a much broader issue that affects a large number of tools, so I suppose I can't complain too much.

Overall, overcoming those issues without rustup sounds feasible but difficult, requiring significant commitment to create a new linter. (Which, again, I'm not interested in actually doing; I'm only speaking hypothetically.) It would still have either the stigma of nightly or the stigma of using RUST_BOOTSTRAP, but maybe that's not the end of the world.

In short, upon reflection, my point about Clippy being in a unique position may have exaggerated the truth. But I do believe that it's true to some extent.

1 Like
39

I strongly disagree with this direction of argument. Yes, clippy is in a somewhat unique position, but I don't think that this is at all a good enough reason to relax any polciies. Rustc is also in the unique position of being able to add features to the language, this does not mean that we should lower the bar for new features being added just because it is a super high bar to maintain your own rustc with your own features.

Put a different way, I do not think solving every problem everyone has is a goal for any Rust team. This is an issue a lot of open source software has -- it tries to please everyone and ends up pleasing no one. It's important to carefully scope what you plan to do.

In open source there's this misconception that "openness" means pleasing everyone. It doesn't, I've written about this before. You have to draw the line somewhere, and carefully scoping your product and your processes is a part of that.

Rust is overall slowly moving towards thinking of itself more as a product, and as such as time passes we're going to be spending more time carefully scoping what each tool/team is and isn't for. Right now it's somewhat ad-hoc, but such scoping already does exist and just needs to be codified.

12 Likes
40

However, there is a difference between rustc and clippy. In rustc, most of the questions need to have one answer, and then the different approaches need to be weighed against each other. Lints, on the other hand, are purely additive: adding a new lint doesn't affect anybody who's not using it, and if it's disabled by default, then it doesn't affect anybody who doesn't look for it. In cases which behave similarly, such as adding new library functions, rustc is also much more lenient.

Of course, this still doesn't mean that any lint should be added, but if there's enough people interested in it, that's probably a good enough reason. Especially if they're willing to write that lint. (A counter-argument could be the maintainance burden, but I haven't seen anybody raise that here).

I guess the bottom line is the question, what is the purpose of Clippy? To guide people towards the One True Style decided upon by the community and the Clippy team, or to help people enforce their favourite style in their codebase?

3 Likes
41

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.