Skip to content

single_match fires on same code with 2 conflicting suggestions in macro #5359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
matthiaskrgr opened this issue Mar 23, 2020 · 4 comments · Fixed by #5397
Closed

single_match fires on same code with 2 conflicting suggestions in macro #5359

matthiaskrgr opened this issue Mar 23, 2020 · 4 comments · Fixed by #5397
Labels
C-bug Category: Clippy is not doing the correct thing good first issue These issues are a good way to get started with Clippy I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied T-macros Type: Issues with macros and macro expansion

Comments

@matthiaskrgr
Copy link
Member

Perhaps macros should be ignored?

One suggestion contains make_value_visitor!(MutValueVisitor, mut); while the other one just contains make_value_visitor!(MutValueVisitor, ); (which probably does not even compile)

    Checking rustc_privacy v0.0.0 (/home/matthias/vcs/github/rust/src/librustc_privacy)
warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> src/librustc_mir/interpret/visitor.rs:183:17
    |
104 |  / macro_rules! make_value_visitor {
105 |  |     ($visitor_trait_name:ident, $($mutability:ident)?) => {
106 |  |         // How to traverse a value and what to do when we are at the leaves.
107 |  |         pub trait $visitor_trait_name<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>>: Sized {
...    |
183 | /|                 match v.layout().ty.kind {
184 | ||                     // If it is a trait object, switch to the real type that was used to create it.
185 | ||                     ty::Dynamic(..) => {
186 | ||                         // immediate trait objects are not a thing
...   ||
196 | ||                     _ => {},
197 | ||                 };
    | ||_________________^ help: try this: `if let ty::Dynamic(..) = v.layout().ty.kind { make_value_visitor!(ValueVisitor,); }`
...    |
244 |  |     }
245 |  | }
    |  |_- in this expansion of `make_value_visitor!`
246 | 
247 |    make_value_visitor!(ValueVisitor,);
    |    ----------------------------------- in this macro invocation
    |
    = note: requested on the command line with `-W clippy::single-match`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match

warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> src/librustc_mir/interpret/visitor.rs:183:17
    |
104 |  / macro_rules! make_value_visitor {
105 |  |     ($visitor_trait_name:ident, $($mutability:ident)?) => {
106 |  |         // How to traverse a value and what to do when we are at the leaves.
107 |  |         pub trait $visitor_trait_name<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>>: Sized {
...    |
183 | /|                 match v.layout().ty.kind {
184 | ||                     // If it is a trait object, switch to the real type that was used to create it.
185 | ||                     ty::Dynamic(..) => {
186 | ||                         // immediate trait objects are not a thing
...   ||
196 | ||                     _ => {},
197 | ||                 };
    | ||_________________^ help: try this: `if let ty::Dynamic(..) = v.layout().ty.kind { make_value_visitor!(MutValueVisitor, mut); }`
...    |
244 |  |     }
245 |  | }
    |  |_- in this expansion of `make_value_visitor!`
...
248 |    make_value_visitor!(MutValueVisitor, mut);
    |    ------------------------------------------ in this macro invocation
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match
@matthiaskrgr matthiaskrgr added C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied labels Mar 23, 2020
@flip1995 flip1995 added good first issue These issues are a good way to get started with Clippy T-macros Type: Issues with macros and macro expansion labels Mar 23, 2020
@pmk21
Copy link
Contributor

pmk21 commented Mar 29, 2020

New to Rust here. Want to contribute though! Mind if I take up this issue?

@flip1995
Copy link
Member

It's all yours!

@pmk21
Copy link
Contributor

pmk21 commented Mar 30, 2020

Could use a little help on where to start and where to look 😄

@flip1995
Copy link
Member

flip1995 commented Mar 30, 2020

This is a good first issue, since you basically only have to do 3 things:

  1. Write a test for this. This is done by adding a macro here

    fn main() {}
    and use it in a function. You know, you successfully added the test, if the single_match lint triggers on the macro use, when executing TESTNAME=single_match cargo uitest.

  2. Add a macro check to the lint. You can search clippy_lints for SINGLE_MATCH and add a if in_macro(expr.span) { return; } where you think it fits. You successfully resolved this issue, when running TESTNAME=single_match cargo uitest succeeds without linting the macro case.

  3. While you're at it, also SINGLE_MATCH_ELSE should not trigger in macros. This lint is tested here https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/single_match_else.rs

To get started with Clippy this document is also a good read: https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md (For this issue especially the "Setup", "Testing" and "Cheatsheet" sections)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good first issue These issues are a good way to get started with Clippy I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied T-macros Type: Issues with macros and macro expansion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants