Added restriction lint: pattern-type-mismatch #4841

phaylon · 2019-11-23T19:58:44Z

changelog: Added a new restriction lint pattern-type-mismatch. This lint is especially helpful for beginners learning about the magic behind pattern matching. (This explanation might be worth to include in the next changelog.)

phaylon · 2019-11-23T19:59:17Z

Hello,

I'd like to propose a new off-by-default restriction lint called pattern-type-mismatch.

Description

The purpose of the lint is to enforce that patterns accurately represent the types they
are applied to.

As far as I'm aware, this would currently only be triggered when default binding modes
are in effect for a particular pattern. Although it is possible that this should include
some future developments, like patterns applying transparently through Deref implementations.

I have avoided mentioning default binding modes in the name and the description. Due to past
controversies I felt it would be benficial to stay away from the specific feature, and focus
on the actual guarantees the lint is supposed to provide. I hope that pattern-type-mismatch
is a good name to achieve that goal.

By going this route, it should be easy for people who are looking for these guarantees to
find them. Yet, since default binding modes aren't mentioned, noone should be inclined to
activate the lint simply because they vaguely remember past discussions.

Motivation

The lint allows enforcing the following code properties:

All pattern specifications reflect the actual types they are applied to, and give some
hints of the ownership, mutability, and drop semantics of the specified types and their
contents.
Bindings for values that aren't moved or copied are explicitly bound via
ref and ref mut. This explicitly communicates their mutability which can help
during refactoring and review.
Code can be made less adaptive to API changes. This includes things like dropping
of values, side effects of modifications, and switching between shared and mutable
access.

Details

The lint would guard the following language constructs:

let
match
if let
while let
for
function signatures and closure parameter specifications

The suggestion to the user takes the following form:

It highlights the pattern that doesn't exactly match the type.
It suggests using &_ and &mut _ in the patterns and to adjust the resulting bindings.
It will begin the hint with suggesting dereferencing the matched expression with * if
the syntax construct allows it, as per the default idiomatic style.

cargo fix

On the surface, it seems like this should be automatable. Some of the bindings would stay
as they are copied. Others would have to be modified to include ref or ref mut as necessary
by other code. I assume this would be hard to make reliable.

I'm also of the opinion that since one purpose of the lint is to give ownership hints in matched
structures, the hinted code should be reevaluated. For example, usage of ref instead of ref mut
bindings, and mutation based operations can be evaluted. I would see the potential for this
reevaluation as a further purpose of the lint by itself.

Given that, I would argue that this should not be automatically fixed.

Some Questions

As this is my first lint, I'm unsure about a few things:

Is cx.tables.liberated_fn_sigs() the right way to get to a functions effective signature?
Am I using is_external_macro correctly?

I'm sure there are other things I'll need to fix as well.

The implementation works by recursing through the involved types and patterns, and looking
for & and &mut types that don't have corresponding patterns. I think there shouldn't be
false positives due to how this works. I've been wondering though: Is there a good way to
run a clippy checkout against some other unrelated codebase without interfering with the
rustup version?

phaylon · 2019-11-23T20:29:39Z

It seems CI fails because the .stderr file I committed has 278 lines, but there is a 275 limit. The bigger number of lines comes from testing the different syntax constructs that involve patterns.

I'm unsure how to proceed.

phaylon · 2019-11-24T20:41:54Z

I upped the STDERR limit to a round 300 for now.

I'm having trouble understanding the new failures though. It seems some ui tests failed even though they are untouched by this PR. I managed to replicate them locally once, but after some branch switching for comparison and rerunning it seems they disappeared (I did a cargo clean run).

flip1995 · 2019-11-25T14:25:21Z

The additional failures are addressed in #4846. Don't worry about them.

For the limit of the stderr files: We want to bring the limit even further down: #2038. You can create a new dir and split up the tests in multiple files.

flip1995 · 2019-11-25T14:36:49Z

