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

1

I wasn't a fan of default binding modes in patterns (aka "pattern matching ergonomics"), but my previous attempt at getting a Clippy lint against this feature failed, because it was immediately rejected without proper consideration. (And I'm starting a new thread because the old one has since been closed.)

Since then, there has been plenty of evidence that pattern matching ergonomics was not received unequivocally positively. In particular, there has been a recent surge in posts on URLO which prove that:

Without further ado: could we please seriously re-consider adding an opt-in lint, as it seems to be a common source of confusion and frustration? And if the Clippy authors still l-wontfix this, can someone suggest another, reasonably easy and robust way of detecting and/or turning off this feature?

14 Likes
2

I'll be honest — I have no idea what you're talking about. Can you provide an example of what you're proposing to lint?

3

The number of posts about ref before the change was made was substantial.

I feel like your request is equivalent to adding a lint so that drop always must be explicitly called.

4

Match ergonomics allow you to write this:

match &foo {
    Some(x) => x, // x is a reference to the value in foo
    None => ...,
}

instead of

match foo {
    Some(ref x) => x,
    None => ...,
}

@H2CO3 wants an allow-by-default lint that can be enabled to warn in the first example.

6 Likes
5

I would also like such a lint. I haven't quite gotten on the Clippy bandwagon, although I've always meant to, but this lint would be highly valuable to me. I try to avoid using match ergonomics as much as possible, but it is difficult to check that I'm doing it correctly. In general, I think match ergonomics makes code less clear. But I recognize this is a subjective opinion, and I do not think its effect is severe.

13 Likes
6

I highly agree with this. Having the option of forcing your code to be more explicit about pattern matching would make it much readable in some contexts. I was suprised that the source code of std::borrow::Cow uses match *self everywhere instead of match self, where self is a &Cow<T>. And since then I am looking for a way of forcing this style.

3 Likes
7

The problem being linted being the lack of an explicit ref, I presume? I personally don't see any issue with that, but disagreement is a perfect reason to have it in clippy and not rustc, admittedly.

8

The first example you post here maybe isn't the best example, because you explain the feature as "this is kinda confusing." Of course if you tell people that it's not intuitive, they'll agree with you.

I still hold that you can teach pattern matching with default binding modes without confusion, if you start with it and don't start without. Logically, pattern matching works on the shape of your data, ignoring &/&mut, and then spits out the correct &/&mut at the end based on what you started with. This is, IMHO, still better than telling the pattern where the refs are and then also saying that you want to keep it as a reference at the end.

However, at this point I agree that this isn't the only meaningfully supported position. I have a very pattern-based understanding of my programs' dataflow, so default binding modes match that well. If you take a more "what's in memory" approach, the "sugar" of default binding modes can be quite unintuitive, since pattern matching treats reference indirection transparently.

