Stream: t-compiler/wg-nll

Topic: #53228 IndexMut note


davidtwco (Aug 29 2018 at 22:14, on Zulip):

@nikomatsakis Taking a look at this issue. Looking at the MIR, we've got the location bb10[0] where the mutability error is being reported, I can see where the index call is happening (where _9 is assigned, bb9[11], in index-mut-help.rs), what's the best way for me to get the location that assigned _9 - so I can check the rvalue?

davidtwco (Aug 30 2018 at 13:36, on Zulip):

ping @nikomatsakis

nikomatsakis (Aug 30 2018 at 13:42, on Zulip):

hey

nikomatsakis (Aug 30 2018 at 13:42, on Zulip):

is the MIR listed somewhere?

nikomatsakis (Aug 30 2018 at 13:42, on Zulip):

anyway, finding out where _9 is assigned... I think we have a visitor for it .. but it's not like mega cheap

nikomatsakis (Aug 30 2018 at 13:42, on Zulip):

I guess that's ok if it's just when an error is reported

davidtwco (Aug 30 2018 at 13:44, on Zulip):

@nikomatsakis https://gist.github.com/davidtwco/3c75a8d83682e689b57ba84be267f76d

nikomatsakis (Aug 30 2018 at 13:44, on Zulip):

ok I see so the error is here:

_8 = &mut (*_9);
davidtwco (Aug 30 2018 at 13:45, on Zulip):

Yeah, and then _9 is assigned above, here: _9 = const std::ops::Index::index(move _10, move _11).

nikomatsakis (Aug 30 2018 at 13:45, on Zulip):

actually

nikomatsakis (Aug 30 2018 at 13:45, on Zulip):

you can probably use the MoveData to answer this question

nikomatsakis (Aug 30 2018 at 13:45, on Zulip):

sec

davidtwco (Aug 30 2018 at 13:46, on Zulip):

I spent a while yesterday messing with MoveData to try get it to give me that - I've not interacted with MoveData at all so I wasn't really getting much of anywhere.

nikomatsakis (Aug 30 2018 at 13:46, on Zulip):

I feel like there was some visitor for finding writes before

nikomatsakis (Aug 30 2018 at 13:46, on Zulip):

I keep meaning to try and document MoveData more

nikomatsakis (Aug 30 2018 at 13:46, on Zulip):

it's a bit opaque

nikomatsakis (Aug 30 2018 at 13:46, on Zulip):

and/or refactor it

davidtwco (Aug 30 2018 at 13:46, on Zulip):

I did notice that above some of the logging statements for the error, there was the MirBorrowckCtxt::process_statement logging statement, that listed the move_outs vec.

davidtwco (Aug 30 2018 at 13:46, on Zulip):

In that vec, there was the location that I was hoping to get.

nikomatsakis (Aug 30 2018 at 13:47, on Zulip):

that said, you can use the init_path_map

davidtwco (Aug 30 2018 at 13:47, on Zulip):

Which led me down a rabbit hole of trying to figure out how to access it.

nikomatsakis (Aug 30 2018 at 13:47, on Zulip):

to find all the initializations of a given MovePathIndex

nikomatsakis (Aug 30 2018 at 13:47, on Zulip):

you can convert a Local to an MPI

davidtwco (Aug 30 2018 at 13:47, on Zulip):

Is that with closest_move_path_to (or something like that)?

davidtwco (Aug 30 2018 at 13:48, on Zulip):

Yesterday when experimenting, I tried with that function (whatever it is actually called), then I looked up a MoveOutIndex and then tried to index the moves - it worked but got me an entirely different location.

nikomatsakis (Aug 30 2018 at 13:49, on Zulip):

basically something like this should work:

let mpi = move_data.rev_lookup.find_local(local_variable);
for init in move_data.init_path_map[mpi].map(|i| &move_data.inits[i]) {
   ... // here `init` is a `Init`
}
davidtwco (Aug 30 2018 at 13:49, on Zulip):

init_path_map might be worth looking into.

nikomatsakis (Aug 30 2018 at 13:49, on Zulip):

at least in the case where you have a single local

davidtwco (Aug 30 2018 at 13:49, on Zulip):

Yeah, I think that'd do it.

nikomatsakis (Aug 30 2018 at 13:50, on Zulip):

the Init structure has the things you need: rustdoc

nikomatsakis (Aug 30 2018 at 13:50, on Zulip):

if you want to handle things that are not single locals, then "closest to" is probably good

nikomatsakis (Aug 30 2018 at 13:51, on Zulip):