Is there a good way to run a clippy checkout against some other unrelated codebase without interfering with the rustup version?

$ cargo +master install --force --debug --path .
# checkout a crate and `cd <crate>`
$ LD_LIBRARY_PATH=$(rustc +master --print=sysroot)/lib cargo clippy -- --Wclippy::pattern_type_mismatch

flip1995

I really like the concept of the lint and think, that this could really help Rust beginners to understand how pattern matching in Rust works. I'll try to do a in depth review ASAP.

phaylon · 2019-11-25T14:58:09Z

@flip1995 Very helpful, thank you!

I'll have a go at splitting up the STDERR files and reset the limit down to 275.

flip1995 · 2019-11-25T15:01:09Z

I'll have a go at splitting up the STDERR files and reset the limit down to 275.

That would be great. I think a good split would be into the different syntax constructs. Make sure to add tests that should trigger the lint and tests that shouldn't trigger it.

phaylon · 2019-11-25T17:01:31Z

I've split up the test file into a subdirectory. I tried to use the opportunity to reorganize the tests themselves to be a bit more easy to follow.

The different kinds of syntaxes are actually now in one file, as the tests there can be rather simple. Instead, I've split it up the different pattern constructs (structs, tuples, alternatives). That's the complex part of the actual lint I feel, and has more variation due to the different forms types can have.

I also removed some probably redundant tests (functions vs associated functions) that I had added more for the purpose of discovering the compiler's API and ensuring my assumptions hold.

Edit: I also organized it so that sections that trigger the lint and sections that don't are next to each other. And all linted constructs should now also have examples that are not linted against.

bors · 2019-11-28T09:11:56Z

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

phaylon · 2019-12-04T12:42:15Z

A procedural question: Should I keep rebasing the PR or would the forced pushes interfere with reviewing?

flip1995 · 2019-12-04T13:41:19Z

You can keep rebasing it. It doesn't make a difference for reviewing. On the other hand, once I get to reviewing this I can ping you, so you don't have to rebase every other day.

phaylon · 2020-01-22T18:24:39Z

Squashed, rebased and caught up with recent changes in master.

bors · 2020-01-29T17:07:24Z

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

phaylon · 2020-02-09T17:05:47Z

Pushed some changes to catch up to master. There are still some errors when trying to compile, but since they're coming from another lint I assume this is a clippy/nightly divergence issue.

bors · 2020-02-10T08:39:08Z

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

bors · 2020-02-21T11:46:19Z

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

flip1995

I finally got to take a look at this. Sorry or the way to long wait time...

I think this is especially a great lint for newcomers to learn pattern magic and the rust magic behind it. That being said, I also think the message use *to dereference the match expression or explicitly match against a&_ pattern and adjust the enclosed variable bindings is too generic for newcomers to understand and apply. It would be great, if this message could point to where this borrows or derefs should happen. Something like this:

error: type of pattern does not match the expression type
  --> $DIR/mutability.rs:9:9
   |
LL |         Some(x) => (),
   |         ^^^^^^^
   |
   = help: use `*` to dereference the match expression
   |
