Stream: t-compiler

Topic: #54538 `unused_patterns` lint


nikomatsakis (Oct 01 2018 at 16:33, on Zulip):

@Kevin Leimkuhler hey =)

nikomatsakis (Oct 01 2018 at 16:35, on Zulip):

So you asked:

Do you have any pointers on how to get a value: &ast::Expr from this situation that I can pass on to check_unused_parens_core to indicate that a PatKind::Wild should not need parens?

I think the answer is that you cannot =) patterns are not expressions, at least in Rust's HIR, and you can't readily interconvert. We'll have to make the logic in check_unused_parens_core either generic over both patterns and types or else make a distinct copy

nikomatsakis (Oct 01 2018 at 16:35, on Zulip):

I don't really remember how general that logic is, tbh

nikomatsakis (Oct 01 2018 at 16:35, on Zulip):

a lot of times we can make logic generic over things by making it just operate on a NodeId or HirId

kleimkuhler (Oct 01 2018 at 18:46, on Zulip):

Ok that makes sense. So I think it could be reasonable to make check_unused_parens_core generic over both patterns and types, since the fields that it uses from ast::Expr already exist in ast::Pat (node and span)

kleimkuhler (Oct 01 2018 at 18:52, on Zulip):

Since this is my first time really digging into the Rust source code, I think I may work towards a distinct copy that works with the specific case pointed out in the issue. I was able to speak about other patterns with @scottmcm over Discord, and think that as I make the check more robust I can look into generalizing check_unused_parens_core.

nikomatsakis (Oct 01 2018 at 18:56, on Zulip):

@Kevin Leimkuhler sounds reasonable

kleimkuhler (Oct 02 2018 at 18:20, on Zulip):

@nikomatsakis I have a minimal working implementation that properly warns on the example in the issue: https://github.com/rust-lang/rust/issues/51087#issuecomment-397918200.

I have essentially copied check_unused_parens_core into check_unused_parens_expr and check_unused_parens_pat; the differ only in how they handle value (either ast::Expr or ast::Pat).

I'd like to ideally make check_unused_parens_core generic of the type of value, but ast::Expr and ast::Pat don't share an obvious parent struct to match on.

kleimkuhler (Oct 02 2018 at 18:21, on Zulip):

Would it make sense to consider making check_unused_parens_core generic of a value type T where T implements Clone, RustEncodable, and RustDecodable (the traits that ast structs implement) and match from there?

kleimkuhler (Oct 02 2018 at 18:22, on Zulip):

If it helps clarify, here is the commit I'm currently working off of: https://github.com/kleimkuhler/rust/commit/9229b025b993bcc6892c0afa1a0ac834005cecea

nikomatsakis (Oct 02 2018 at 18:30, on Zulip):

I don't think RustEncodable and RustDecodable makes much sense; it might not be worth trying to collapse it

nikomatsakis (Oct 02 2018 at 18:31, on Zulip):

@kleimkuhler it seems like we could extract most of the logic from that fn into a helper anyway

nikomatsakis (Oct 02 2018 at 18:31, on Zulip):

we could certainly make this generic in various ways

nikomatsakis (Oct 02 2018 at 18:31, on Zulip):

one would be to supply closures -- there are basically only two things we do: "check if this is parens" and "pretty print"

nikomatsakis (Oct 02 2018 at 18:31, on Zulip):

or we could make a little one-off trait and implement it for hir::Expr and hir::Pat

nikomatsakis (Oct 02 2018 at 18:32, on Zulip):

(with those two methods)

nikomatsakis (Oct 02 2018 at 18:32, on Zulip):

or we could just have two functions but share the "guts" — basically starting from the point where we pretty-print

nikomatsakis (Oct 02 2018 at 18:32, on Zulip):

at which point, the function seems to just be operating over spans and strings

kleimkuhler (Oct 03 2018 at 04:50, on Zulip):