(FWIW, I'd just like to posit that if patterns had always worked this way, it wouldn't be described as sugar, just as how pattern matching works. That's part of why I think it can be taught intuitively from first principles.)

Because of this, I'm now personally in weak agreement that a pedantic lint against binding modes other than by-value (or a terminal ref [mut] ident) is reasonable. Moreso, though, I think a tool like cppinsights to desugar Rust intelligently, including match binding modes, more granularly than cargo expand, would be a quite valuable tool for understanding how code is understood by the compiler and building intuition.

3 Likes
9

There's cargo-inspect, which displays HIR after desugaring.

2 Likes
10

I am against such a switch or a lint. While I do appreciate that some people disagree with the changes, I'd like to make some counter-points:

  • Match ergonomics are single-handedly responsible for quite some reduction in "oh, that's because" in courses that I give. The second is NLL. While I appreciate that some beginners run against it, in my experience, the vast majority doesn't. I appreciate that's just my anecdata, but as all cases brought forward are also anecdata, I will throw it into the ring.
  • I am strongly against an optional lint here, clippy or compiler level. It opens up an alternative and introduces a question into the ecosystem: "which setting for this option is better"? It essentially introduces 2 language modes. This will lead to tons of confusions, especially for beginners.
  • It also requires defining 2 rulesets for the same language edition, which will blow up the complexity of describing match statement holistically, despite coming under the flag of simplification. The future surface measure of match would at least be double.

I'm not even on the side of more compiler help, but would like to have logged that I am very convinced on the match ergonomics changes.

Also, I would like to mention that we are essentially relitigating this issue again and again.

11 Likes
11

I think my main response here is that I still, even to this day, continue to try and write code without using match ergonomics. But since I am a human and I have no plausible tool to check my work, I will occasionally write code that relies on match ergonomics. This means my code, and presumably the code of anyone else who follows this strategy, will be a weird mix of both rule sets.

I understand why re-litigation is annoying. I even understand denying this lint in the past. It's not a good idea to use Clippy to squash new language features. But I think the heat has subsided and we've all had a bit more time to reflect. Time has personally convinced me of a lot of things that I was previously skeptical of or against. But it hasn't done that for me for match ergonomics. I don't mind that match ergonomics exists. It's hard for me to say whether it's a net negative or net positive. But I personally find code written using it less clear, and it would be nice to have tooling that would help me write code that I think is clearer.

6 Likes
12

I don't understand the "this would create two different versions of the language" argument. We already have two different versions - some people use match ergonomics and some prefer not to.

I use match ergonomics all the time - I think they're a huge relief - and I'm in favor of creating this (allow-by-default) lint.

If we wanted to make the language more standardized, what we should do is create a warn-by-default lint against writing something using ref that could be expressed using match ergonomics. But I don't think that's the right approach community-wise.

EDIT: Actually, thinking about that now, maybe it wouldn't be a bad idea to have both lints available, but both set to allow-by-default?

9 Likes
13

I understand your personal point, but also would like to remind that probably the majority of Rust users nowadays don't even know Rust without match ergonomics. This is a function of the programming base growing. To them, the proposed change or lint would paint a very odd picture and only add confusion. It answers a question that they didn't even ask.

6 Likes
14

This is not what I'm saying. It puts the language into two, very clearly distinct, modes. This can be seen in 2018 already but in smaller cases (edition 2018 idioms vs. not). This is not as much of a problem, as the edition 2018 idioms lint just carries people over to a new coding style (it's uni-directional), but it still has people asking questions around why leaving off dyn might be a worthwhile thing to do.

Whenever you open up an alternative, you open up a potential for confusion. This needs to be taken into account for every proposal that puts "reduce confusion" as one of its goals.

1 Like
15

Clippy has over 400 lints. I think the argument against adding one lint because it might be confusing seems pretty weak to me.

I acknowledge this. This is the hard thing about adding alternative features that don't have universal support among the community. With dyn, for example, we've deprecated the old syntax and there is active pressure to conform to the use of dyn. There is near universal agreement, IMO, that using dyn makes code clearer. But with things like match ergonomics (or even the alternative directory hierarchy for modules), there hasn't been that sort of active pressure to move away from the old system. (Whether it's due to a lack of universal agreement or not is not actually something I don't know. But one can speculate.) So we wind up with two different ways of doing things like this. I still happen to think that the old way is better in terms of code clarity. Given that Clippy seems to be an aggregator of lints---even if they aren't universally acknowledged as good lints---it seems reasonable to me to add something like this to Clippy.

The obvious argument against my position is that as more and more people learn Rust and are taught the "match ergonomics" way of doing things, code written without using match ergonomics will become less clear to them. If that does indeed happen, then maybe my code will be clearer to me, but less clear to others. That is definitely a point against my position even using my own value system. But it's a balancing act. For example, I get PRs against my projects every now and then that fix the code to pass Clippy. But almost always, there are several transformations that I disagree with on the grounds of code clarity. But I don't disagree that the lints exist in the first place. So in practice, I end up accepting a subset of the patch.

This is why my default position is to not accept language changes like this that purport to "reduce confusion." Precisely because of the risk of now having two different ways of doing it because the community doesn't universally move to the new system. And then you're in a position where it's actually kind of hard to tell whether you're better off or not. But figuring this out is really hard and I've certainly been wrong.

But I suppose we are getting a bit off topic now. Maybe this perspective helps though. I don't know.

14 Likes
16

Clippy already has a large collection of 'restriction lints', which are exactly the kind of optional lint that you're describing: Clippy Lints

10 Likes
17

From the point of view of language specification and formal description of semantics, I am pretty sure the original match behaviour is the most natural and that's the reason it came first and had to be enhanced. I have yet to see an explanation of the default binding mode mechanism that does not boil down to desugaring it to the original match semantics, regardless of how hard the explanation tries to hide it.