LL |     match *expr {
   |           ^^^^^
   |
   = help: or explicitly match against a `&_` pattern...
   |
LL |         &Some(x) => (),
   |         ^^^^^^^^
   |
   = help: ...and adjust the enclosed variable bindings
   |
LL |         Some(&x) => (),
   |              ^^
   = help: ...also here  // add more hints recursively 
   ...

I can see, that the recursive suggestion generation might be too much to ask. But I think, this lint would strongly benefit from at least building the suggestion for the top level deref or the & match. You can optionally add suggestions/help messages with span_lint_and_then(.., |db| { db.span_{help,suggestion,...}). See also the DiagnosticBuilder documentation. Recently I implemented similar suggestion generation here:

rust-clippy/clippy_lints/src/types.rs

Lines 686 to 693 in 5924fa6

    
           fn lint_unit_args(cx: &LateContext<'_, '_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) { 
        
               let mut applicability = Applicability::MachineApplicable; 
        
               let (singular, plural) = if args_to_recover.len() > 1 { 
        
                   ("", "s") 
        
               } else { 
        
                   ("a ", "") 
        
               }; 
        
               span_lint_and_then(

clippy_lints/src/pattern_type_mismatch.rs

phaylon · 2020-04-05T14:30:05Z

@flip1995 Apologies for taking a while.

I can see where the expanded diagnostic including bindings can be useful. I think expanding it for nested patterns or those with many bindings might make things less clear. There are advantages to the extra information when the lint is used as a teaching utility, but outside of that it would potentially be a very long version of a type mismatch error.

Do you have a preference as to how the diagnostic should look for the cases where bindings aren't mentioned? Should it just stay the way it currently is implemented for those?

flip1995 · 2020-04-05T18:18:01Z

Do you have a preference as to how the diagnostic should look for the cases where bindings aren't mentioned?

Maybe add a configuration option for this lint: pattern-type-mismatch-verbose. If this is set, for every binding a suggestion is made. If this is not set output something like

   = help: ...and adjust the enclosed variable bindings
   = note: for suggestions on how to do this set the configuration `pattern-type-mismatch-verbose = true`

Without the configuration set, this help message should be span-less or you can just point at bindings like this:

   = help: ...and adjust the enclosed variable bindings
   |
LL |         ManyBindings(x, y, z) => (),
   |                      ^^^^^^^
   = note: for suggestions on how to do this set the configuration `pattern-type-mismatch-verbose = true`

flip1995 · 2020-04-05T18:19:03Z

As I said above, the recursive suggestion for all bindings might be too hard to implement for a fist version and we can add the configuration option and inner binding suggestions later.

flip1995 · 2020-05-25T15:35:00Z

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.

flip1995 · 2020-07-03T10:20:07Z

Thanks, this looks really good now!

Also big thanks for sticking around so long and be patiently waiting for my reviews.

@bors r+

bors · 2020-07-03T10:20:08Z

📌 Commit 77b6130 has been approved by flip1995

bors · 2020-07-03T10:20:16Z

⌛ Testing commit 77b6130 with merge ba83747...

Added restriction lint: pattern-type-mismatch changelog: Added a new restriction lint `pattern-type-mismatch`. This lint is especially helpful for beginners learning about the magic behind pattern matching. (This explanation might be worth to include in the next changelog.)

bors · 2020-07-03T10:27:25Z

💔 Test failed - checks-action_test

phaylon · 2020-07-03T11:24:05Z

@flip1995 Thanks! :D And a very big thanks to you for being so helpful!

flip1995 · 2020-07-03T15:16:32Z

@bors r+

bors · 2020-07-03T15:16:33Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2020-07-03T15:16:34Z

📌 Commit 77b6130 has been approved by flip1995

bors · 2020-07-03T15:16:41Z

⌛ Testing commit 77b6130 with merge 92dbc64...

Added restriction lint: pattern-type-mismatch changelog: Added a new restriction lint `pattern-type-mismatch`. This lint is especially helpful for beginners learning about the magic behind pattern matching. (This explanation might be worth to include in the next changelog.)

bors · 2020-07-03T15:22:53Z

💔 Test failed - checks-action_test

flip1995 · 2020-07-03T16:09:01Z

You need to remove the first LT parameter of every LateLintPass / LateContext.

phaylon · 2020-07-03T16:34:42Z

@flip1995

You need to remove the first LT parameter of every LateLintPass / LateContext.

Thanks, and done!

flip1995 · 2020-07-03T16:36:13Z

@bors r+

Thanks! Let's get this finally through 🚀

bors · 2020-07-03T16:36:14Z

📌 Commit c0fd452 has been approved by flip1995

bors · 2020-07-03T16:41:31Z

⌛ Testing commit c0fd452 with merge 57cdf2d...

bors · 2020-07-03T17:04:55Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 57cdf2d to master...

flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 25, 2019

flip1995 reviewed Nov 25, 2019

View reviewed changes

flip1995 added the A-lint Area: New lints label Nov 25, 2019

phaylon force-pushed the pattern-type-mismatch branch from fd76273 to 1378f58 Compare January 22, 2020 18:09

phaylon force-pushed the pattern-type-mismatch branch from 1378f58 to 235a035 Compare February 9, 2020 16:58

phaylon force-pushed the pattern-type-mismatch branch from 235a035 to 3010bda Compare February 10, 2020 21:19

flip1995 reviewed Feb 21, 2020

View reviewed changes

clippy_lints/src/pattern_type_mismatch.rs Show resolved Hide resolved

clippy_lints/src/pattern_type_mismatch.rs Show resolved Hide resolved

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2020

flip1995 closed this May 25, 2020

flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 3, 2020

phaylon and others added 8 commits July 3, 2020 18:12

Added restriction lint: pattern-type-mismatch

c3c4027

span_help_and_lint -> span_lint_and_help

55877d7

Adjusted expected STDERR

346ee96

Fix rebase fallout

6447507

Catching up with rustc changes

92ecc53

Expanded lint documentation

d617551

LateContext has only one lifetime parameter now

aa4bee2

fmt fix

c0fd452

phaylon force-pushed the pattern-type-mismatch branch from 77b6130 to c0fd452 Compare July 3, 2020 16:24

bors merged commit 57cdf2d into rust-lang:master Jul 3, 2020

	fn lint_unit_args(cx: &LateContext<'_, '_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) {
	let mut applicability = Applicability::MachineApplicable;
	let (singular, plural) = if args_to_recover.len() > 1 {
	("", "s")
	} else {
	("a ", "")
	};
	span_lint_and_then(

Added restriction lint: pattern-type-mismatch #4841

Added restriction lint: pattern-type-mismatch #4841

Conversation

phaylon commented Nov 23, 2019 • edited by flip1995 Loading

phaylon commented Nov 23, 2019

Description

Motivation

Details

cargo fix

Some Questions

phaylon commented Nov 23, 2019

phaylon commented Nov 24, 2019

flip1995 commented Nov 25, 2019

flip1995 commented Nov 25, 2019 • edited Loading

flip1995 left a comment

Choose a reason for hiding this comment

phaylon commented Nov 25, 2019

flip1995 commented Nov 25, 2019 • edited Loading

phaylon commented Nov 25, 2019 • edited Loading

bors commented Nov 28, 2019

phaylon commented Dec 4, 2019

flip1995 commented Dec 4, 2019 • edited Loading

phaylon commented Jan 22, 2020

bors commented Jan 29, 2020

phaylon commented Feb 9, 2020

bors commented Feb 10, 2020

bors commented Feb 21, 2020

flip1995 left a comment

Choose a reason for hiding this comment

phaylon commented Apr 5, 2020

flip1995 commented Apr 5, 2020

flip1995 commented Apr 5, 2020

flip1995 commented May 25, 2020

flip1995 commented Jul 3, 2020

bors commented Jul 3, 2020

bors commented Jul 3, 2020

bors commented Jul 3, 2020

phaylon commented Jul 3, 2020

flip1995 commented Jul 3, 2020

bors commented Jul 3, 2020

bors commented Jul 3, 2020

bors commented Jul 3, 2020

bors commented Jul 3, 2020

flip1995 commented Jul 3, 2020

phaylon commented Jul 3, 2020

flip1995 commented Jul 3, 2020

bors commented Jul 3, 2020

bors commented Jul 3, 2020

bors commented Jul 3, 2020

phaylon commented Nov 23, 2019 •

edited by flip1995

Loading

flip1995 commented Nov 25, 2019 •

edited

Loading

flip1995 commented Nov 25, 2019 •

edited

Loading

phaylon commented Nov 25, 2019 •

edited

Loading

flip1995 commented Dec 4, 2019 •

edited

Loading