@nikomatsakis Great, those are definitely all good starting points I can work from. I think there may need to be three things covered - the two you mentioned plus a necessary check (ast::Pat does not have an equivalent parser::contains_exterior_struct_lit(&inner) so I just simplified it to if !struct_lit_needs_parens?

Either way for the issue this is solving I lean towards just sharing the "guts". A generic solution seems like an interesting path I can explore after it's ready to go, but seems maybe a little unnecessary right now. Do you agree or would you a generic solution put in place for this?

nikomatsakis (Oct 03 2018 at 16:35, on Zulip):

I'd prefer to share the guts

nikomatsakis (Oct 03 2018 at 16:35, on Zulip):

if that works out

nikomatsakis (Oct 03 2018 at 16:35, on Zulip):

generics are great sometimes but other times kind of obfuscating

nikomatsakis (Oct 03 2018 at 16:35, on Zulip):

to me, this feels like it will be the latter

kleimkuhler (Oct 04 2018 at 16:30, on Zulip):

@nikomatsakis I am trying to get the match arms in check_pat to all have the same type and I'm having an issue in handling the Paren case. In the Paren case, it should pass along the entire &ast::Pat (to indicate the entire pattern does not need to be surrounded by parentheses). In the other cases, it should pass along a ref.

For example, the last two currently are:

Slice(_, Some(ref pat), _) => (pat, "optional position pattern", false),
Paren(_) => (p, "pattern", false),

which produce the following types:

(&syntax::ptr::P<syntax::ast::Pat>, &str, bool)
(&syntax::ast::Pat, &'static str, bool)
kleimkuhler (Oct 04 2018 at 16:32, on Zulip):

I've tried wrapping the p in the Paren case with &P::new(p) and a few variants of it (supplying the type it should be and all), but can't seem to get things to align.

Is there a different way to go about handling the case that does not involve the fiddling with wrapping it in a new syntax::ptr::P?

nikomatsakis (Oct 04 2018 at 16:32, on Zulip):

if you have a &P<Pat>, you can deref that to a &Pat

nikomatsakis (Oct 04 2018 at 16:32, on Zulip):

is that what you need?

nikomatsakis (Oct 04 2018 at 16:33, on Zulip):

maybe you can send along the code that doesn't build + the errors?

kleimkuhler (Oct 04 2018 at 16:34, on Zulip):

The other arms all produce values of &P<Pat>, it's the Paren case that just produces a Pat is the issue.

kleimkuhler (Oct 04 2018 at 16:35, on Zulip):

Sure that may be more helpful. How do you prefer that usually - just push up what I have to a branch or a different way?

nikomatsakis (Oct 04 2018 at 16:35, on Zulip):

I usually prefer a [WIP] PR

nikomatsakis (Oct 04 2018 at 16:35, on Zulip):

but pushing to a branch and sending a link to the branch is also ok

nikomatsakis (Oct 04 2018 at 16:35, on Zulip):

it's just that it's easier to comment on PRs

kleimkuhler (Oct 04 2018 at 16:53, on Zulip):

Ok here is a comment: https://github.com/rust-lang/rust/pull/54820/files#r222748510

kleimkuhler (Oct 04 2018 at 16:57, on Zulip):

And the reason I want to pass p and not what Paren matches on, is because it then strips the Paren and will not lint on a case like (_) => {},

nikomatsakis (Oct 04 2018 at 17:00, on Zulip):

@kleimkuhler try one of two things:

 let (value, msg, struct_lit_needs_parens): (&ast::Pat, _, _, _) =

or

 Ident(.., Some(ref pat)) => (&*pat, "optional subpattern", false),
nikomatsakis (Oct 04 2018 at 17:01, on Zulip):

the former will force a coercion, the latter does the coercion explicitly

nikomatsakis (Oct 04 2018 at 17:01, on Zulip):

you'll have to change all the arms to match, in the second case

nikomatsakis (Oct 04 2018 at 17:01, on Zulip):

P is a "smart pointer", much like Box, so it can be deref'd

nikomatsakis (Oct 04 2018 at 17:01, on Zulip):

you might need &**pat though (ugh..)

kleimkuhler (Oct 04 2018 at 17:02, on Zulip):

Hm yea that's what I was trying to avoid (changing all the arms to match)... I'll try the coercion explicitly and hopefully can get somewhere with that.

kleimkuhler (Oct 04 2018 at 17:03, on Zulip):

Does it make sense though in trying to make a new syntax::ptr::P with p? That way I don't need to be explicit about the coercion and I just handle the Paren case?

kleimkuhler (Oct 04 2018 at 17:06, on Zulip):

And quick unrelated question... how should [WIP] PRs be handled once they've served the purpose of sharing progress? Should I close and reopen a new one when it's ready, or keep it open and just edit the title when it's ready?

nikomatsakis (Oct 04 2018 at 17:09, on Zulip):

Does it make sense though in trying to make a new syntax::ptr::P with p? That way I don't need to be explicit about the coercion and I just handle the Paren case?

That does not make sense

nikomatsakis (Oct 04 2018 at 17:09, on Zulip):

And quick unrelated question... how should [WIP] PRs be handled once they've served the purpose of sharing progress? Should I close and reopen a new one when it's ready, or keep it open and just edit the title when it's ready?

I would say just keep it open — then I can come along and make quick comments

nikomatsakis (Oct 04 2018 at 17:09, on Zulip):

depends how actively you are working on it, I think

nikomatsakis (Oct 04 2018 at 17:09, on Zulip):

to elaborate: P is an "owning" pointer, like Box

nikomatsakis (Oct 04 2018 at 17:10, on Zulip):

so you never convert things into a P, you only convert from &P<T> into &T; otherwise, you'd be deep-cloning a piece of the tree

kleimkuhler (Oct 04 2018 at 17:16, on Zulip):

I’ll keep it open; I plan to wrap it up soon.

Thanks for the elaboration on that!

nikomatsakis (Oct 04 2018 at 17:27, on Zulip):

@kleimkuhler send me a link to the PR

nikomatsakis (Oct 04 2018 at 17:27, on Zulip):

when it's open..

kleimkuhler (Oct 06 2018 at 02:18, on Zulip):

@nikomatsakis It is ready for a review! https://github.com/rust-lang/rust/pull/54820. The Paren check ended up being a lot simpler in the end.

nikomatsakis (Oct 08 2018 at 15:01, on Zulip):

@kleimkuhler nice! I left a review describing how we could make it more precise, if we wanted. What do you think?

kleimkuhler (Oct 08 2018 at 16:02, on Zulip):

@nikomatsakis I like your suggestion! My reply points out that the paren usage in expressions does also seem to specifically look for &pat and box pat, so while I agree it seems a little hacky, it is a sure way to warn when necessary.

I'll work on implementing those changes and leave a comment when it's good to go again. Thanks for the review!

kleimkuhler (Oct 08 2018 at 22:56, on Zulip):

@nikomatsakis I have a working implementation based off your review. I'd like to add at least an additional test to make sure the "more cases" part of these additions is covered.

A ast::PatKind::Range deconstructs into an a: ast::Expr and b: ast::Expr (so a little different than your review where you mentioned it would deconstruct into Pats). This means at the very least, a match arm should have the form &(expr ..= expr) => {}

What I'm struggling on is finding expr: ast::Exprs that fit into those slots (as in it compiles). I thought something like &((1+1)..=(1+2)) => {} would be a simple case, but it does not seem to parse correctly.

kleimkuhler (Oct 08 2018 at 22:58, on Zulip):

Looking at: https://doc.rust-lang.org/beta/reference/expressions/range-expr.html, it seems like Expression ..= Expression should be Ok, but I may be interpreting or assuming something incorrectly about that.

kleimkuhler (Oct 09 2018 at 04:07, on Zulip):

So it looks like right here: https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/parser.rs#L4065-L4066, a pattern the starts similar to the one above &(... will be parsed to be a token::OpenDelim and only expect a (pat, ..) tuple pattern. However, what I really want to happen is the surround the &(...) to be dropped, and it be parsed as a token::DotDotEq.

nikomatsakis (Oct 09 2018 at 13:18, on Zulip):

@kleimkuhler ok so you are correct about the ranges -- that's ok, it can return a Vec<&Expr> I suppose, though even better might be to return something like a trait object — but that's a bit of "premature generalization" I suppose

nikomatsakis (Oct 09 2018 at 13:18, on Zulip):

ultimately it just needs to return something "visitable"

nikomatsakis (Oct 09 2018 at 13:19, on Zulip):

I don't really understand why you are looking at the parser though :)

kleimkuhler (Oct 09 2018 at 14:04, on Zulip):

@nikomatsakis So you can see here the commit where I've added what you recommended in your review: https://github.com/rust-lang/rust/pull/54820/commits/2cb37eaea52c8ab3e97a7d536686c78ffe5e9c29#diff-d85eb66bc4756598e30d93a5d07d3d7fR401

I'm trying to add an additional test case where a match arm matches that additional block: &(expr ..= expr) => {}

nikomatsakis (Oct 09 2018 at 14:06, on Zulip):

@kleimkuhler great, I'll take a look in a bit :)

kleimkuhler (Oct 09 2018 at 14:07, on Zulip):

I was looking at the parser because I was having a hard time writing a test case that properly fits that format. I tried things like &(&1 ..= &2) => {} and variations of surround the exprs with parentheses, as well as different exprs like (1+1), but I was getting parsing errors... so I trying to figure out why

kleimkuhler (Oct 09 2018 at 14:07, on Zulip):

@nikomatsakis Ah great appreciate it! :slight_smile:

nikomatsakis (Oct 09 2018 at 16:54, on Zulip):

@kleimkuhler I just realized that I was confused about something

nikomatsakis (Oct 09 2018 at 16:54, on Zulip):

somewhat annoyingly, the lint visitor (I believe) always walks all the patterns

nikomatsakis (Oct 09 2018 at 16:55, on Zulip):

this means that the scheme I had in mind doesn't quite work

nikomatsakis (Oct 09 2018 at 16:55, on Zulip):

since it relied on "skipping over" parens that were necessary during the walk

kleimkuhler (Oct 09 2018 at 17:44, on Zulip):

@nikomatsakis I'm not sure where you'd like to carry out conversation on this since it's split half/half between here and GH. I'll plan on getting a better idea of the mutable state here and then reply on GH if thats Ok

kleimkuhler (Oct 09 2018 at 17:47, on Zulip):

One option would be to keep a little mutable state in the UnusedParens lint itself; we could for example add the ids for 'necessary' parens into a set stored in the lint. Then when we reach the paren node, we can check if it is present in that set

I'm not sure I completely follow what the ids part of this would hold. Is it the patterns that should be surrounded by parens in the match arm? Since the lint visitor visits all the subpatterns, is it checking something in ids each time to see if that subpattern needs to be surrounded by parens?

nikomatsakis (Oct 09 2018 at 17:53, on Zulip):

each pattern has a NodeId

nikomatsakis (Oct 09 2018 at 17:53, on Zulip):

(I think)

nikomatsakis (Oct 09 2018 at 17:53, on Zulip):

so the lint would store a set of these ids

nikomatsakis (Oct 09 2018 at 17:53, on Zulip):

we would add "necessary parens" into the set

nikomatsakis (Oct 09 2018 at 17:53, on Zulip):

e.g., the paren in &(a ..= b)

nikomatsakis (Oct 09 2018 at 17:53, on Zulip):

then, when we reach a paren, we could check if it is in the set (and, probably, remove it if it is)

nikomatsakis (Oct 09 2018 at 17:53, on Zulip):

if it was not in the set, we can lint

nikomatsakis (Oct 09 2018 at 17:54, on Zulip):

so basically when visiting the &, we would see that it has a child which is a paren, and this paren has a "low priority" pat as its child

nikomatsakis (Oct 09 2018 at 17:54, on Zulip):

this makes the pat necessary

nikomatsakis (Oct 09 2018 at 17:54, on Zulip):

so we could add its id to the set

nikomatsakis (Oct 09 2018 at 17:54, on Zulip):

then when we visit the paren node (which we will do next), we can remove it from the set

kleimkuhler (Oct 09 2018 at 18:24, on Zulip):

Replying to acknowledge I've read the above, but I think I may need to familiarize myself more with the pattern visitor before understanding the distinction in adding "necessary parens" into the set, and then reaching a paren to check if it's there.

Am I correct in understanding that the visitor adds all the NodeIds for patterns and expressions (e.g. PatKind::Range(Expr, Expr, _)) and then check_pat is where it actually determines if a paren should be present?

kleimkuhler (Oct 09 2018 at 18:26, on Zulip):

In order to understand the responsibilities of the lint visitor and check_pat for implementing the above, is starting from here a good place to work from? https://github.com/rust-lang/rust/blob/master/src/librustc/lint/context.rs#L972-L973

nikomatsakis (Oct 09 2018 at 18:31, on Zulip):

that does not sound correct

nikomatsakis (Oct 09 2018 at 18:31, on Zulip):

when you write a lint and implement the EarlyLintPass trait

nikomatsakis (Oct 09 2018 at 18:31, on Zulip):

there is some outside code that is walking over the AST

nikomatsakis (Oct 09 2018 at 18:31, on Zulip):

and invoking check_pattern and check_expr on what it finds

nikomatsakis (Oct 09 2018 at 18:31, on Zulip):

so that is just going to happen regardless

nikomatsakis (Oct 09 2018 at 18:31, on Zulip):

our job is to implement those callbacks and figure out what to do

nikomatsakis (Oct 09 2018 at 18:32, on Zulip):

so if you have e.g. a pattern like &(a ..= b)

nikomatsakis (Oct 09 2018 at 18:32, on Zulip):

you will get a callback at the & pattern

nikomatsakis (Oct 09 2018 at 18:32, on Zulip):

then it will desend and you will get a callback on the (..) pattern

nikomatsakis (Oct 09 2018 at 18:32, on Zulip):

then you will get a callback on the a ..= b pattern

nikomatsakis (Oct 09 2018 at 18:32, on Zulip):

and then you will get callbacks for the a and b expressions

nikomatsakis (Oct 09 2018 at 18:33, on Zulip):

I am saying that when we get the callback for the outermost & pattern, we can add the id for the (..) pattern into a set; the reason we would do this is that — once we reach the (..) pattern – we have lost the context of where it appears

nikomatsakis (Oct 09 2018 at 18:33, on Zulip):

you can only look "down" the tree, in other words

nikomatsakis (Oct 09 2018 at 20:10, on Zulip):

did that help @kleimkuhler at all?

kleimkuhler (Oct 09 2018 at 20:32, on Zulip):

@nikomatsakis Trying not to follow-up with another question, but my best point of reference right now is when check_pat/check_expr gets called for UnusedParens. I'm trying to understand where the "add id for the (..) pattern into a set" differs from "once we reach the (..) pattern" because I currently think that would occur at the same time.

I understand about how the pattern is deconstructed down the tree, but I also don't really know why we can assume that a "lower priority" paren is always necessary. That may be solved with the is_high_precedence_pat/is_low_precedence_pat methods?

kleimkuhler (Oct 09 2018 at 20:32, on Zulip):

So I just need to familiarize myself more with the code before I have a better reply I think.

nikomatsakis (Oct 09 2018 at 20:33, on Zulip):

a paren is necessary if it is (a) a child of a high-precedence pattern and (b) contains a low-precedence pattern

nikomatsakis (Oct 09 2018 at 20:33, on Zulip):

that is, parentheses are basically the way you get low-precedence things into high-precedence ones

kleimkuhler (Oct 09 2018 at 20:41, on Zulip):

Ok so when (a) and (b) are satisfied the paren NodeId is considered necessary and should be added to the mutable set

kleimkuhler (Oct 09 2018 at 20:42, on Zulip):

And when we arrive at a paren outside the context of (a) and (b), we need to check if that paren NodeId is considered required

kleimkuhler (Oct 09 2018 at 20:43, on Zulip):

(and do the removal or lint if it is in the set or not)

nikomatsakis (Oct 09 2018 at 20:45, on Zulip):

right

nikomatsakis (Oct 09 2018 at 20:45, on Zulip):

I'm trying to understand where the "add id for the (..) pattern into a set" differs from "once we reach the (..) pattern" because I currently think that would occur at the same time.

nikomatsakis (Oct 09 2018 at 20:45, on Zulip):

the first one ("add to set") would occur while we are visiting the high-precedence pattern

nikomatsakis (Oct 09 2018 at 20:46, on Zulip):

the problem is that when we are visiting the paren (the child of the high-precedence pattern) we don't know anymore what its parent was

nikomatsakis (Oct 09 2018 at 20:46, on Zulip):

also, the helpers we introducd are fine, but since we don't need to control the walk anymore, the "is low-precedence" helper can just return a boolean

kleimkuhler (Oct 09 2018 at 20:54, on Zulip):

Great I'll work on those adjustments. Thank you for the clarifications in this.

I can work towards this so that there is a working implementation, but is the other option you mentioned in the PR after discussion with @manishearth something you'd like to still consider?

kleimkuhler (Oct 09 2018 at 20:54, on Zulip):

I see the appeal if that allows a solution to to be implemented without adding a mutable set to struct UnusedParens

kleimkuhler (Oct 09 2018 at 20:58, on Zulip):

Also... to bring up an old point I was trying to explain earlier, I'm having a hard time making a test case that actually compiles. Essentially it would be a pattern of the form &(expr ..= expr) => {} in a match.

expr should be something like &1 or (1+1)

kleimkuhler (Oct 09 2018 at 20:58, on Zulip):

I'll make a playground link one moment

nikomatsakis (Oct 09 2018 at 21:00, on Zulip):

I see the appeal if that allows a solution to to be implemented without adding a mutable set to struct UnusedParens

@manishearth felt that the mutable set was superior

nikomatsakis (Oct 09 2018 at 21:00, on Zulip):

it's more performant, for one thing, since it allows us to have all active lints visit each node as we walk

nikomatsakis (Oct 09 2018 at 21:00, on Zulip):

versus walking many times, once per lint

nikomatsakis (Oct 09 2018 at 21:00, on Zulip):

I'm not sure how much that matters, really

nikomatsakis (Oct 09 2018 at 21:01, on Zulip):

one question I guess is "what does the expr code do here"

nikomatsakis (Oct 09 2018 at 21:01, on Zulip):

my guess is that it is simply not as precise as it could be

nikomatsakis (Oct 09 2018 at 21:01, on Zulip):

b/c I don't see how you could get the right answers

nikomatsakis (Oct 09 2018 at 21:01, on Zulip):

without doing a similar scheme

nikomatsakis (Oct 09 2018 at 21:01, on Zulip):

hypothesis confirmed:

fn main() {
    let x = (1 * 2) + 3;
}

gets no warnings

nikomatsakis (Oct 09 2018 at 21:02, on Zulip):

I am not convinced this is bad

nikomatsakis (Oct 09 2018 at 21:02, on Zulip):

that is, another option would be for us to just give less lints :)

nikomatsakis (Oct 09 2018 at 21:02, on Zulip):

I guess I don't remember what the real motivation was here

nikomatsakis (Oct 09 2018 at 21:02, on Zulip):

:)

