Rust-clippy: Restriction Lint: Literal Patterns

[phaylon]

It can sometimes be desirable to not use the default binding modes, but instead write the patterns to literally match the types they are matching. Example:

match *state {
    State::A { ref left_stream, ref mut right_stream } => { ... },
    ...
}

instead of

match state {
    State::A { left_stream, right_stream } => { ... },
    ...
}

Situations I ran into where this would help are:

• Splitting structures and variants into mutably and immutably referenced parts.
• Extracting copyable parts from referenced structures and variants.
• Combinations of the above, both at the same and different levels.

In all these cases a literal pattern match has the information more locally available:

• ref mut x instead of x and somewhere later let x = &*x to limit mutability.
• x instead of x and later *x for copying values out. This also prevents accidental later mutation when x is a &mut _.

More specifically, this would be useful for complex state machines and event processing decisions where you'd want ownership and mutability to be at their most self-documenting level. A restriction lint can be used to catch accidental adaptive matching from oneself or outside contributors.

The code making me hope for such a lint was a stream based event processing library where many nested parts kept individual but sometimes also semantically connected state. Unwanted mutations and non-evident copy semantics for flags and counters were the biggest time sinks when debugging, since the problems only became visible in edge cases once the whole stream was processed and combined into a whole.

Having these markers at the point of destructuring was a big help discovering these problem causes and I'd love for a way to uphold the information-locality for those code parts when multiple contributors are working on code.

[oli-obk]

If all patterns in a match on a &T value are getting dereferenced at all use sites (and are Copy), then clippy can lint to suggest not dereferencing any of them but instead dereferencing the match value.

If all patterns in a match on a &mut T are used in an immutable way, then we should suggest to limit the match value directly to an immutable reference by reborrowing as immutable.

The rest of your lint suggestion only makes sense to me as a lint forbidding any binding of (mutable?) reference type. You have the exact same problem if someone writes let foo = x.some_fn_returning_mut_ref(); and you can't tell from the foo whether you can mutate something from it. I don't think forbidding named (mutable) references in general is practical, especially since it would not catch Option<&mut T> and structs with fields of &mut T type.

If you are doing let x = &*x somewhere in a match arm, you already know what's going on and could instead just deref the match value, right?

[phaylon]

@oli-obk

If all patterns in a match on a &T value are getting dereferenced at all use sites (and are Copy), then clippy can lint to suggest not dereferencing any of them but instead dereferencing the match value.

I agree.

If all patterns in a match on a &mut T are used in an immutable way, then we should suggest to limit the match value directly to an immutable reference by reborrowing as immutable.

I also agree here.

But that doesn't cover mixed bindings.

The rest of your lint suggestion only makes sense to me as a lint forbidding any binding of (mutable?) reference type. You have the exact same problem if someone writes let foo = x.some_fn_returning_mut_ref(); and you can't tell from the foo whether you can mutate something from it.

I would disagree with this because:

• That issues is something one has to combat with defensive programming in other areas, like function names and interfaces, and locally with variable names.
• The fact that the problem can't be solved 100% by this lint doesn't mean that solving any part of it at all isn't worth it.

I don't think forbidding named (mutable) references in general is practical, especially since it would not catch Option<&mut T> and structs with fields of &mut T type.

I didn't suggest that. Like I said above, that problem is more something for naming in APIs, which isn't easily lintable.

If you are doing let x = &*x somewhere in a match arm, you already know what's going on and could instead just deref the match value, right?

I'm not sure I understand you here. The issue is you're having a &mut Foo containing multiple parts. If you want to mutate some and not others, you'd reborrow. Do you mean the reborrow should be enough as a hint that the pattern should be changed? The problem here I see is that &* is an easy to skip marker. Also, in a restricted setting, you'd have to combine ref mut foo with &*foo which makes that fact more evident. (And that could be caught by a "uselessly mutable" lint)

[oli-obk]

Do you mean the reborrow should be enough as a hint that the pattern should be changed?

yes

That issues is something one has to combat with defensive programming in other areas, like function names and interfaces, and locally with variable names.

So the suggested restriction lint should catch

• all bindings of &mut T type
• all bindings of a type with a field of &mut T type

Since function arguments are also bindings, the lint will also trigger on mutable references to functions. This means you are getting into very functional territory, because you are essentially banning non-local mutation. I can totally get behind that, but we need to discuss the implications.

I suggest you open three issues: one for each of the general lints and one for the functional restriction lint.

[phaylon]

You've lost me there. I don't think I've ever used a "mutable reference to a function". Do you mean closures? I don't understand how this hangs together.

I'm after a way to locally require that patterns specify how they bind, instead of adapting to whatever type of value they got.

[oli-obk]

Sorry, that was badly worded. I meant a function having an argument of &mut T type.

Patterns are not special. The reason we have match ergonomics now is because everything else was already behaving this way. So either all bindings of mutable reference type are problematic or none. I don't see how a match pattern binding is different from any other binding.

[phaylon]

@oli-obk
Sorry for the late reply, was a bit under the weather.

I agree it would be great if there were ways to combat that. I feel it's different for me because it's a very specific scope where this lint would be helpful for me. This is basically for when I have big state machines taking things apart with ref and ref mut bindings and some immediately copying out, with nested levels of match expressions.

Using an explicit matching mode makes it easier to see those semantics of the event processor (or whatever). The lint's primary function would be to enforce consistency inside a fn for that.

So, even if we had a lint that tells me that I should name a field, variable, type, or function better to indiciate ownership transfers and mutable access, I'd still want to have a lint like this for complex matching logic functions (they're usually big but mostly do matching) just to keep consistency for how things are taken apart in that logic.

[durka]

Since default binding modes have been shown to easily trigger soundness bugs, linting them seems like not the worst idea ever.

[oli-obk]

Linting rustc bugs is out of scope for clippy. The effort is better spent fixing rustc.

The way to resolve this issue is to lint all bindings of &mut T type which were not created by a ref mut pattern

[taiki-e]

This seems implemented as clippy::pattern_type_mismatch.