Refactored used_mut_nodes

[nikhilshagri]

Fixes #42384

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[nikhilshagri]

@nikomatsakis I've created the PR, but I haven't been able to build it yet, something went awfully wrong when I was rebasing it. I'll let the bot do its job :)

[kennytm]

Failed to compile due to denied warning.

[00:07:27] error: field is never used: `used_mut_nodes`
[00:07:27]     --> /checkout/src/librustc/ty/mod.rs:2607:5
[00:07:27]      |
[00:07:27] 2607 |     used_mut_nodes: NodeSet
[00:07:27]      |     ^^^^^^^^^^^^^^^^^^^^^^^
[00:07:27]      |

[nikomatsakis]

this will need to be declared pub, that is why

[nikhilshagri]

huh, that's weird. It's used both in check_loans.rs and gather_loans.rs, I can't figure why that's cropping up.

[nikomatsakis]

Looking good so far =)

[nikomatsakis]

It seems that there are no uses of this field -- looking at the rest of this file, it seems like it still reads from tcx.used_mut_nodes. Once you add the pub keyword to the above file, of course, we'll get to this crate, at which point it won't compile, since tcx.used_mut_nodes has been removed.

[nikomatsakis]

@cynicaldevil did what I wrote make sense to you? seems like this PR is close, I'd love to see it get over the finish line! =)

[bors]

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

[nikhilshagri]

I've been trying to build my PR for the last few days, and have been running into a lot of little problems. Is it ok if I just make the changes you described and push them, without building it first?

[Mark-Simulacrum]

That should be fine. Let us know if we can help out with any of those problems (which I take are errors during the build?).

[alexcrichton]

@nikomatsakis @cynicaldevil hey! I was passing by and noticed this, and was curious, I wonder if we need to save off the set of unused nodes still? Long ago we had to save off these nodes as we didn't know the lint level for nodes until the very end of the compiler, but as of recently we actually know the lint levels of all nodes throughout most of compilation.

With that information, I wonder if this could be refactored to directly emit the lint as soon as it's detected? That way there's no need to persist these sets until the end of compilation, but rather we can emit the lint right-then-and-there and the diagnostic is cached through standard mechanisms for incremental.

[nikomatsakis]

@alexcrichton um yeah good point! In this case, the lint is purely local to the function, so there is no need to combine the results of multiple functions, so I think we could just do it "in place" as you suggest.

[nikomatsakis]

@cynicaldevil do you understand the alternative approach that @alexcrichton was describing? Think you would have time to pursue that?

[nikhilshagri]

@nikomatsakis Hmm I think I understand. I'll ask some more questions if I get stuck along the way.

[nikomatsakis]

@cynicaldevil any update? Also, see #44195, which performed a similar refactoring (for a different lint).

[nikhilshagri]

@nikomatsakis I asked @alexcrichton some questions on IRC a couple days ago, and I plan to work on it tomorrow. Sorry for dragging it, I'll wrap this up soon!

[nikomatsakis]

@cynicaldevil great! I was just checking in. =)

[nikhilshagri]

@alexcrichton tcx.lint_node() requires a lint and a span argument, and I can't figure out what I should pass as parameters for those. Could you help me out?

[alexcrichton]

I believe the lint is UNUSED_MUT and the span should be something along the lines of tcx.hir.span_if_local(id) (or something like that)

[alexcrichton]

@cynicaldevil ping just to make sure this stays on your radar!

If you're busy nowadays no worries as well!

[nikhilshagri]

Yes, sorry about that! I'll try to work on this during the weekend.

[nikomatsakis]

@cynicaldevil friendly ping =)

PS It's actually kind of important for us to fix this particular problem soon-ish. It's perfectly fine if you don't have time right now to get to it, someone else can write the patch -- there will be more decent bugs to tackle!

[nikhilshagri]

From IRC:
@nikomatsakis Is this struct here: https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L39 unwanted now? Should I delete it and just add the UNUSED_MUT lint to the lint_array in builtin.rs?

[nikhilshagri]

Right, I've pushed the latest changes here. I'm still getting a couple of errors:

error[E0308]: mismatched types
   --> src/librustc_borrowck/borrowck/check_loans.rs:853:72
    |
853 |                                        self.bccx.tcx.hir.span_if_local(local_id),
    |                                                                        ^^^^^^^^ expected struct `rustc::hir::def_id::DefId`, found struct `syntax::ast::NodeId`
    |
    = note: expected type `rustc::hir::def_id::DefId`
               found type `syntax::ast::NodeId`

error[E0277]: the trait bound `syntax_pos::MultiSpan: std::convert::From<std::option::Option<syntax_pos::Span>>` is not satisfied
   --> src/librustc_borrowck/borrowck/check_loans.rs:851:35
    |
851 |                     self.bccx.tcx.lint_node(UNUSED_MUT,
    |                                   ^^^^^^^^^ the trait `std::convert::From<std::option::Option<syntax_pos::Span>>` is not implemented for `syntax_pos::MultiSpan`
    |
    = help: the following implementations were found:
              <syntax_pos::MultiSpan as std::convert::From<syntax_pos::Span>>
    = note: required because of the requirements on the impl of `std::convert::Into<syntax_pos::MultiSpan>` for `std::option::Option<syntax_pos::Span>`

And the exact same errors appear in gather_loans.rs as well.

[aidanhs]

@cynicaldevil just so we're clear on where this PR is at, are you planning on asking on IRC about the failures above or are you hoping for some input as a comment on this PR?

[nikhilshagri]

Oh, I was hoping for some input here itself :)

[arielb1]

@cynicaldevil

You can use tcx.hir.span(local_id) instead of tcx.hir.span_if_local(local_id) and it will work. The former takes a NodeId and the latter a DefId. I think the second type error is caused by the first.

[nikhilshagri]

@arielb1 That solved the problem, thanks. But now I'm getting a long list of unused mut variable errors when I try to compile, all from src/libcore, which I'm pretty sure is wrong. This is happening when my stage1 compiler is trying to build the std artifacts. What I think might be happening is that the compiler is flagging down all mutable variables, not just unused ones.

[carols10cents]

ping @nikomatsakis @arielb1, looks like @cynicaldevil could use some input!

[nikhilshagri]

Thanks @carols10cents! @arielb1 and I had talked over IRC, and I progressed further, but got stuck again. I was thinking of asking on the IRC channel, but I'll post it here:
So I believe this code here: https://github.com/rust-lang/rust/pull/43945/files#diff-6674f3bcb9693693bcdeacfba9cb16efR452 is wrong, because the lint is actually being called on the set of used mut nodes.
Therefore, I must find a place where I can access all nodes marked mutable, and then check if they are unused, and then call the lint accordingly. I browsed some code in librustc_borrowck, but couldn't find such a place. I need help locating it.

[alexcrichton]

@cynicaldevil yes the warning there I believe is warning about the inverse. As described on #42384 what you'll probably want to do is to change the borrowck query to return a set of used mutable nodes. Then somewhere else in the compiler you'll use these results to warn about mutable nodes that aren't in the set.

[nikomatsakis]

Per our conversation on IRC, I'm going to close this PR (@cynicaldevil told me that they won't have time to follow up on it). Sorry @cynicaldevil that this turned out to be more difficult than I initially anticipated! Thanks for banging on it, in any case.