nikomatsakis (Oct 09 2018 at 21:03, on Zulip):

that is, I don't know whether it is the "mission statement" of unused parens to be as precise as possible, or just capture certain silly patterns

nikomatsakis (Oct 09 2018 at 21:03, on Zulip):

I am not convinced this is bad

in particular, I know that many people add extra parens to things like (A && B) || C because they don't care to memorize the precedence

nikomatsakis (Oct 09 2018 at 21:03, on Zulip):

and it's hard to argue with that...

kleimkuhler (Oct 09 2018 at 21:06, on Zulip):

Yes I agree the additional paren linting would be issuing warnings on patterns that are already complicated looking based off the predicates that have to be satisfied for paren to be necessary

kleimkuhler (Oct 09 2018 at 21:07, on Zulip):

(The parsing stuff I was talking about earlier is demonstrated here: https://play.rust-lang.org/?gist=4e7a62b619c2f8bdd8c4ddac6898561f&version=stable&mode=debug&edition=2015)

kleimkuhler (Oct 09 2018 at 21:08, on Zulip):

I'm happy to take this as far as linting should be. I picked this up as my first issue and have learned a lot from it already

nikomatsakis (Oct 09 2018 at 21:09, on Zulip):

:)

nikomatsakis (Oct 09 2018 at 21:09, on Zulip):