if you have a path like a.b.c.d, then closest-to would give you the MPI for that path, or for some prefix is that path does not have an MPI -- really though once you get into that, you have to be concerned about all kinds of complications

nikomatsakis (Aug 30 2018 at 13:51, on Zulip):

and I suspect that for the purposes of this diagnostic

nikomatsakis (Aug 30 2018 at 13:51, on Zulip):

we can just ignore cases where it's not a local

nikomatsakis (Aug 30 2018 at 13:51, on Zulip):

if you have a path like a.b.c.d, then closest-to would give you the MPI for that path, or for some prefix is that path does not have an MPI -- really though once you get into that, you have to be concerned about all kinds of complications

e.g., you really want all prefixes, since a.b = ... also "reassigns" a.b.c.d

davidtwco (Aug 30 2018 at 13:54, on Zulip):

:+1:

davidtwco (Aug 30 2018 at 14:17, on Zulip):

@nikomatsakis I'm not sure that works for this. If I'm not mistaken, I'll need a Location so I can access the terminator (bb9[11]) which is what will mention the call to index. However, your approach wouldn't give me a Location? Changing it slightly to do:

let mpi  = self.move_data.rev_lookup.find_local(*local);
let moi = self.move_data.path_map[mpi].first().unwrap(); // Only one item.
debug!("{}", self.move_data.moves[moi].source); // gives bb13[2]

That's the same location I got yesterday with however I managed to do it then - it isn't the bb9[11] that I'd like to access, bb13[2] is the upcoming StorageDead(_9) that follows from the error location bb10[0].

nikomatsakis (Aug 30 2018 at 14:18, on Zulip):

not path-map

nikomatsakis (Aug 30 2018 at 14:18, on Zulip):

init_path_map

davidtwco (Aug 30 2018 at 14:18, on Zulip):

Yeah, I tried it with that, getting an Init - but I don't know how that helps me find the Location?

nikomatsakis (Aug 30 2018 at 14:18, on Zulip):

oh, I see, Init doesn't have a location

nikomatsakis (Aug 30 2018 at 14:18, on Zulip):

well, maybe we should add one to Init

davidtwco (Aug 30 2018 at 14:21, on Zulip):

It seems like Init is made at src/librustc_mir/dataflow/move_paths/builder.rs:430 and src/librustc_mir/dataflow/move_paths/builder.rs:243.

nikomatsakis (Aug 30 2018 at 14:25, on Zulip):

sounds about right

nikomatsakis (Aug 30 2018 at 14:25, on Zulip):

I would probably remove the span field

nikomatsakis (Aug 30 2018 at 14:25, on Zulip):

and replace it with a location

nikomatsakis (Aug 30 2018 at 14:25, on Zulip):

oh hmm

nikomatsakis (Aug 30 2018 at 14:25, on Zulip):

in the case of arguments, I guess, there is no "location"

davidtwco (Aug 30 2018 at 14:25, on Zulip):

It isn't obvious to me how to get the location for the first of those cases.

nikomatsakis (Aug 30 2018 at 14:26, on Zulip):

there is none

davidtwco (Aug 30 2018 at 14:26, on Zulip):

Yeah, what you said.

nikomatsakis (Aug 30 2018 at 14:26, on Zulip):

so perhaps it has to be an Option<Location>

davidtwco (Aug 30 2018 at 14:26, on Zulip):

Should I still keep the span - since in some cases that won't be computable by the location?

nikomatsakis (Aug 30 2018 at 14:27, on Zulip):

yes

nikomatsakis (Aug 30 2018 at 14:27, on Zulip):

/me thinks

nikomatsakis (Aug 30 2018 at 14:27, on Zulip):

I mean it's fine

nikomatsakis (Aug 30 2018 at 14:27, on Zulip):

the most concise representation would be to have some method that gets the span given the mir

nikomatsakis (Aug 30 2018 at 14:28, on Zulip):

and perhaps not use Option<Location> but a more meaningful custom enum, like

enum InitLocation {
    Argument(Local),
    Statement(Location),
}

or something

nikomatsakis (Aug 30 2018 at 14:28, on Zulip):

but I think it's fine to just add an opt_location field and explain why it is sometimes None

davidtwco (Aug 30 2018 at 15:16, on Zulip):

@nikomatsakis What's the best way to check if the rustc::ty::Const I have is std::ops::Index::index?

nikomatsakis (Aug 30 2018 at 15:17, on Zulip):

that's not really a well-formed question

nikomatsakis (Aug 30 2018 at 15:18, on Zulip):

what is this "const"?

nikomatsakis (Aug 30 2018 at 15:18, on Zulip):

what I mean is: a type can implement Index<T>