Also, please understand that people think differently depending on their background and that some find the original match semantics quite natural and use it to extract information from a program source that's not as easy to get when using default binding modes.

EDIT: example of confusion Using default binding modes, both `Option::as_ref` and `Option::as_mut` can be given the following body. 
match self { Some(x) => Some(x), None => None }

However, neither of these methods is the identity and you can't simplify the match expression to return self. The original match semantics make the source show why (as can still be seen in the current implementation of these methods).

(It is in discussion like this that I think programmers should edit ASTs so that each could apply their own display preferences.)

3 Likes
18

I'd like to suggest that this thread will likely be more constructive if it carefully avoids rehashing arguments for or against "default binding modes". The proposal that started this thread was a request for a lint, not a request to change the default or to change how we teach Rust.

The existence of restriction lints in clippy does indeed make an excellent argument for this kind of thing. Such lints exist to help people enforce preferences on a codebase, and in particular, often to provide extra compiler guardrails to help people.

For instance, there's an allow-by-default lint for as conversions, so that people can use a different kind of conversion; that's useful for code that might otherwise do many such conversions, to steer towards safer alternatives. We have the default_numeric_fallback lint, which will lint against let x = 5 without specifying a type; that seems like a similar "lint against compiler inference and make the code be more explicit". We have clippy lints for AT&T vs Intel asm syntax, so that projects can enforce a single preference. We have lints that will trip if you ever call x + y rather than using a method like wrapping_add or checked_add; obviously many codebases will not want that.

None of these lints cause any kind of damage or bifurcation in the ecosystem. It's uncommon to even come across a project that uses them, but the projects that use them are grateful for the additional compiler support.

"Default binding modes", in particular, seem to provoke very strong reactions in many people. As far as I can tell, people who have argued against the lint when it has come up (in several threads, not just this one) are not making an argument about their own code; they're making an argument about other peoples' code, and in particular, arguing that tools should not make it easier to write such code even if people want to. That implies we're actively trying to discourage such code.

We do, sometimes, take steps to deprecate and discourage certain code patterns, and sometimes eventually remove them when there's a good reason to do so. When we do that, we provide lints against them, and migrations towards preferred approach, and similar. We use editions to make sure that old code still works, but we steer people towards one approach or away from another.

But we haven't done that for "default binding modes". It's been proposed, and the lang team discussed it; the current sentiment is not in favor of doing so. (For context, I was one of the people who argued against doing so, but not the only person.) The option to write code that doesn't use default binding modes is not going away.

I think if we take the arguments against the lint to be strong enough to decline to add such a lint, those arguments would also support deprecating non-"default binding modes" patterns (e.g. ref).

And conversely, I think if we're not deprecating non-"default binding modes" patterns, there's no fundamental reason we shouldn't add a lint to support people who wish to use them.

28 Likes
19

I would like to give some counter arguments to the points made by @skade.

Those "settings" already exists in the grammar of language. You will either ask yourself "which setting for this option is better" everytime you write a match expression, or you will just stick to one style. Or, arguably worse, you will mix and use both of them. We propose an optional lint for not having to think about that question and strictly following one style.

Both of them are currently a part of the language model described in the reference. We don't propose a new semantic or syntax.

Since the proposed lint is allow by default, I doubt any beginner will encounter this lint unless they read other people's code, at which point every lint you came across has the potential of confusing you. Because all lints are about the semantics of the language, and you need to know the relevant semantics if you want to understand the lint. This sentence seems like it supports the idea of "pattern matching ergonomics are confusing" in that regard.

6 Likes
20

While I appreciate your experience concerning beginners, I don't see why they should be the only group being considered here. In general, I wish the Rust community gave more support and more consideration to advanced programmers.

This lint is obviously not aimed at people who have a hard time understanding the concept of references, ownership, and pattern matching, let alone sheanigans like ref. Rather, it is aimed at people who are either:

  • trying to gain a deeper understanding of the language itself, while learning advanced concepts of lower-level programming for the first time in the context of Rust, or
  • at those who are already familiar with strongly-typed pattern-matching languages, and wonder why type checking in Rust behaves inconsistently with regards to patterns (i.e. why can you treat a reference as its referred type).

This is true for all lints, so it is a non-argument.

6 Likes