maybe we should leave some comments on the issue

nikomatsakis (Oct 09 2018 at 21:09, on Zulip):

I think that to be most analogous to the existing lint

kleimkuhler (Oct 09 2018 at 21:09, on Zulip):

If you want to see how aggressive the paren lints seem with the above implemented, I can do that. I also think the original implementation is just helpful enough as well.

nikomatsakis (Oct 09 2018 at 21:09, on Zulip):

we might only warn in the case where the outermost pattern has parens

nikomatsakis (Oct 09 2018 at 21:09, on Zulip):

that I think is how expressions work

nikomatsakis (Oct 09 2018 at 21:09, on Zulip):

e.g. if (expr) { .. } warns

nikomatsakis (Oct 09 2018 at 21:09, on Zulip):

but if (1 * 2) + 3 { .. } does not, even though the parens are equally unnecessary

nikomatsakis (Oct 09 2018 at 21:10, on Zulip):

honestly I think this was more about nudging people away from C/Java style

kleimkuhler (Oct 09 2018 at 21:10, on Zulip):

It incorrectly warns though as well in expressions

kleimkuhler (Oct 09 2018 at 21:10, on Zulip):

I believe there is an open issue on that

nikomatsakis (Oct 09 2018 at 21:11, on Zulip):

define incorrect?