nikomatsakis (Aug 30 2018 at 15:18, on Zulip):

i'm not sure what it means for a constant to be Index

nikomatsakis (Aug 30 2018 at 15:18, on Zulip):

what is the role that the constant plays? :)

davidtwco (Aug 30 2018 at 15:19, on Zulip):

Well, the terminator is TerminatorKind::Call, the func of that is a Operand::Constant, which contains a ty::Const, which when you print in a debug log, says const std::ops::index::Index.

davidtwco (Aug 30 2018 at 15:20, on Zulip):

I figure if we find out we called index(..) on a value and have a AccessKind::MutableBorrow then we should add the help message - I've got the function we called, I just don't know what to compare that against to confirm it is in fact std::ops::index::Index.

davidtwco (Aug 30 2018 at 15:21, on Zulip):

The terminator is what I got from the location bb9[11] that I was previously looking for, where _9 was assigned.

nikomatsakis (Aug 30 2018 at 15:24, on Zulip):

Well, the terminator is TerminatorKind::Call, the func of that is a Operand::Constant, which contains a ty::Const, which when you print in a debug log, says const std::ops::index::Index.

ah I misunderstood your question

nikomatsakis (Aug 30 2018 at 15:25, on Zulip):

yes, ok, so...

nikomatsakis (Aug 30 2018 at 15:27, on Zulip):

I think you want to check if the const is a ConstVal::Unevaluated

nikomatsakis (Aug 30 2018 at 15:27, on Zulip):

we probably want some helpers for this

nikomatsakis (Aug 30 2018 at 15:27, on Zulip):

the def-id you can get from the lang items

nikomatsakis (Aug 30 2018 at 15:27, on Zulip):

self.tcx.lang_items().index_trait()

davidtwco (Aug 30 2018 at 16:07, on Zulip):

Annoyingly, this is the DefId I'm getting (it ended up not being a ConstVal::Unevaluated):

DefId(2/0:990 ~ core[177e]::ops[0]::index[0]::Index[0]::index[0])

...and this is the DefId I expect (from lang items):

