Stream: t-compiler/wg-nll

Topic: #55344 `unused_mut` lint


davidtwco (Nov 07 2018 at 12:50, on Zulip):

@pnkfelix I've got a branch that stops applying the unused_mut lint to unreachable code - this mirrors the AST borrowck behaviour.

I'm not sure that the way I'm doing this is super great, but as far as I could gather, the only thing that would indicate that unreachable code had been removed was that the blocks/statements were gone and therefore locals go unused - I've not been able to work out any cases that would break my approach, but I might after stepping away from it for a while.

You mentioned reaching out to stakeholders to see what approach to take here, who did you have in mind? language team? or are there compiler team people who normally take point on lint issues?

pnkfelix (Nov 07 2018 at 14:01, on Zulip):

hmm, I guess I was thinking of clippy maintainers

pnkfelix (Nov 07 2018 at 14:01, on Zulip):

but of course clippy is a different beast

pnkfelix (Nov 07 2018 at 14:01, on Zulip):

How does your branch look w.r.t. the test suite? I assume it breaks next to nothing ... ?

davidtwco (Nov 07 2018 at 14:09, on Zulip):

It doesn't break anything. I've added a test case for the issue and everything else passes.

davidtwco (Nov 07 2018 at 14:10, on Zulip):

I'm not all that satisfied with the approach it takes, but I couldn't think of anything better. This is the branch.

pnkfelix (Nov 07 2018 at 14:18, on Zulip):

hmm

pnkfelix (Nov 07 2018 at 14:18, on Zulip):

do you need this condition index < self.arg_count + 1 ? It seems like the iterator would never hit it, based on the given range?

pnkfelix (Nov 07 2018 at 14:19, on Zulip):

(I guess I could have posted that as a comment on the commit, heh)

davidtwco (Nov 07 2018 at 14:20, on Zulip):

Ah, you're right, I don't.

pnkfelix (Nov 07 2018 at 14:21, on Zulip):

what happens if you just use the iterator from mut_vars_and_args_iter instead?

pnkfelix (Nov 07 2018 at 14:21, on Zulip):

(asking out of curiosity, mostly; I don't particularly care/worry about you adding this new method)

davidtwco (Nov 07 2018 at 14:22, on Zulip):

If they aren't assigned to, then I won't see statements/terminators with them, and therefore won't be removed from the new set (that is used in the union) and therefore will be considered used - even if they aren't.

davidtwco (Nov 07 2018 at 14:22, on Zulip):

I had that initially and it didn't work IIRC.

davidtwco (Nov 07 2018 at 14:22, on Zulip):

I don't think my explanation there quite makes sense, but I did try it.

davidtwco (Nov 07 2018 at 14:31, on Zulip):

(i've just double checked that it doesn't work with that)

pnkfelix (Nov 07 2018 at 14:34, on Zulip):

I don't think the approach you've adopted there is all that bad

pnkfelix (Nov 07 2018 at 14:36, on Zulip):

it'd be good to document the (very different) roles of the two fields of GatherUsedMutsVisitor. E.g.., if I understand correctly, one field is built up inductively, and the other is torn down co-inductively. I'm not 100% sure I'm even using those words quite right

pnkfelix (Nov 07 2018 at 14:36, on Zulip):

but the main point being: one starts out as the empty set and gets elements added to it, and the other starts as the whole universe of possible values (the set of Locals) and then gets elements removed from it. Right?

davidtwco (Nov 07 2018 at 14:44, on Zulip):

Yeah, that's right. I'll add a comment for that. I just wasn't sure if using "not being initialized" as an signal for "use of local is unreachable" would work.

pnkfelix (Nov 07 2018 at 14:56, on Zulip):

hmm. I imagine we must have some tests of that (uses of uninitialized locals), but it wouldn't hurt to double check quickly how your branch behaves there

davidtwco (Nov 07 2018 at 15:12, on Zulip):

Submitted #55758 for this with those changes. I tried a few tests locally but I couldn't think of anything that would misbehave.

Last update: Nov 21 2019 at 23:35UTC