nikomatsakis (Oct 09 2018 at 21:11, on Zulip):

there is a pending PR I saw about changing how expressions work in at least some cases

nikomatsakis (Oct 09 2018 at 21:11, on Zulip):

maybe related

kleimkuhler (Oct 09 2018 at 21:11, on Zulip):

Looks like it's currently being worked on: https://github.com/rust-lang/rust/issues/54704

nikomatsakis (Oct 09 2018 at 21:12, on Zulip):

ah yes

nikomatsakis (Oct 09 2018 at 21:12, on Zulip):

(there is a pending attempt to fix this that I was supposed to review)

kleimkuhler (Oct 09 2018 at 21:14, on Zulip):

If you look at the test cases I originally added, I do think the cases that are covered are generally what I would expect from a lint. It covers outer parens, but inner parens as well in a not very obtrusive way (mostly just by avoiding the Range cases)

nikomatsakis (Oct 09 2018 at 21:17, on Zulip):

yeah, I guess that's reasonable

nikomatsakis (Oct 09 2018 at 21:17, on Zulip):

maybe we should just go back to that :)

nikomatsakis (Oct 09 2018 at 21:17, on Zulip):

and consider this little diversion a "learning exercise"

kleimkuhler (Oct 09 2018 at 21:17, on Zulip):