DefId(2/0:988 ~ core[177e]::ops[0]::index[0]::Index[0]

davidtwco (Aug 30 2018 at 16:08, on Zulip):

Actually, pasting it out like that helped spot the difference I didn't see.

davidtwco (Aug 30 2018 at 16:49, on Zulip):

I feel like I know this stuff but I'm struggling to remember: what's the best way to get the not-a-compiler-temporary type from a place? Is there a helper function that'll work out the original type that the user was using?

nikomatsakis (Aug 30 2018 at 16:59, on Zulip):

do you mean place.ty(tcx).to_ty() ?

nikomatsakis (Aug 30 2018 at 16:59, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/tcx/enum.PlaceTy.html#method.to_ty

davidtwco (Aug 30 2018 at 17:08, on Zulip):

It isn't that - it gives me a type to print, but it returns std::string::String rather than std::collections::HashMap<&str, std::string::String> that I'd expect. _9 has the value of std::string::String - it's correct; but that's a compiler temporary, the actual type that the user would expect is the hashmap.

nikomatsakis (Aug 30 2018 at 17:12, on Zulip):

what type are you looking for exactly?

nikomatsakis (Aug 30 2018 at 17:13, on Zulip):

you want to find the type that failed to implement IndexMut?

nikomatsakis (Aug 30 2018 at 17:13, on Zulip):

if so, you can extract it from the "substs" applied to Index

nikomatsakis (Aug 30 2018 at 17:13, on Zulip):

you should be able to extract the "self type" out of the constant

nikomatsakis (Aug 30 2018 at 17:14, on Zulip):

and/or extract the type of the receiver (but I guess that will be a &HashMap<...>)

davidtwco (Aug 30 2018 at 17:15, on Zulip):

you want to find the type that failed to implement IndexMut?

Sorry, I wasn't clear, this is the type I'm looking for, yeah.

davidtwco (Aug 30 2018 at 17:47, on Zulip):

Submitted #53830 - sorry about all the questions.

nikomatsakis (Aug 30 2018 at 17:48, on Zulip):

heh no worries!

davidtwco (Aug 30 2018 at 17:48, on Zulip):

I'm clearly out of practice for having a lazy week or two.

davidtwco (Aug 31 2018 at 14:17, on Zulip):

I updated this to address your suggestion @nikomatsakis.

nikomatsakis (Aug 31 2018 at 14:18, on Zulip):

ah, great

davidtwco (Aug 31 2018 at 14:18, on Zulip):

Well, I guess I updated it with my suggestion regarding your question.

nikomatsakis (Aug 31 2018 at 14:27, on Zulip):

I like your suggestion

nikomatsakis (Aug 31 2018 at 14:29, on Zulip):

if you'll forgive me, I have some ideas for how to "go for the gold" here =)

nikomatsakis (Aug 31 2018 at 14:29, on Zulip):

that is, some further tweaks we could do

nikomatsakis (Aug 31 2018 at 14:29, on Zulip):

will leave a comment

nikomatsakis (Aug 31 2018 at 14:29, on Zulip):

actually hmm

nikomatsakis (Aug 31 2018 at 14:29, on Zulip):

this may not be the right place to do what I was going to suggest :)

nikomatsakis (Aug 31 2018 at 14:29, on Zulip):

nm, I'll leave a more limited comment

davidtwco (Aug 31 2018 at 15:12, on Zulip):

Do you want me to attempt making that change to use a predicate?

nikomatsakis (Aug 31 2018 at 15:13, on Zulip):

I've been debating. I guess I think it'd be better

nikomatsakis (Aug 31 2018 at 15:13, on Zulip):

I wanted to go and search and see where we do similar tings

nikomatsakis (Aug 31 2018 at 15:13, on Zulip):

see if we could make a helper

nikomatsakis (Aug 31 2018 at 15:14, on Zulip):

now that we have a proper rustc query this is relatively painless

davidtwco (Aug 31 2018 at 15:15, on Zulip):

I found this which as far as I could tell looked similar to what we'd want.

davidtwco (Sep 04 2018 at 14:10, on Zulip):

@nikomatsakis Did you get a chance to work out if I should attempt to use a predicate here?

nikomatsakis (Sep 04 2018 at 18:38, on Zulip):

@davidtwco I think it'd be better, but what I did not do was go and find the best example of doing so

davidtwco (Sep 05 2018 at 09:04, on Zulip):

@nikomatsakis What's the best way for me to get access to infcx in order to call predicate_{must|may}_hold in librustc_mir/borrow_check/mutability_errors.rs?

davidtwco (Sep 05 2018 at 10:48, on Zulip):

I've mangled things around such that I can call type_known_to_meet_bound.

However, this thinks that IndexMut is not implemented. I can see in the logs that it is looking at std::string::String (which is the type I'm indexing in my "has IndexMut" test case) against all the implementations of IndexMut - however, I think it is because I've not specified what T I'm looking for in IndexMut<T> that it never finds it. Just guessing though.

It also causes the other test case to panic with a "region constraints already solved" error - I've not bothered to look into that given that it isn't working on the primary case I'd like it to solve.

davidtwco (Sep 05 2018 at 10:51, on Zulip):

At least with the current implementation, if it incorrectly assumes IndexMut is implemented for the type then it only becomes less helpful rather than making a wrong suggestion.

nikomatsakis (Sep 05 2018 at 13:55, on Zulip):

@davidtwco did you push your commit?

davidtwco (Sep 05 2018 at 13:57, on Zulip):

I haven't because it wasn't quite working.

davidtwco (Sep 05 2018 at 13:58, on Zulip):

I'll make another branch with it so I don't waste Travis time and include it in the PR, for now here's the diff: https://gist.github.com/davidtwco/399f769051d890af9aedbdf022ad6097

davidtwco (Sep 05 2018 at 14:02, on Zulip):

Here's it on a branch: https://github.com/davidtwco/rust/commit/f053d19716ce11f82fbb15356afe5eb9ad2cddbf

davidtwco (Sep 05 2018 at 16:05, on Zulip):

If it is easier for you @nikomatsakis (if you're intending to do a build) then I could give you access to the box I have a build on. Could save some time. It's just some (relatively) beefy dedicated box I rent for faster compilations on Rust contributions and saving me rebuilding between machines - so there's nothing else on it.

nikomatsakis (Sep 05 2018 at 16:09, on Zulip):

@davidtwco ok lemme look now..

davidtwco (Sep 05 2018 at 19:55, on Zulip):

cough

nikomatsakis (Sep 05 2018 at 20:10, on Zulip):

sorry :)

nikomatsakis (Sep 05 2018 at 20:45, on Zulip):

ok, so, I looked a bit more at type_known_to_meet_bound. It's not really what you are looking for. Among other things, it is designed for queries like T: Copy that have only one type involved. But in this case, we want to check T: IndexMut<U> -- two types.

However, we do have access to the substs from the call, which is convenient.

nikomatsakis (Sep 05 2018 at 21:09, on Zulip):

ok so I think that the substs we have is for the method call; this actually extends the substs from the trait with those of the method. In this case, though, that is probably equal.

nikomatsakis (Sep 05 2018 at 21:10, on Zulip):

(yes)

nikomatsakis (Sep 05 2018 at 21:12, on Zulip):
let index_trait = self.infcx.tcx.lang_items().index_trait();
if self.infcx.tcx.parent(id) == index_trait {
    let trait_ref = ty::TraitRef {
        def_id: /* DefId of IndexMut */,
        substs,
    };
    let obligation = Obligation {
        param_env,
        cause: ObligationCause::misc(span, ast::DUMMY_NODE_ID),
        recursion_depth: 0,
        predicate: trait_ref.to_predicate(),
    };
    if self.infcx.predicate_may_hold(&obligation) { ... }
}
nikomatsakis (Sep 05 2018 at 21:12, on Zulip):

cc @davidtwco :point_up:

nikomatsakis (Sep 05 2018 at 21:13, on Zulip):

I could also imagine adding a few assert! in there

nikomatsakis (Sep 05 2018 at 21:13, on Zulip):

in particular, asserting that the generics.len() for index-mut trait and index trait are equal, and that the method has an empty set of generics on its own

davidtwco (Sep 05 2018 at 21:13, on Zulip):

I assume param_env is self.param_env?

davidtwco (Sep 05 2018 at 21:13, on Zulip):

I've tried almost essentially that.

nikomatsakis (Sep 05 2018 at 21:14, on Zulip):

yes, param-env is self.param_env

davidtwco (Sep 05 2018 at 21:14, on Zulip):

Except I used must hold rather than may hold.

nikomatsakis (Sep 05 2018 at 21:14, on Zulip):

that is ok too

davidtwco (Sep 05 2018 at 21:14, on Zulip):

So perhaps that will make the difference.

nikomatsakis (Sep 05 2018 at 21:14, on Zulip):

well, it may be ok let's say ;)

nikomatsakis (Sep 05 2018 at 21:14, on Zulip):

ok, if you hit problems, let me see that version?

davidtwco (Sep 05 2018 at 21:16, on Zulip):

Sure thing. I tried a handful of things so I'll give what you suggested a go and then let you know.

davidtwco (Sep 05 2018 at 21:28, on Zulip):

Apparently if I had tried predicate_may_hold rather than predicate_must_hold then I'd have had it this morning.

davidtwco (Sep 05 2018 at 21:29, on Zulip):

But it does ICE with the non-IndexMut-implementing case still.

davidtwco (Sep 05 2018 at 21:29, on Zulip):

With "region constraints already solved".

nikomatsakis (Sep 05 2018 at 21:29, on Zulip):

oh bother

nikomatsakis (Sep 05 2018 at 21:29, on Zulip):

hmm

davidtwco (Sep 05 2018 at 21:29, on Zulip):

Updated that temp branch.

nikomatsakis (Sep 05 2018 at 21:31, on Zulip):

can you run with RUST_BACKTRACE=1

davidtwco (Sep 05 2018 at 21:32, on Zulip):

https://gist.github.com/davidtwco/f1d5bfc80c8bb95708524930706e6a8c

nikomatsakis (Sep 05 2018 at 21:32, on Zulip):

oh, interesting

nikomatsakis (Sep 05 2018 at 21:41, on Zulip):

well that's annoying

nikomatsakis (Sep 05 2018 at 21:41, on Zulip):

there's not really a good reason for that code to fail

nikomatsakis (Sep 05 2018 at 21:41, on Zulip):

actually

nikomatsakis (Sep 05 2018 at 21:42, on Zulip):

that .. is kind of a bug in canonicalization, now that I think about it

nikomatsakis (Sep 05 2018 at 21:42, on Zulip):

in particular, if you have the various "replace all free regions" flags set...we still track if you have the same region variable appearing twice..which I think we didn't want to do

davidtwco (Sep 05 2018 at 21:51, on Zulip):

I'm not sure how I'd go about fixing that?

nikomatsakis (Sep 05 2018 at 21:53, on Zulip):

yeah, I wasn't suggesting you should

nikomatsakis (Sep 05 2018 at 21:53, on Zulip):

maybe we should land your PR as is

nikomatsakis (Sep 05 2018 at 21:53, on Zulip):

and file a follow-up issue

nikomatsakis (Sep 05 2018 at 21:54, on Zulip):

with your diff here in the issue

nikomatsakis (Sep 05 2018 at 21:54, on Zulip):

I can try to patch canonicalization in the meantime

davidtwco (Sep 05 2018 at 21:54, on Zulip):

I can keep the branch around that I've been doing the experimentation on.

Last update: Nov 22 2019 at 00:55UTC