I can add some of this conversation back to GH, but I also don't know where we stand exactly. I can add the mutable state and see what it looks like? I can keep it how it is. Or I can make it even less and just warn on the outer parens (which still may require then knowing where we are at in the pattern)

nikomatsakis (Oct 09 2018 at 21:18, on Zulip):

I think I would add a note to the GH issue like:

We decided not to try and get more precise (maybe give an example where we don't warn but parens are not needed) since it would complicate the lint and expressions are also not this precise.

nikomatsakis (Oct 09 2018 at 21:18, on Zulip):

I think the old behavior was just not to warn on parens that wrap a range?

nikomatsakis (Oct 09 2018 at 21:19, on Zulip):

(unless they are at the top-level, perhaps?)

nikomatsakis (Oct 09 2018 at 21:19, on Zulip):

in that case, you could give an example like ((a ..= b), (a ..= b)) as a pattern that "could warn" but doesn't

nikomatsakis (Oct 09 2018 at 21:19, on Zulip):

seems pretty obscure

kleimkuhler (Oct 09 2018 at 21:22, on Zulip):

Yes correct. How it was before adding the high/low precedence stuff it would not warn on the pattern above.

kleimkuhler (Oct 09 2018 at 21:23, on Zulip):

I think this set of my test cases is a pretty reasonable expectation of lints

match &1 {
        (e @ &(1...2)) => {} //~ WARNING: unnecessary parentheses around outer pattern
        &(_) => {}           //~ WARNING: unnecessary parentheses around pattern
        e @ &(1...2) => {}   // Ambiguous range pattern should not warn
        &(1..=2) => {}       // Ambiguous range pattern should not warn
    }
nikomatsakis (Oct 09 2018 at 21:27, on Zulip):

ok, let's go back to the old way :)

nikomatsakis (Oct 09 2018 at 21:28, on Zulip):

and just add some notes on the uncovered cases

kleimkuhler (Oct 09 2018 at 21:30, on Zulip):

Ok sounds good. I'll remove the most recent commit and add some comments for uncovered cases in the check_pat method. I won't be able to make those changes until I'm home tonight. I'll make sure it's good to go for a final review by end of day though!

nikomatsakis (Oct 09 2018 at 21:33, on Zulip):

great! :)

kleimkuhler (Oct 09 2018 at 22:39, on Zulip):

@nikomatsakis Knowing my next commit will most likely be the final one for this PR, the PR will contain 5 (4 after I remove the last, +1 with adding comments for uncovered cases). Is there a preference that I squash them or will that be covered upon a merge/approval by you?

kleimkuhler (Oct 09 2018 at 22:39, on Zulip):

Can't really find any preference for that noted in contributing guidelines.

varkor (Oct 09 2018 at 22:45, on Zulip):

@kleimkuhler: generally, reviewers tend not to be too fussy, but if each commit is approximately a unit, that's preferable

kleimkuhler (Oct 09 2018 at 22:45, on Zulip):

Great sounds good, thanks!

varkor (Oct 09 2018 at 22:45, on Zulip):

it's mainly the long chains of "fix typo"-style commits that get annoying if they're left unsquashed :grinning_face_with_smiling_eyes:

kleimkuhler (Oct 15 2018 at 17:06, on Zulip):

@nikomatsakis Hey since you picked this up more as a mentor issue and it's good to go, want me to switch the reviewer to anyone else? It's been ready to go for a few days, but I can take it off your plate since it's small if you'd prefer that.

nikomatsakis (Oct 15 2018 at 17:09, on Zulip):

no problem, I'll r+, I haven't done my "PR sweep" for the day though

Last update: Nov 20 2019 at 01:10UTC