Stream: t-compiler/wg-nll

Topic: diagnostics


nikomatsakis (May 17 2018 at 13:55, on Zulip):

Dear world,

So here's something I would like feedback on. I've got this 'almost PR' that aims to fix https://github.com/rust-lang-nursery/polonius/issues/24. However, it seems to — for reasons I'm not 100% sure about — regress diagnostics in some cases. I'd sort of like to land it anyway, and then revisit diagnostics, but I do think we need to start focusing on those and I'm not sure how best to do it.

Reed Koser (May 17 2018 at 16:26, on Zulip):

I think what would probably be useful is just manually wirting a bunch of diagnosis messages to get a feel for the interface we want to present

Reed Koser (May 17 2018 at 16:27, on Zulip):

and then working backwards to the code from there

Reed Koser (May 17 2018 at 16:27, on Zulip):

AST borrow-check errors is probably a good place to start, breaking everyone's ability to read error messages would probably not be a good idea

Reed Koser (May 17 2018 at 16:28, on Zulip):

As a personal anecdote, I really pretty heavily on the precise wording of error messages; especially when starting out

Reed Koser (May 17 2018 at 16:30, on Zulip):

I still mostly solve compilation problems with unfamiliar Haskell code that way since I still haven't learned all the jargon to understand the error messages on a deeper level, and I imagine a number of people are in a similar position with Rust

nikomatsakis (May 17 2018 at 16:39, on Zulip):

I'm torn. On the one hand, I think our current AST-based messages use way too much jargon, and can be simplified. On the other, I'd like to do things step by step.

nikomatsakis (May 17 2018 at 16:39, on Zulip):

but the phasing is only part of it, it's more about "how shall we get the info and spans we need"

Santiago Pastorino (May 17 2018 at 22:38, on Zulip):

as for diagnostics messages, what happened with the big spreadsheet Ariel have built?

Santiago Pastorino (May 17 2018 at 22:38, on Zulip):

seemed like a good way to start

nikomatsakis (May 18 2018 at 00:51, on Zulip):

well we have one better now, the .nll reference files

nikomatsakis (May 18 2018 at 00:51, on Zulip):

anyway, i've had a few thoughts but I'll write them up tomorrow

nikomatsakis (May 21 2018 at 12:49, on Zulip):

@pnkfelix we had also talked about doing a kind of diagnostic triage this morning, right?

pnkfelix (May 21 2018 at 12:49, on Zulip):

yes, later today

Jake Goulding (May 21 2018 at 14:45, on Zulip):
error: free region `` does not outlive free region `'a`

is not great :-)

nikomatsakis (May 21 2018 at 14:50, on Zulip):

you don't say :)

nikomatsakis (May 21 2018 at 14:51, on Zulip):

see also https://github.com/rust-lang/rust/issues/49397

pnkfelix (May 21 2018 at 19:00, on Zulip):

Hey @nikomatsakis I'm ready whenever you are. (I'll start looking over the list in the meantime, take notes here as I notice things.)

nikomatsakis (May 21 2018 at 19:00, on Zulip):

@pnkfelix ok

nikomatsakis (May 21 2018 at 19:00, on Zulip):

how should we start?

nikomatsakis (May 21 2018 at 19:00, on Zulip):

is there some central list ?

pnkfelix (May 21 2018 at 19:00, on Zulip):

THe idea is to review the .nll.stderr cases , right?

pnkfelix (May 21 2018 at 19:01, on Zulip):

I guess I could start by transcribing the existing ls output to a canonical list or table

nikomatsakis (May 21 2018 at 19:01, on Zulip):

right

nikomatsakis (May 21 2018 at 19:01, on Zulip):

I was just wondering in what order we wanted to go, I guess?

nikomatsakis (May 21 2018 at 19:01, on Zulip):

i'm bringing things back into cache here

nikomatsakis (May 21 2018 at 19:01, on Zulip):

I think our goal was to kind of "develop a triage system"?

pnkfelix (May 21 2018 at 19:02, on Zulip):

right, we were going to (if necessary?) sample a few?

pnkfelix (May 21 2018 at 19:02, on Zulip):

to maybe provide guidance?

pnkfelix (May 21 2018 at 19:02, on Zulip):

I will note that one subcategory that we may have overlooked

pnkfelix (May 21 2018 at 19:02, on Zulip):

are cases where we are producing many errors. One situation is with outright repetition, from the user's POV

pnkfelix (May 21 2018 at 19:03, on Zulip):

another is many related errors where only one is really pointing to the key error. (Though I'm guessing in general such identification will be subjective.)

nikomatsakis (May 21 2018 at 19:03, on Zulip):

I guess let's just go through find . -name '*nll.stderr' in alphabetical order?

pnkfelix (May 21 2018 at 19:04, on Zulip):

sorted by full path?

pnkfelix (May 21 2018 at 19:04, on Zulip):

that was unclear

nikomatsakis (May 21 2018 at 19:04, on Zulip):

idk, somebody make a list :p

pnkfelix (May 21 2018 at 19:04, on Zulip):

right was about to do that

nikomatsakis (May 21 2018 at 19:05, on Zulip):

ok lemme know when it exists :)

pnkfelix (May 21 2018 at 19:09, on Zulip):

here is a list: https://gist.github.com/pnkfelix/9f145908d70655a702787d9cadf78977

pnkfelix (May 21 2018 at 19:10, on Zulip):

can you edit my gists, or do I need to put this elsewhere?

nikomatsakis (May 21 2018 at 19:10, on Zulip):

I don't think I can edit a gist of yours

nikomatsakis (May 21 2018 at 19:10, on Zulip):

but I guess you can be the master

nikomatsakis (May 21 2018 at 19:11, on Zulip):

so we start with first thing?

pnkfelix (May 21 2018 at 19:11, on Zulip):

I guess so

pnkfelix (May 21 2018 at 19:12, on Zulip):

it seems essentially the same

pnkfelix (May 21 2018 at 19:12, on Zulip):

why does that output differ in that superficial way?

nikomatsakis (May 21 2018 at 19:13, on Zulip):

sorry, I'm having some git trouble :)

nikomatsakis (May 21 2018 at 19:13, on Zulip):

still haven't seen the new output

pnkfelix (May 21 2018 at 19:13, on Zulip):

yeah give me a sec

pnkfelix (May 21 2018 at 19:13, on Zulip):

I'll show you links

pnkfelix (May 21 2018 at 19:13, on Zulip):

its possible I'm misinterpreting the filenames

pnkfelix (May 21 2018 at 19:13, on Zulip):

since I myself am unfamiliar with how revisions and compiletest modes got mixed.

nikomatsakis (May 21 2018 at 19:13, on Zulip):

ok I'm back now

pnkfelix (May 21 2018 at 19:14, on Zulip):

so I'm comparing, old: https://github.com/rust-lang/rust/blob/master/src/test/ui/E0508.ast.stderr

nikomatsakis (May 21 2018 at 19:14, on Zulip):

seems like it's a missing suggestiojn

pnkfelix (May 21 2018 at 19:14, on Zulip):

new: https://github.com/rust-lang/rust/blob/master/src/test/ui/E0508.ast.nll.stderr

nikomatsakis (May 21 2018 at 19:14, on Zulip):

I would rate this as "meh"

pnkfelix (May 21 2018 at 19:14, on Zulip):

so if there's no suggestion, the diagnostic system collapses the error onto the same line as the "^^^^^^" ?

nikomatsakis (May 21 2018 at 19:14, on Zulip):

yes

nikomatsakis (May 21 2018 at 19:14, on Zulip):

if there is just one label at a given spot

pnkfelix (May 21 2018 at 19:14, on Zulip):

okay

nikomatsakis (May 21 2018 at 19:15, on Zulip):

if there are more labels, it falls back to other layouts

pnkfelix (May 21 2018 at 19:15, on Zulip):

here wait

pnkfelix (May 21 2018 at 19:15, on Zulip):

I'll move the gist to a Paper doc

nikomatsakis (May 21 2018 at 19:15, on Zulip):

+1

pnkfelix (May 21 2018 at 19:15, on Zulip):

(or ... does paper support the same table syntax? I guess I'll find out)

nikomatsakis (May 21 2018 at 19:16, on Zulip):

I..don't know about that

nikomatsakis (May 21 2018 at 19:16, on Zulip):

I doubt it

nikomatsakis (May 21 2018 at 19:16, on Zulip):

but maybe!

pnkfelix (May 21 2018 at 19:17, on Zulip):

I'll just transcribe to a Paper table as we go

nikomatsakis (May 21 2018 at 19:18, on Zulip):

which paper doc?

pnkfelix (May 21 2018 at 19:19, on Zulip):

here it is: https://paper.dropbox.com/doc/Untitled-KzroSvNgQ7lip3WQyiLTa

nikomatsakis (May 21 2018 at 19:21, on Zulip):

@pnkfelix ok augmented-assignment looks terrible :)

nikomatsakis (May 21 2018 at 19:21, on Zulip):

there seem to be a few things going on

pnkfelix (May 21 2018 at 19:21, on Zulip):

well the old one doesn't look that good to me either

nikomatsakis (May 21 2018 at 19:21, on Zulip):

I think it looks fine

nikomatsakis (May 21 2018 at 19:22, on Zulip):

but it is somewhat mesed up because of the comments that are in there

nikomatsakis (May 21 2018 at 19:22, on Zulip):

e.g., the output I see:

nikomatsakis (May 21 2018 at 19:22, on Zulip):
error[E0596]: cannot borrow immutable local variable `y` as mutable
  --> $DIR/augmented-assignments.rs:30:5
   |
LL |     let y = Int(2);
   |         - consider changing this to `mut y`
LL |     //~^ consider changing this to `mut y`
LL |     y   //~ error: cannot borrow immutable local variable `y` as mutable
   |     ^ cannot borrow mutably
pnkfelix (May 21 2018 at 19:22, on Zulip):

yeah I guess that's the issue

nikomatsakis (May 21 2018 at 19:22, on Zulip):

but without the comments, would be:

nikomatsakis (May 21 2018 at 19:22, on Zulip):
error[E0596]: cannot borrow immutable local variable `y` as mutable
  --> $DIR/augmented-assignments.rs:30:5
   |
LL |     let y = Int(2);
   |         - consider changing this to `mut y`
LL |
LL |     y
   |     ^ cannot borrow mutably
nikomatsakis (May 21 2018 at 19:22, on Zulip):

that seems relatively clean

pnkfelix (May 21 2018 at 19:22, on Zulip):

(I still like our embedded expected-errors-in-comments system but we probably need to move the outside of the spans that rustc reports)

nikomatsakis (May 21 2018 at 19:23, on Zulip):

(I'm not crazy about the wild jargon we add, like "immutable local variable y", but...)

nikomatsakis (May 21 2018 at 19:23, on Zulip):

anyway so in that first example, we are no longer issuing a suggestion:

nikomatsakis (May 21 2018 at 19:23, on Zulip):

I was thinking,

nikomatsakis (May 21 2018 at 19:23, on Zulip):

we should probably be categorizing the suggestions?

nikomatsakis (May 21 2018 at 19:23, on Zulip):

many of them already have issues afaik

nikomatsakis (May 21 2018 at 19:23, on Zulip):

not sure whether that's worth the trouble

nikomatsakis (May 21 2018 at 19:23, on Zulip):

I guess we could categorize the text of the missing suggestion

pnkfelix (May 21 2018 at 19:24, on Zulip):

well, if one can find the relevant link easily, no harm in linking to the issue from the paper doc then

nikomatsakis (May 21 2018 at 19:26, on Zulip):

I'm at least adding the text of the suggestion

nikomatsakis (May 21 2018 at 19:26, on Zulip):

I put some notes on augmented-assignment

pnkfelix (May 21 2018 at 19:27, on Zulip):

thanks

pnkfelix (May 21 2018 at 19:27, on Zulip):

I agree, I think the fact that there was no += in the output from the old error is probably in part what made me think it was not so good

nikomatsakis (May 21 2018 at 19:27, on Zulip):

yeah, that fn is less good than the other

nikomatsakis (May 21 2018 at 19:29, on Zulip):

so borrowck/borrowck-box-insensitivity.stderr

nikomatsakis (May 21 2018 at 19:29, on Zulip):

I think that what is happening here is that NLL lets us "see through" Box again

nikomatsakis (May 21 2018 at 19:29, on Zulip):

hence going from 16 errors to 2

nikomatsakis (May 21 2018 at 19:29, on Zulip):

those two errors seem fine

pnkfelix (May 21 2018 at 19:29, on Zulip):

yeah, I think we decided that was now part of the plan of record, right?

pnkfelix (May 21 2018 at 19:30, on Zulip):

(being able to "see through" Box)

nikomatsakis (May 21 2018 at 19:30, on Zulip):

yeah, @arielb1 and I said that

nikomatsakis (May 21 2018 at 19:30, on Zulip):

I think..it's fine

nikomatsakis (May 21 2018 at 19:30, on Zulip):

it was kind of a pain to reproduce the old behavior

nikomatsakis (May 21 2018 at 19:30, on Zulip):

and...for what?

nikomatsakis (May 21 2018 at 19:30, on Zulip):

ultimately we'd like other types to be able to behave like Box, but...

nikomatsakis (May 21 2018 at 19:31, on Zulip):

(particularly since the old logic was incomplete anyway)

pnkfelix (May 21 2018 at 19:31, on Zulip):

If I happen to see the record of that conversation, I'll link it there. Lets move along.

pnkfelix (May 21 2018 at 19:31, on Zulip):

borrowck/borrowck-escaping-closure-error-1.nll.stderr

pnkfelix (May 21 2018 at 19:31, on Zulip):

we've lost the special handling of closures here

nikomatsakis (May 21 2018 at 19:32, on Zulip):

yeah

nikomatsakis (May 21 2018 at 19:32, on Zulip):

this .. hmm. This is a tricky one. =)

nikomatsakis (May 21 2018 at 19:32, on Zulip):

I mean the old error was clearly smarter

pnkfelix (May 21 2018 at 19:32, on Zulip):

right, but do we want to attempt to re-embed such intelligence

nikomatsakis (May 21 2018 at 19:32, on Zulip):

and yet I'm not sure if I'd call it a blocker

nikomatsakis (May 21 2018 at 19:32, on Zulip):

it's kind of in between

pnkfelix (May 21 2018 at 19:32, on Zulip):

its not quite "meh"

nikomatsakis (May 21 2018 at 19:33, on Zulip):

maybe we want a middle category

pnkfelix (May 21 2018 at 19:33, on Zulip):

maybe "acceptable losses"

pnkfelix (May 21 2018 at 19:33, on Zulip):

Like, we know its something that would be to fix, but we have to prioritize...

nikomatsakis (May 21 2018 at 19:33, on Zulip):

right. kind of long tho

pnkfelix (May 21 2018 at 19:34, on Zulip):

doesn't the military have a succinct phrase for this?

nikomatsakis (May 21 2018 at 19:34, on Zulip):

probably :)

pnkfelix (May 21 2018 at 19:34, on Zulip):

collateral

pnkfelix (May 21 2018 at 19:34, on Zulip):

damage

pnkfelix (May 21 2018 at 19:34, on Zulip):

?

nikomatsakis (May 21 2018 at 19:34, on Zulip):

maybe :face_with_head_bandage:

nikomatsakis (May 21 2018 at 19:34, on Zulip):

heh =)

pnkfelix (May 21 2018 at 19:34, on Zulip):

("collateral" on its own dosn't make sense)

pnkfelix (May 21 2018 at 19:34, on Zulip):

heh

pnkfelix (May 21 2018 at 19:34, on Zulip):

:bandage:

pnkfelix (May 21 2018 at 19:34, on Zulip):

hmm

nikomatsakis (May 21 2018 at 19:35, on Zulip):

"if necessary"

nikomatsakis (May 21 2018 at 19:35, on Zulip):

anyway, pick something :)

nikomatsakis (May 21 2018 at 19:35, on Zulip):

doesn't have to be funny I guess ;)

pnkfelix (May 21 2018 at 19:36, on Zulip):

next one, borrowck/borrowck-escaping-closure-error-2.nll.stderr, seems to be another instance of the same

nikomatsakis (May 21 2018 at 19:36, on Zulip):

yeah

nikomatsakis (May 21 2018 at 19:37, on Zulip):

I think the next one is a case of "meh" to me

nikomatsakis (May 21 2018 at 19:37, on Zulip):

well

nikomatsakis (May 21 2018 at 19:37, on Zulip):

they're both not that great

nikomatsakis (May 21 2018 at 19:37, on Zulip):

but I guess that in this case, the old message is giving useful info

nikomatsakis (May 21 2018 at 19:37, on Zulip):

in a jargon-y way

nikomatsakis (May 21 2018 at 19:38, on Zulip):

I guess this is "if needed"? (I'm wondering if it's going to be easy to tell the difference between that and "meh", but we can always collapse them later)

pnkfelix (May 21 2018 at 19:38, on Zulip):

yeah there's somewhat more specific info there that might help someone over a hurdle.

pnkfelix (May 21 2018 at 19:38, on Zulip):

I'd say "if needed" for it

nikomatsakis (May 21 2018 at 19:38, on Zulip):

right

pnkfelix (May 21 2018 at 19:38, on Zulip):

part of our problem

pnkfelix (May 21 2018 at 19:38, on Zulip):

is that our first instance of "meh"

pnkfelix (May 21 2018 at 19:38, on Zulip):

really was so ignorable

pnkfelix (May 21 2018 at 19:38, on Zulip):

it may have a set a more extreme bar than we want for "meh" in general.

nikomatsakis (May 21 2018 at 19:39, on Zulip):

:)

nikomatsakis (May 21 2018 at 19:39, on Zulip):

there's a lot going on in this one

nikomatsakis (May 21 2018 at 19:40, on Zulip):

seems like three things:

1. match statements giving us a lot of duplicates
2. no suggestions about ref
3. useful specificity is gone ("which implements the Drop trait")

pnkfelix (May 21 2018 at 19:41, on Zulip):

even so

pnkfelix (May 21 2018 at 19:41, on Zulip):

I'm not feeling it to be a MY EYES case

pnkfelix (May 21 2018 at 19:42, on Zulip):

except for the duplication perhaps...

pnkfelix (May 21 2018 at 19:42, on Zulip):

I guess the ref suggestions could go a long way

pnkfelix (May 21 2018 at 19:42, on Zulip):

or rather, if we have regressed to point where we never suggest ref

pnkfelix (May 21 2018 at 19:42, on Zulip):

then that probably does need to be fixed.

nikomatsakis (May 21 2018 at 19:43, on Zulip):

I'm not really sure

nikomatsakis (May 21 2018 at 19:43, on Zulip):

I think the duplication is more severe

nikomatsakis (May 21 2018 at 19:43, on Zulip):

I think I'd call it "if needed" overall

pnkfelix (May 21 2018 at 19:44, on Zulip):

to be clear

pnkfelix (May 21 2018 at 19:44, on Zulip):

the duplication here

pnkfelix (May 21 2018 at 19:44, on Zulip):

...

nikomatsakis (May 21 2018 at 19:44, on Zulip):

the duplication seems to be O(num_arms)

nikomatsakis (May 21 2018 at 19:44, on Zulip):

it could be pretty intense

nikomatsakis (May 21 2018 at 19:44, on Zulip):

on a big program

nikomatsakis (May 21 2018 at 19:44, on Zulip):

so that might be a MY EYES problem

pnkfelix (May 21 2018 at 19:44, on Zulip):

ah okay

pnkfelix (May 21 2018 at 19:44, on Zulip):

you're not talking about the failure to collapse in the first arm

pnkfelix (May 21 2018 at 19:44, on Zulip):

the num1 and num2

nikomatsakis (May 21 2018 at 19:44, on Zulip):

we're also highlighting a weird span

nikomatsakis (May 21 2018 at 19:45, on Zulip):

it's natural to me

nikomatsakis (May 21 2018 at 19:45, on Zulip):

but blaming a binding in the match arm

nikomatsakis (May 21 2018 at 19:45, on Zulip):

instead of the path being matched

pnkfelix (May 21 2018 at 19:45, on Zulip):

but is this actually showing (more) duplication?

nikomatsakis (May 21 2018 at 19:45, on Zulip):

might be quite confusing to people

pnkfelix (May 21 2018 at 19:45, on Zulip):

sorry I'm just going through the old error

nikomatsakis (May 21 2018 at 19:45, on Zulip):

new error:

nikomatsakis (May 21 2018 at 19:45, on Zulip):
error[E0507]: cannot move out of borrowed content
  --> $DIR/borrowck-move-error-with-note.rs:23:19
   |
LL |         Foo::Foo1(num1,
   |                   ^^^^ cannot move out of borrowed content

error[E0507]: cannot move out of borrowed content
  --> $DIR/borrowck-move-error-with-note.rs:24:19
   |
LL |                   num2) => (),
   |                   ^^^^ cannot move out of borrowed content

error[E0507]: cannot move out of borrowed content
  --> $DIR/borrowck-move-error-with-note.rs:25:19
   |
LL |         Foo::Foo2(num) => (),
   |                   ^^^ cannot move out of borrowed content
nikomatsakis (May 21 2018 at 19:45, on Zulip):

it's actually O(num_bindings)

nikomatsakis (May 21 2018 at 19:45, on Zulip):

which is > arms

pnkfelix (May 21 2018 at 19:46, on Zulip):

oh you are talking about the failure to collapse then?

pnkfelix (May 21 2018 at 19:46, on Zulip):

the old error did report each binding

pnkfelix (May 21 2018 at 19:46, on Zulip):

just more concisely

nikomatsakis (May 21 2018 at 19:46, on Zulip):

yes, just one "master error"

pnkfelix (May 21 2018 at 19:46, on Zulip):

I think that's why I'm more willing to accept it as not as high a priority

nikomatsakis (May 21 2018 at 19:46, on Zulip):

I guess I could go with "if needed" here

pnkfelix (May 21 2018 at 19:46, on Zulip):

but maybe that's in part because I'm assuming it will be readily fixed...

nikomatsakis (May 21 2018 at 19:47, on Zulip):

in some sense the current output is better than if it were:

error[E0507]: cannot move out of borrowed content
  --> $DIR/borrowck-move-error-with-note.rs:21:11
   |
LL |     match *f {             //~ ERROR cannot move out of
   |           ^^ cannot move out of borrowed content

without the rest

pnkfelix (May 21 2018 at 19:47, on Zulip):

maybe we will need to categorize the distinct subproblems we've identified

nikomatsakis (May 21 2018 at 19:47, on Zulip):

because often it is very confusing to find the arm

nikomatsakis (May 21 2018 at 19:47, on Zulip):

that said, note that as ref bindings get deprecated in favor of match-ref-bindings this will be nicer :)

nikomatsakis (May 21 2018 at 19:47, on Zulip):

did you see the notes I put in the paper?

nikomatsakis (May 21 2018 at 19:47, on Zulip):

that is, I did try to break down

pnkfelix (May 21 2018 at 19:47, on Zulip):

Hmm

pnkfelix (May 21 2018 at 19:47, on Zulip):

the old errors show the match input

pnkfelix (May 21 2018 at 19:48, on Zulip):

as well as the arm

nikomatsakis (May 21 2018 at 19:48, on Zulip):

yeah

nikomatsakis (May 21 2018 at 19:48, on Zulip):

anyway I can get behind "if needed" here

pnkfelix (May 21 2018 at 19:48, on Zulip):

The new errors don't include the match input at all

nikomatsakis (May 21 2018 at 19:48, on Zulip):

though I think .. this is a fairly common thing

nikomatsakis (May 21 2018 at 19:48, on Zulip):

so I lean towards MY EYES

pnkfelix (May 21 2018 at 19:49, on Zulip):

oh you did say this as part of your span note

nikomatsakis (May 21 2018 at 19:49, on Zulip):

also we should move quickly :)

pnkfelix (May 21 2018 at 19:49, on Zulip):

okay

nikomatsakis (May 21 2018 at 19:49, on Zulip):

I did

nikomatsakis (May 21 2018 at 19:50, on Zulip):

hmm

nikomatsakis (May 21 2018 at 19:50, on Zulip):

I sorta preer the NLL output here :)

nikomatsakis (May 21 2018 at 19:50, on Zulip):

neither is great

pnkfelix (May 21 2018 at 19:50, on Zulip):

the NLL output does not include the type being moved out of

nikomatsakis (May 21 2018 at 19:50, on Zulip):

but NLL is better, I think

pnkfelix (May 21 2018 at 19:50, on Zulip):

which can be convenient

pnkfelix (May 21 2018 at 19:51, on Zulip):

but over all

nikomatsakis (May 21 2018 at 19:51, on Zulip):

true

nikomatsakis (May 21 2018 at 19:51, on Zulip):

the biggest problem w/ original is the span...

pnkfelix (May 21 2018 at 19:51, on Zulip):

I also like the NLL output.

nikomatsakis (May 21 2018 at 19:51, on Zulip):

it should really be highlighting the thing being matched probably

nikomatsakis (May 21 2018 at 19:51, on Zulip):

like the prior example

nikomatsakis (May 21 2018 at 19:51, on Zulip):

(sort of ironic that it isn't)

pnkfelix (May 21 2018 at 19:51, on Zulip):

yeah. Which it is odd the old error wasn't doing that

nikomatsakis (May 21 2018 at 19:51, on Zulip):

:shrug:

nikomatsakis (May 21 2018 at 19:51, on Zulip):

not sure why not

pnkfelix (May 21 2018 at 19:51, on Zulip):

anyway

pnkfelix (May 21 2018 at 19:51, on Zulip):

i say "meh"

nikomatsakis (May 21 2018 at 19:51, on Zulip):

yeah

nikomatsakis (May 21 2018 at 19:53, on Zulip):

seems like next case is :100: — NLL gets no error

nikomatsakis (May 21 2018 at 19:53, on Zulip):

hopefully correctly:)

nikomatsakis (May 21 2018 at 19:53, on Zulip):

I can't even tell what the original was trying to test

nikomatsakis (May 21 2018 at 19:54, on Zulip):

(do we want to record tests that we ought to update for NLL?)

pnkfelix (May 21 2018 at 19:54, on Zulip):

okay next up

pnkfelix (May 21 2018 at 19:54, on Zulip):

oh sorry

pnkfelix (May 21 2018 at 19:54, on Zulip):

didn't keep up with you (lost my browser windows with the tests)

pnkfelix (May 21 2018 at 19:54, on Zulip):

we need to have a category for that, at lesast in terms of whether to try to reproduce the original

nikomatsakis (May 21 2018 at 19:55, on Zulip):

right

pnkfelix (May 21 2018 at 19:55, on Zulip):

for some, the error will be inherently about testing lexical lifetimes

nikomatsakis (May 21 2018 at 19:55, on Zulip):

this particular test seems kind of .. like it must be covered elsewhere

nikomatsakis (May 21 2018 at 19:55, on Zulip):

but yeah

pnkfelix (May 21 2018 at 19:55, on Zulip):

"weak test"

nikomatsakis (May 21 2018 at 19:55, on Zulip):

I added an update category

pnkfelix (May 21 2018 at 19:56, on Zulip):

Hmm you really want to mark this as "100" ?

pnkfelix (May 21 2018 at 19:56, on Zulip):

I guess your point is that it does not block NLL

pnkfelix (May 21 2018 at 19:56, on Zulip):

but

pnkfelix (May 21 2018 at 19:56, on Zulip):

it probably should ?

pnkfelix (May 21 2018 at 19:56, on Zulip):

otherwise turning NLL on my default will then lead to risk of future regression not being caught

nikomatsakis (May 21 2018 at 19:57, on Zulip):

ah

nikomatsakis (May 21 2018 at 19:57, on Zulip):

ok, we can add some new category

pnkfelix (May 21 2018 at 19:57, on Zulip):

I suggested "weak test" before

nikomatsakis (May 21 2018 at 19:58, on Zulip):

ah ok

nikomatsakis (May 21 2018 at 19:58, on Zulip):

sounds good

nikomatsakis (May 21 2018 at 19:58, on Zulip):

I wrote "update" but "weak test" is fine

pnkfelix (May 21 2018 at 19:59, on Zulip):

Update is fine, lets just make use of our last minute

pnkfelix (May 21 2018 at 19:59, on Zulip):

oy that's a MY EYES

nikomatsakis (May 21 2018 at 20:00, on Zulip):

ah yeah definitely

pnkfelix (May 21 2018 at 20:00, on Zulip):

though it actually ... might be close

pnkfelix (May 21 2018 at 20:00, on Zulip):

if you squint just right. :)

nikomatsakis (May 21 2018 at 20:00, on Zulip):

the old one is .. light years better =)

nikomatsakis (May 21 2018 at 20:00, on Zulip):

hard to know where to even begin

nikomatsakis (May 21 2018 at 20:00, on Zulip):

I guess it's a specificity fail

pnkfelix (May 21 2018 at 20:01, on Zulip):

i do find it funny that NLL does point out that let x is not let mut x

pnkfelix (May 21 2018 at 20:01, on Zulip):

while the old diagnostic is focusing on the most fundamental issue to be addressed

nikomatsakis (May 21 2018 at 20:01, on Zulip):

hey, yes

nikomatsakis (May 21 2018 at 20:01, on Zulip):

I think that error is coming from regionck

nikomatsakis (May 21 2018 at 20:02, on Zulip):

so borrowck has not yet run

nikomatsakis (May 21 2018 at 20:02, on Zulip):

@pnkfelix I gotta another meeting now

nikomatsakis (May 21 2018 at 20:02, on Zulip):

but this was a good start

pnkfelix (May 21 2018 at 20:02, on Zulip):

yeahs lets stop here

nikomatsakis (May 21 2018 at 20:02, on Zulip):

:+1:

pnkfelix (May 21 2018 at 20:03, on Zulip):

I'll try to at least guessetimate how long the remaing categorization would take us, vs the time it would take if we tried to farm it out

nikomatsakis (May 23 2018 at 12:25, on Zulip):

@pnkfelix so I was thinking of looking over the list and starting already to mentor people through some of the existing problems. For example, I thought maybe @Santiago Pastorino might have interest in tackling the augmented-assignment problem (though I've not looked deeply at it)

pnkfelix (May 23 2018 at 12:26, on Zulip):

sure

pnkfelix (May 23 2018 at 12:26, on Zulip):

I added a column where we can add links to actual filed issues

pnkfelix (May 23 2018 at 12:26, on Zulip):

but I haven't filed any issues yet

pnkfelix (May 23 2018 at 12:27, on Zulip):

(I've just put in single letters to try to correlate the rows that should have the same issue)

nikomatsakis (May 23 2018 at 12:37, on Zulip):

ok

Santiago Pastorino (May 23 2018 at 14:51, on Zulip):

@nikomatsakis @pnkfelix wherever you need help :)

pnkfelix (May 23 2018 at 14:52, on Zulip):

I'm almost done going through the list

pnkfelix (May 23 2018 at 14:53, on Zulip):

have like 19 uncategorized rows left

pnkfelix (May 23 2018 at 14:53, on Zulip):

(I'll need to leave in perhaps 15 min or so... so we'll see ...)

pnkfelix (May 23 2018 at 15:21, on Zulip):

Okay, finished going through the whole list!

pnkfelix (May 23 2018 at 15:21, on Zulip):

https://paper.dropbox.com/doc/NLL-stderr-diagnostic-deviations-KzroSvNgQ7lip3WQyiLTa

pnkfelix (May 23 2018 at 15:21, on Zulip):

I stopped trying to correlate the rows as I went

pnkfelix (May 23 2018 at 15:21, on Zulip):

(in part because it was leading me to revise earlier rows as I identified commonalities)

pnkfelix (May 23 2018 at 15:22, on Zulip):

but I hope to do another pass over the table and actually create the issues for the related rows.

pnkfelix (May 23 2018 at 15:22, on Zulip):

Not right now though, have to go!

nikomatsakis (May 23 2018 at 15:26, on Zulip):

@pnkfelix awesome

nikomatsakis (May 23 2018 at 16:53, on Zulip):

@Santiago Pastorino so I was specifically looking at augmented-assignments.stderr from the paper document as a good starting diagnostics issue

nikomatsakis (May 23 2018 at 16:53, on Zulip):

not sure if there is a rust issue filed for that or not yet

nikomatsakis (May 23 2018 at 16:58, on Zulip):

looks like no, I filed https://github.com/rust-lang/rust/issues/51000

nikomatsakis (May 23 2018 at 17:07, on Zulip):

@Santiago Pastorino added some mentoring notes

Santiago Pastorino (May 23 2018 at 17:36, on Zulip):

cool

Santiago Pastorino (May 23 2018 at 17:36, on Zulip):

will check all this out later

Santiago Pastorino (May 23 2018 at 20:21, on Zulip):

@nikomatsakis I'm tackling this

Santiago Pastorino (May 23 2018 at 20:22, on Zulip):

was reading the issue

Santiago Pastorino (May 23 2018 at 20:22, on Zulip):

in the first part you said

Santiago Pastorino (May 23 2018 at 20:22, on Zulip):

unclear that the original selection is so great either. Maybe the += would be ideal.

Santiago Pastorino (May 23 2018 at 20:22, on Zulip):

what do you mean by original selection?

Santiago Pastorino (May 23 2018 at 20:22, on Zulip):

by original you meant non NLL?

Santiago Pastorino (May 23 2018 at 20:25, on Zulip):

if I got it correctly by original selection you meant non NLL which is x the one after +=

Santiago Pastorino (May 23 2018 at 20:25, on Zulip):

nll selection is += x

Santiago Pastorino (May 23 2018 at 20:25, on Zulip):

and you want just +=

Santiago Pastorino (May 23 2018 at 20:25, on Zulip):

is that correct?

Santiago Pastorino (May 23 2018 at 20:25, on Zulip):

it's weird to just highlight +=

nikomatsakis (May 23 2018 at 20:25, on Zulip):

well I think the += would be my ideal

nikomatsakis (May 23 2018 at 20:25, on Zulip):

but I guess it's a matter of taste :)

Santiago Pastorino (May 23 2018 at 20:26, on Zulip):

explain :)

nikomatsakis (May 23 2018 at 20:26, on Zulip):

anything that is not multi-line is going to be an improvement

nikomatsakis (May 23 2018 at 20:26, on Zulip):

(to start)

Santiago Pastorino (May 23 2018 at 20:26, on Zulip):

I mean with that selection the error will never show x

nikomatsakis (May 23 2018 at 20:26, on Zulip):

I personally find underlining the operator very clear:

nikomatsakis (May 23 2018 at 20:26, on Zulip):

the error occurs when executing this operator

nikomatsakis (May 23 2018 at 20:26, on Zulip):

typically the stuff is all on one line

nikomatsakis (May 23 2018 at 20:26, on Zulip):

e.g., x += x

nikomatsakis (May 23 2018 at 20:26, on Zulip):

I guess in that sense the test is kind of artificial

nikomatsakis (May 23 2018 at 20:27, on Zulip):

and maybe we over-rated its importance

nikomatsakis (May 23 2018 at 20:27, on Zulip):

anyway I suspect it's fine to just match the existing behavior

Santiago Pastorino (May 23 2018 at 20:27, on Zulip):

actually

Santiago Pastorino (May 23 2018 at 20:27, on Zulip):

in the existing behavior

Santiago Pastorino (May 23 2018 at 20:27, on Zulip):
23 |     x   //~ error: use of moved value: `x`
   |     ^ value used here after move
Santiago Pastorino (May 23 2018 at 20:27, on Zulip):

how do you know that x is used?

Santiago Pastorino (May 23 2018 at 20:28, on Zulip):

if = never shows up in the error

Santiago Pastorino (May 23 2018 at 20:28, on Zulip):

not sure in this kind of multi line statements that is helpful to remove parts of the statement that are in different lines

Santiago Pastorino (May 23 2018 at 20:28, on Zulip):

besides from what you highlight

nikomatsakis (May 23 2018 at 20:29, on Zulip):

that seems like an orthogonal concern

Santiago Pastorino (May 23 2018 at 20:29, on Zulip):

first, I guess is better to show the statement entirely

nikomatsakis (May 23 2018 at 20:29, on Zulip):

not if we underline it

nikomatsakis (May 23 2018 at 20:29, on Zulip):

:)

Santiago Pastorino (May 23 2018 at 20:29, on Zulip):

hehe yes

nikomatsakis (May 23 2018 at 20:29, on Zulip):

that is basically always worse

nikomatsakis (May 23 2018 at 20:29, on Zulip):

but you could imagine some kind of heuristic in the error reporter that "grows" a span

Santiago Pastorino (May 23 2018 at 20:29, on Zulip):

but if you only underline += will x show up?

nikomatsakis (May 23 2018 at 20:29, on Zulip):

but .. it's kind of tricky

nikomatsakis (May 23 2018 at 20:30, on Zulip):

no, but so what?

nikomatsakis (May 23 2018 at 20:30, on Zulip):

I guess what I mean is that

nikomatsakis (May 23 2018 at 20:30, on Zulip):

I don't know how realistic this test is :)

nikomatsakis (May 23 2018 at 20:30, on Zulip):

more commonly, I think that the += is not on a line by itself

Santiago Pastorino (May 23 2018 at 20:30, on Zulip):
24 |     +=
   |      ^ value used here after move
nikomatsakis (May 23 2018 at 20:31, on Zulip):

(but that argues perhaps it's not an important bug to fix...)

Santiago Pastorino (May 23 2018 at 20:31, on Zulip):
24 |     +=
   |      ^ value used here after move

this doesn't tell me anything :P

nikomatsakis (May 23 2018 at 20:31, on Zulip):

it looks like in the original we underline the left-hand-side of the +=

Santiago Pastorino (May 23 2018 at 20:32, on Zulip):

yes

nikomatsakis (May 23 2018 at 20:32, on Zulip):

@Santiago Pastorino well, I mean, you have to look at the source, sure

nikomatsakis (May 23 2018 at 20:32, on Zulip):

anyway, so why don't we just match the original?

nikomatsakis (May 23 2018 at 20:32, on Zulip):

I think realistic output would be:

nikomatsakis (May 23 2018 at 20:32, on Zulip):
24 |     x +=
   |       -- value used here after move
nikomatsakis (May 23 2018 at 20:32, on Zulip):

which is perhaps clearer

Santiago Pastorino (May 23 2018 at 20:33, on Zulip):

yeah, I'm not sure if I'm saying something practical or easy to implement or even a better thing :P, was just thinking a bit on the thing

nikomatsakis (May 23 2018 at 20:33, on Zulip):

but anyway, let's match the current behavior, it's easier anyway =)

Santiago Pastorino (May 23 2018 at 20:33, on Zulip):

:+1:

nikomatsakis (May 23 2018 at 20:33, on Zulip):

I think to do that, we have to extract the span of the LHS of the +=

nikomatsakis (May 23 2018 at 20:33, on Zulip):

and use it in place of the current expr_span

nikomatsakis (May 23 2018 at 20:34, on Zulip):

yeah, I'm not sure if I'm saying something practical or easy to implement or even a better thing :P, was just thinking a bit on the thing

well it's good to question my assumptions :) to me underlining the operator is super clear, but my brain sometimes works differently than others

Santiago Pastorino (May 23 2018 at 20:35, on Zulip):

having doubts about what you think is like when you think you found a bug in the compiler, you will almost always be wrong ;)

Santiago Pastorino (May 23 2018 at 20:35, on Zulip):

but helps in trying to understand stuff

nikomatsakis (May 23 2018 at 20:35, on Zulip):

maybe not in this case...

nikomatsakis (May 23 2018 at 20:35, on Zulip):

I think like a compiler author :)

nikomatsakis (May 23 2018 at 20:36, on Zulip):

that said, I think there is precedent for this: e.g., clang underlines the operator sometimes

Keith Yeung (May 23 2018 at 20:36, on Zulip):

looks like both of you are having fun working on the diagnostics

Keith Yeung (May 23 2018 at 20:36, on Zulip):

will join you on one issue later today

nikomatsakis (May 23 2018 at 20:37, on Zulip):

there's a lot more, to be sure

nikomatsakis (May 23 2018 at 20:37, on Zulip):

I think the closure one is a good pick but I didn't want to do my homework to dig in :) happy to help answer questions though

Keith Yeung (May 23 2018 at 20:39, on Zulip):

not sure which one you're talking about; is it issue-45983?

nikomatsakis (May 23 2018 at 20:40, on Zulip):

I meant e.g. borrowck/issue-45983.nll.stderr

nikomatsakis (May 23 2018 at 20:41, on Zulip):

but borrowck/borrowck-move-error-with-note.stderr is also pretty bad

nikomatsakis (May 23 2018 at 21:53, on Zulip):

@Santiago Pastorino so it looks like, for that example, we used to offer a suggestion ("consider adding mut") that we no longer do; that is a simple enough thing, and might be worth re-adding

nikomatsakis (May 23 2018 at 21:53, on Zulip):

@Keith Yeung could probably advise, given their work on the "unused mutability" lint

nikomatsakis (May 23 2018 at 21:54, on Zulip):

however, not sure if it is top priority

nikomatsakis (May 23 2018 at 21:54, on Zulip):

let me look at the rest of the list

nikomatsakis (May 23 2018 at 21:55, on Zulip):

lots of the closure cases, which I think are also imp't

nikomatsakis (May 23 2018 at 21:55, on Zulip):

there is lifetime-errors/ex2a-push-one-existing-name-early-bound.nll.stderr, which is very odd

nikomatsakis (May 23 2018 at 21:56, on Zulip):

@Santiago Pastorino up for a challenge? :)

nikomatsakis (May 23 2018 at 21:57, on Zulip):

I think that investigating lifetime-errors/ex2c-push-inference-variable.nll.stderr would be great, and might give you some insight into how to solve some of the other tricky ones

nikomatsakis (May 23 2018 at 21:57, on Zulip):

I'm gonna have to run though shortly

nikomatsakis (May 23 2018 at 21:58, on Zulip):

it's interesting to compare the NLL error and the non-NLL from the paper doc though

Santiago Pastorino (May 23 2018 at 21:59, on Zulip):

:+1:

nikomatsakis (May 23 2018 at 22:04, on Zulip):

@Santiago Pastorino opened up https://github.com/rust-lang/rust/issues/51012

nikomatsakis (May 24 2018 at 00:19, on Zulip):

@Santiago Pastorino ever figure out what was up w/ the span?

Santiago Pastorino (May 24 2018 at 00:22, on Zulip):

will be back to the chat in a bit

Santiago Pastorino (May 24 2018 at 00:22, on Zulip):

will you be around?

Santiago Pastorino (May 24 2018 at 00:22, on Zulip):

short answer is ... kind is Call

Santiago Pastorino (May 24 2018 at 00:23, on Zulip):

couldn’t debug properly yet because I had no more time

nikomatsakis (May 24 2018 at 00:26, on Zulip):

will you be around?

maybe

Santiago Pastorino (May 24 2018 at 12:52, on Zulip):

@nikomatsakis so as I was saying yesterday kind is of type Call and not AssignOp

Santiago Pastorino (May 24 2018 at 12:52, on Zulip):

well in reality it starts as kind Scope

Santiago Pastorino (May 24 2018 at 12:53, on Zulip):
DEBUG 2018-05-24T12:45:35Z: rustc_mir::build::expr::stmt: expr=Expr { ty: (), temp_lifetime: Some(Node(ItemLocalId(10))), span: src/test/ui/augmented-assignments.rs:25:5: 28:6, kind: Scope { region_scope: Node(ItemLocalId(9)), lint_level: Explicit(NodeId(34)), value: Mirror(Expr { ty: (), temp_lifetime: Some(Node(ItemLocalId(10))), span: src/test/ui/augmented-assignments.rs:25:5: 28:6, kind: Call { ty: for<'r> fn(&'r mut Int, Int) {<Int as std::ops::AddAssign>::add_assign}, fun: Mirror(Expr { ty: for<'r> fn(&'r mut Int, Int) {<Int as std::ops::AddAssign>::add_assign}, temp_lifetime: Some(Node(ItemLocalId(10))), span: src/test/ui/augmented-assignments.rs:25:5: 28:6, kind: Literal { literal: const std::ops::AddAssign::add_assign } }), args: [Hair(expr(32: x)), Hair(expr(33: x))] } }) } }
Santiago Pastorino (May 24 2018 at 12:53, on Zulip):

and Scope arm does ...

Santiago Pastorino (May 24 2018 at 12:53, on Zulip):
                this.in_scope((region_scope, source_info), lint_level, block, |this| {
                    this.stmt_expr(block, value)
                })
Santiago Pastorino (May 24 2018 at 12:54, on Zulip):

and that second call gives an ExprKind::Call

Santiago Pastorino (May 24 2018 at 12:54, on Zulip):

that's because the test does ...

Santiago Pastorino (May 24 2018 at 12:54, on Zulip):
impl AddAssign for Int {
    fn add_assign(&mut self, _: Int) {
        unimplemented!()
    }
}
Santiago Pastorino (May 24 2018 at 12:56, on Zulip):

hmm I guess I know what we can do ... trying ...

nikomatsakis (May 24 2018 at 12:56, on Zulip):

hmm I'm a bit confused :)

nikomatsakis (May 24 2018 at 12:57, on Zulip):

I mean everything you've posted makes sense to me :)

nikomatsakis (May 24 2018 at 12:57, on Zulip):

but one question I have: did you figure out which statement the span comes from in the MIR?

Santiago Pastorino (May 24 2018 at 13:04, on Zulip):

haven't checked that

Santiago Pastorino (May 24 2018 at 13:05, on Zulip):

we have this ...

nikomatsakis (May 24 2018 at 13:05, on Zulip):

OK

Santiago Pastorino (May 24 2018 at 13:05, on Zulip):
            _ => {
                let expr_ty = expr.ty;
                let temp = this.temp(expr.ty.clone(), expr_span);
                unpack!(block = this.into(&temp, block, expr));
                unpack!(block = this.build_drop(block, expr_span, temp, expr_ty));
                block.unit()
            }
Santiago Pastorino (May 24 2018 at 13:06, on Zulip):

wanted to check quickly by doing ...

Santiago Pastorino (May 24 2018 at 13:06, on Zulip):
            ExprKind::Call { ty: _, fun, args: _} => {
                let fun = this.hir.mirror(fun);
                let expr_ty = expr.ty;
                let temp = this.temp(expr.ty.clone(), expr_span);
                unpack!(block = this.into(&temp, block, expr));
                unpack!(block = this.build_drop(block, fun.span, temp, expr_ty));
                block.unit()
            }
Santiago Pastorino (May 24 2018 at 13:06, on Zulip):

basically for call change expr.span with fun.span

Santiago Pastorino (May 24 2018 at 13:08, on Zulip):
152 |                 unpack!(block = this.into(&temp, block, expr));
    |                                                         ^^^^ value used here after move
Santiago Pastorino (May 24 2018 at 13:08, on Zulip):

:)

pnkfelix (May 24 2018 at 13:12, on Zulip):

@Santiago Pastorino do you need to use ref fun or something?

pnkfelix (May 24 2018 at 13:12, on Zulip):

/me hasn't looked carefully at surrounding context; just a guess from the code as listed

Santiago Pastorino (May 24 2018 at 13:14, on Zulip):

yeah, tried that but mirror need to take ownership of fun after that

Santiago Pastorino (May 24 2018 at 13:15, on Zulip):
error[E0507]: cannot move out of borrowed content
   --> librustc_mir/build/expr/stmt.rs:149:43
    |
149 |                 let fun = this.hir.mirror(*fun);
    |                                           ^^^^ cannot move out of borrowed content

error[E0505]: cannot move out of `expr` because it is borrowed
   --> librustc_mir/build/expr/stmt.rs:152:57
    |
148 |             ExprKind::Call { ty: _, ref fun, args: _} => {
    |                                     ------- borrow of `expr.kind.fun` occurs here
...
152 |                 unpack!(block = this.into(&temp, block, expr));
    |                                                         ^^^^ move out of `expr` occurs here
Santiago Pastorino (May 24 2018 at 13:16, on Zulip):

@pnkfelix if I do ref fun :point_up:

nikomatsakis (May 24 2018 at 13:20, on Zulip):

@Santiago Pastorino can you send me a link into the source?

nikomatsakis (May 24 2018 at 13:20, on Zulip):

(the "mirror" stuff is a bit artificial; it is used to convert the HIR "bit by bit" into HAIR, but I've thought about just converting everything in one big pass for a given fn and removing the mirror traits...)

nikomatsakis (May 24 2018 at 13:20, on Zulip):

but if needed we can extend the trait with some peak to "peek" at the span

nikomatsakis (May 24 2018 at 13:21, on Zulip):

that said, usually it is possible to mirror first

nikomatsakis (May 24 2018 at 13:21, on Zulip):

and to pass the mirrored result

Santiago Pastorino (May 24 2018 at 13:27, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/blob/master/src/librustc_mir/build/expr/stmt.rs#L141-L147

Santiago Pastorino (May 24 2018 at 13:28, on Zulip):

I was just trying to extract Call from there

Santiago Pastorino (May 24 2018 at 13:28, on Zulip):

with the code I've showed you

Santiago Pastorino (May 24 2018 at 13:28, on Zulip):

so I can use fun.span

Santiago Pastorino (May 24 2018 at 13:28, on Zulip):

which I'm not 100% sure what it gives but I guess is what I need

nikomatsakis (May 24 2018 at 13:29, on Zulip):

you're saying that this expr_span (from the call) is where you think the error is ultimately coming from?

nikomatsakis (May 24 2018 at 13:29, on Zulip):

@Santiago Pastorino argh so .. what test are we looing at again? (just to make sure you and I are thinknig of the same test)

Santiago Pastorino (May 24 2018 at 13:31, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/test/ui/augmented-assignments.rs

Santiago Pastorino (May 24 2018 at 13:31, on Zulip):

you're saying that this expr_span (from the call) is where you think the error is ultimately coming from?

yes

Santiago Pastorino (May 24 2018 at 13:32, on Zulip):

I'm not 100% sure what expr.span and what fun.span are exactly

Santiago Pastorino (May 24 2018 at 13:32, on Zulip):

I'd need to investigate a bit more

Santiago Pastorino (May 24 2018 at 13:32, on Zulip):

was just trying to quickly test

nikomatsakis (May 24 2018 at 13:32, on Zulip):

well if you have foo(bar) then expr_span would cover the whole call, and fun.span would just cover foo

nikomatsakis (May 24 2018 at 13:32, on Zulip):

that said, I think the reaosn you are seeing this here

Santiago Pastorino (May 24 2018 at 13:32, on Zulip):

:)

Santiago Pastorino (May 24 2018 at 13:32, on Zulip):

that's was my guess

nikomatsakis (May 24 2018 at 13:32, on Zulip):

is because x += y is being desugared into a call to a trait method

Santiago Pastorino (May 24 2018 at 13:32, on Zulip):

so ... doing fun.span would work

Santiago Pastorino (May 24 2018 at 13:33, on Zulip):

yeah += is the fun

nikomatsakis (May 24 2018 at 13:33, on Zulip):

yeah so I'm not sure just how the spans got setup in that case

nikomatsakis (May 24 2018 at 13:33, on Zulip):

we'd have to consult the HAIR desugaring

Santiago Pastorino (May 24 2018 at 13:33, on Zulip):

now the thing is how do I get rid of borrowck issues with the code I'm trying to write :P

nikomatsakis (May 24 2018 at 13:33, on Zulip):

specifically this function call here

nikomatsakis (May 24 2018 at 13:35, on Zulip):

tracing through a bit I suspect the span on fun might be the same as expr_span =)

Santiago Pastorino (May 24 2018 at 13:36, on Zulip):

:(

nikomatsakis (May 24 2018 at 13:37, on Zulip):

interesting problem :)

nikomatsakis (May 24 2018 at 13:37, on Zulip):

see, here, my sense is that the span of += is perfect to use for the span of the "function being called" in x += y :)

nikomatsakis (May 24 2018 at 13:38, on Zulip):

well, actually, this is coming back to a concern I had early on

nikomatsakis (May 24 2018 at 13:38, on Zulip):

remember @pnkfelix when I was saying that we might want to extend MIR to have each Lvalue have a span?

nikomatsakis (May 24 2018 at 13:38, on Zulip):

that seems relevant here

nikomatsakis (May 24 2018 at 13:38, on Zulip):

we are using the span of the statement but really we want the span (I suspect) of the Place within the statement

pnkfelix (May 24 2018 at 13:39, on Zulip):

yeah, that or have assignments carry multiple spans...?

pnkfelix (May 24 2018 at 13:39, on Zulip):

(but that seems silly)

pnkfelix (May 24 2018 at 13:39, on Zulip):

((although it might be less painful to implement in the short term...?))

nikomatsakis (May 24 2018 at 13:40, on Zulip):

hmm

nikomatsakis (May 24 2018 at 13:40, on Zulip):

this is not an assignment

nikomatsakis (May 24 2018 at 13:40, on Zulip):

it's an overloaded method call

Keith Yeung (May 24 2018 at 13:40, on Zulip):

so i'm looking at closure diagnostics rn, and one thing i immediately hit upon is the concept of "free regions"

Keith Yeung (May 24 2018 at 13:40, on Zulip):

i've no idea what those are

nikomatsakis (May 24 2018 at 13:41, on Zulip):

@Keith Yeung I was going to point you at the rustc-guide coverage of the topic, but it appears I left it TBD :(

nikomatsakis (May 24 2018 at 13:42, on Zulip):

a "free region" (aka "universal region") is basically a lifetime parameter

nikomatsakis (May 24 2018 at 13:42, on Zulip):

or some lifetime that appears in the fn signature

nikomatsakis (May 24 2018 at 13:46, on Zulip):

@Santiago Pastorino well.. I'm not sure what's the best way to continue. This may turn into a larger refactoring.

nikomatsakis (May 24 2018 at 13:46, on Zulip):

that said, I think that maybe we should shelve this issue, because I'm not sure just how bad it is

nikomatsakis (May 24 2018 at 13:46, on Zulip):

that is, I Think it looks extra bad because the source is written in such a way that it spans multiple lines

nikomatsakis (May 24 2018 at 13:47, on Zulip):

though I think long term we do want to modify mir Place to carry a span

nikomatsakis (May 24 2018 at 13:47, on Zulip):

but that will mean reworking the borrowck a bit

nikomatsakis (May 24 2018 at 13:47, on Zulip):

it's probably not that hard really

nikomatsakis (May 24 2018 at 13:47, on Zulip):

it might really be needed to solve a bunch of diagnostic spans actually

Keith Yeung (May 24 2018 at 13:47, on Zulip):

i at least found out where exactly the error message is generated

Keith Yeung (May 24 2018 at 13:47, on Zulip):

/me goes back to sleep for a while before his day starts

nikomatsakis (May 24 2018 at 13:48, on Zulip):

@Keith Yeung I can try to give you some tips later perhaps

pnkfelix (May 24 2018 at 13:55, on Zulip):

(in response to "these are overloaded method calls") okay, true. But method calls as terminators are just really heavy weight instances of assignments,right?

pnkfelix (May 24 2018 at 13:55, on Zulip):

(anyway it was just a throwaway remark)

nikomatsakis (May 24 2018 at 14:00, on Zulip):

@pnkfelix depends on your POV :) my point is that adding just to StmtAssign would not suffice

pnkfelix (May 24 2018 at 14:01, on Zulip):

agreed

pnkfelix (May 24 2018 at 14:02, on Zulip):

It was really just a question of "would a series of band-aid extensions to individual Stmt/Terminator variants be 'simpler' to implement than trying to add span to every Lvalue

Santiago Pastorino (May 24 2018 at 14:04, on Zulip):

@Santiago Pastorino well.. I'm not sure what's the best way to continue. This may turn into a larger refactoring.

:+1:

nikomatsakis (May 24 2018 at 14:18, on Zulip):

It was really just a question of "would a series of band-aid extensions to individual Stmt/Terminator variants be 'simpler' to implement than trying to add span to every Lvalue

yeah — and I guess the answer is ... maybe?

nikomatsakis (May 24 2018 at 14:18, on Zulip):

another idea: we could refactor Operand to have a span

nikomatsakis (May 24 2018 at 14:18, on Zulip):

that woudlnt' be too hard, might suffice

Reed Koser (May 24 2018 at 15:01, on Zulip):

a "free region" (aka "universal region") is basically a lifetime parameter

Just to double check my understanding, these are also called "universal regions" right? Or are universal regions only things that actually live for every point in the CFG

nikomatsakis (May 24 2018 at 15:02, on Zulip):

same thing

nikomatsakis (May 24 2018 at 15:02, on Zulip):

I've been wrestling with the best terminology

nikomatsakis (May 24 2018 at 15:02, on Zulip):

"free" is a kind of general PL term though

nikomatsakis (May 24 2018 at 15:02, on Zulip):

referring to something that is not "bound" within a term or expression

nikomatsakis (May 24 2018 at 15:03, on Zulip):

so e.g. if we say that the statement S is let x = y + z, then y and z "appear free" in S, but x is bound in S

nikomatsakis (May 24 2018 at 15:03, on Zulip):

I was calling them "universal regions" because they are universally quantified ("forall")

nikomatsakis (May 24 2018 at 15:03, on Zulip):

so e.g. fn foo<'a> (...) works for any region 'a

nikomatsakis (May 24 2018 at 15:03, on Zulip):

just as fn foo<T>(...) works any type T

Reed Koser (May 24 2018 at 15:09, on Zulip):

So they are both free and universal, and we're lazy and occasionally only use one of those words?

Reed Koser (May 24 2018 at 15:09, on Zulip):

There is also explicit universal quantification syntax though

Reed Koser (May 24 2018 at 15:09, on Zulip):

which I think is used for declaring closures, where the incoming regions are neither free nor universal (unless declared to be so via for <'a> syntax if I'm understanding correctly)

Reed Koser (May 24 2018 at 15:09, on Zulip):

I guess they're always free+universal from the perspective of type checking within the closure themselves? just not the body of the surrounding fn

Jake Goulding (May 24 2018 at 15:24, on Zulip):

as an observer, please dont let the super type theory leak out in the messages :hear_no_evil:

Jake Goulding (May 24 2018 at 15:24, on Zulip):

No skolemization redux

Jake Goulding (May 24 2018 at 15:26, on Zulip):

I feel like "generic lifetime parameter" is an understood term. Could extend to "implicit G..L..P.." and "higher-ranked g..l..p" as needed

nikomatsakis (May 24 2018 at 15:56, on Zulip):

So they are both free and universal, and we're lazy and occasionally only use one of those words?

yes, and — specifically — depending on your POV and the context, there may be free regions that are not universal

nikomatsakis (May 24 2018 at 15:57, on Zulip):

No skolemization redux

skolemization was never .. intentionally in the error messages :) but yes, of course I agree.. I don't think anyone was talking about the phrasing of error messages here.

Jake Goulding (May 24 2018 at 16:03, on Zulip):

I just note its the diagnostics thread, so I was worried :wink:

qmx (May 24 2018 at 16:05, on Zulip):

c'mon, it took me a while to learn how to spell skolemization properly, now I want to inflict this pain on others </sarcasm>

Keith Yeung (May 25 2018 at 05:02, on Zulip):

"free regions associated with closures" how?

Keith Yeung (May 25 2018 at 05:03, on Zulip):

i'm guessing that happens when you have a lifetime'd variable as a fn arg and then you reference it in a closure created inside the fn body?

pnkfelix (May 25 2018 at 08:19, on Zulip):

@Keith Yeung well, [issue-45983.rs(https://github.com/rust-lang/rust/blob/master/src/test/ui/borrowck/issue-45983.rs) shows things like fn give_any<F: for<'r> FnOnce(&'r ())>(f: F) { ... } and then elsewhere a call like give_any(|y| x = Some(y));

Keith Yeung (May 25 2018 at 08:19, on Zulip):

was in fact looking at that

pnkfelix (May 25 2018 at 08:21, on Zulip):

yeah I'd suggest perhaps start by surveying all the tests

pnkfelix (May 25 2018 at 08:21, on Zulip):

to identify the cases that lead to this

nikomatsakis (May 25 2018 at 08:49, on Zulip):

@Keith Yeung not sure if this answers your question, but you may find this comment interesting — it describes the different "categories" of universal regions and how each arises

pnkfelix (May 25 2018 at 09:48, on Zulip):

I have to say: Working on rust-lang/rust#51025, I become contintually impressed by how much more sane Rust seems to be with NLL in place.

nikomatsakis (May 25 2018 at 09:49, on Zulip):

I'm pretty excited to see its effects

pnkfelix (May 25 2018 at 09:49, on Zulip):

e.g. there were behaviors before that one sort of justified by saying "ah there are borrows, and lifetimes are lexical, so the compiler rejects this code, but oh it accepts this one, watch me wave my hands wildly"

pnkfelix (May 25 2018 at 09:50, on Zulip):

I was quite surprised to see this test in particular: vec_refs_data_with_early_death.rs

pnkfelix (May 25 2018 at 09:50, on Zulip):

Of all things that one would expect #[may_dangle] to take care of ...

pnkfelix (May 25 2018 at 10:54, on Zulip):

woot: https://github.com/rust-lang/rust/pull/51057

Matthew Jasper (May 28 2018 at 13:24, on Zulip):

So I've got my cannot-move-from errors branch using an approach that I'm a bit happier with.

Matthew Jasper (May 28 2018 at 13:24, on Zulip):

If anyone wants to suggest anything some examples of the output are here: https://gist.github.com/matthewjasper/15eac85086ee6a8adb806450b046c57a.

nikomatsakis (May 28 2018 at 13:25, on Zulip):

@Matthew Jasper ooh, nice

nikomatsakis (May 29 2018 at 12:52, on Zulip):

@Matthew Jasper is there an open issue that you know of? did you make a PR?

nikomatsakis (May 29 2018 at 13:25, on Zulip):

I decided to call it #45699

nikomatsakis (May 29 2018 at 16:47, on Zulip):

@Santiago Pastorino are you looking at https://github.com/rust-lang/rust/issues/51031 ?

Santiago Pastorino (May 29 2018 at 16:59, on Zulip):

yes

Santiago Pastorino (May 29 2018 at 16:59, on Zulip):

was starting now

Santiago Pastorino (May 29 2018 at 17:00, on Zulip):

do you still want me to do it?

nikomatsakis (May 29 2018 at 17:02, on Zulip):

yep

Santiago Pastorino (May 29 2018 at 17:03, on Zulip):

tell me :)

Santiago Pastorino (May 29 2018 at 17:05, on Zulip):

ahh sorry, I read it wrong

Santiago Pastorino (May 29 2018 at 17:05, on Zulip):

ok starting this thing

Santiago Pastorino (May 29 2018 at 17:11, on Zulip):

@nikomatsakis how do I test this?

Santiago Pastorino (May 29 2018 at 17:11, on Zulip):

or better asked, what's the example you're using that triggers this issue?

Santiago Pastorino (May 29 2018 at 17:12, on Zulip):

ahh sorry sorry, Felix, added a lot of examples

Santiago Pastorino (May 29 2018 at 17:12, on Zulip):

forget about what I've said :P

Santiago Pastorino (May 29 2018 at 17:23, on Zulip):

@nikomatsakis is there a way now to compile in nll mode without changing the file?

Santiago Pastorino (May 29 2018 at 17:24, on Zulip):

rustc -Znll :'(

nikomatsakis (May 29 2018 at 17:25, on Zulip):

@Santiago Pastorino there is, -Zborrowck=mir -Zallow-two-phase-borrows I think will do it

nikomatsakis (May 29 2018 at 17:25, on Zulip):

I was just complaining to @pnkfelix earlier that this is too complicated :)

nikomatsakis (May 29 2018 at 17:26, on Zulip):

I'd take a PR to change it to -Zdisallow-two-phase-borrows =)

nikomatsakis (May 29 2018 at 17:26, on Zulip):

or perhaps -Ztwo-phase-borrows=no with -Zallow-two-phase-borrows being a deprecated no-op (I think it apperas in some places like perf?)

Santiago Pastorino (May 29 2018 at 17:26, on Zulip):

ya, saw that way yesterday

Santiago Pastorino (May 29 2018 at 17:26, on Zulip):

I'd take a PR to change it to -Zdisallow-two-phase-borrows =)

don't understand why that?

Santiago Pastorino (May 29 2018 at 17:26, on Zulip):

or what you mean exactly

nikomatsakis (May 29 2018 at 17:27, on Zulip):

so right now #![feature(nll)] implies those two -Z flags

pnkfelix (May 29 2018 at 17:27, on Zulip):

We want to retain the ability to isolate two phase borrows

pnkfelix (May 29 2018 at 17:27, on Zulip):

For debugging

nikomatsakis (May 29 2018 at 17:27, on Zulip):

what I mean is that I think we should default to the behavior that #![feature(nll)] gives you

nikomatsakis (May 29 2018 at 17:27, on Zulip):

and make you give add'l flags to disable 2-phase borrows

nikomatsakis (May 29 2018 at 17:27, on Zulip):

or make other changes

Santiago Pastorino (May 29 2018 at 17:28, on Zulip):

so you meant to say that -Zborrowck=mir implies -Ztwo-phase-borrows?

Santiago Pastorino (May 29 2018 at 17:29, on Zulip):

and in order to turn off you can -Zdisallow-two-phase-borrows?

Santiago Pastorino (May 29 2018 at 17:29, on Zulip):

did I follow correctly

nikomatsakis (May 29 2018 at 17:31, on Zulip):

@Santiago Pastorino yes

Santiago Pastorino (May 29 2018 at 17:31, on Zulip):

ok

Santiago Pastorino (May 29 2018 at 17:32, on Zulip):

or even -Znll imply those things and add -Zdisallow-two-phase-borrows

Santiago Pastorino (May 29 2018 at 17:34, on Zulip):

was reading comments on https://github.com/rust-lang/rust/issues/51031

Santiago Pastorino (May 29 2018 at 17:35, on Zulip):
Notably, this call to is_mutable — when it returns Err — also returns some information about why this path is not mutable:
Santiago Pastorino (May 29 2018 at 17:35, on Zulip):

I guess what you mean is that given the if let the result when Err is being discarded and we need that

Santiago Pastorino (May 29 2018 at 17:36, on Zulip):

sorry if I ask trivial questions but from time to time I'm pretty sure I got 98% of the wording on things because of english issues and I want 100% :D

nikomatsakis (May 29 2018 at 17:36, on Zulip):

yes, right

Santiago Pastorino (May 29 2018 at 17:36, on Zulip):

and the other thing is

Santiago Pastorino (May 29 2018 at 17:37, on Zulip):

you mentioned calling report_illegal_reassignment function with parts of that Err stuff

Santiago Pastorino (May 29 2018 at 17:37, on Zulip):

that thing is already being called in the iter_incoming loop

Santiago Pastorino (May 29 2018 at 17:37, on Zulip):

is it there being called with other purposes?

nikomatsakis (May 29 2018 at 17:39, on Zulip):

don't know :)

nikomatsakis (May 29 2018 at 17:39, on Zulip):

but I doubt it

nikomatsakis (May 29 2018 at 17:39, on Zulip):

you'd have to ripgrep around

nikomatsakis (May 29 2018 at 17:40, on Zulip):

I should mention, once we fix this part, there are probably a few other cases w'ell want to tweak in similar ways — e.g., when you have an example like &mut foo where let foo

Santiago Pastorino (May 29 2018 at 17:41, on Zulip):

ok

pnkfelix (May 29 2018 at 21:54, on Zulip):

@nikomatsakis Is rustc::infer::lexical_region_resolve intrinsically tied to AST-borrowck? Or will that remain in place even after a full switch to NLL ?

nikomatsakis (May 29 2018 at 21:54, on Zulip):

I hope to kill it after a full switch to NLL

nikomatsakis (May 29 2018 at 21:54, on Zulip):

but it's going to take a bit of work

nikomatsakis (May 29 2018 at 21:54, on Zulip):

it is also used to report some other errors unrelated to borrowck

pnkfelix (May 29 2018 at 21:55, on Zulip):

So the cases where our only good diagnostic spans are attached to that "not reporting region error due to nll" message ...

nikomatsakis (May 29 2018 at 21:55, on Zulip):

well, in the long term, we should get good spans from borrowck

nikomatsakis (May 29 2018 at 21:55, on Zulip):

but this is that case I was tlaking about

nikomatsakis (May 29 2018 at 21:55, on Zulip):

where regionck today reports an error

nikomatsakis (May 29 2018 at 21:55, on Zulip):

that NLL ought to accept

nikomatsakis (May 29 2018 at 21:55, on Zulip):

I am not sure whether we want to "silence" those in this brave new future or what

nikomatsakis (May 29 2018 at 21:55, on Zulip):

maybe we do

nikomatsakis (May 29 2018 at 21:55, on Zulip):

just like we silence AST borrowck

pnkfelix (May 29 2018 at 21:56, on Zulip):

/me pops his mental stack a few frames

pnkfelix (May 29 2018 at 21:57, on Zulip):

The overarching thing I'm trying to figure out right now

pnkfelix (May 29 2018 at 21:57, on Zulip):

is how many bugs that I'm about to file

pnkfelix (May 29 2018 at 21:57, on Zulip):

are actually instances of "this needs to be fixed by producing better explanations for how our region constraints arose"

pnkfelix (May 29 2018 at 21:58, on Zulip):

A concrete example is: https://github.com/rust-lang/rust/blob/master/src/test/ui/in-band-lifetimes/impl/dyn-trait.stderr

pnkfelix (May 29 2018 at 21:58, on Zulip):

where AST-borrowck produces a readable series of lines explaining the constraints that led us to require 'a to outlive 'static

nikomatsakis (May 29 2018 at 21:59, on Zulip):

readable

maybe... but better than NLL I suppose :)

pnkfelix (May 29 2018 at 21:59, on Zulip):

but NLL just says "'a needs to outlive 'static`", and doesn't say why

nikomatsakis (May 29 2018 at 21:59, on Zulip):

I don't quite understand the question

pnkfelix (May 29 2018 at 21:59, on Zulip):

(Yeah I debated about how to phrase that)

nikomatsakis (May 29 2018 at 21:59, on Zulip):

I guess you are saying: Is this something we have to fix, or could we rely on the old errors here?

pnkfelix (May 29 2018 at 21:59, on Zulip):

(in terms of whether to assert readability or not)

pnkfelix (May 29 2018 at 22:00, on Zulip):

I guess you are saying: Is this something we have to fix, or could we rely on the old errors here?

Well, I haven't even gotten that far in my thinking.

pnkfelix (May 29 2018 at 22:00, on Zulip):

I'm honestly just trying to create a github issue

pnkfelix (May 29 2018 at 22:00, on Zulip):

but I wasn't sure whether I should be looking for a pre-existing one

pnkfelix (May 29 2018 at 22:00, on Zulip):

Or rather, I tried looking for a pre-existing one

pnkfelix (May 29 2018 at 22:01, on Zulip):

but none of the search terms I thought would yield results (like "regionck") actually did

nikomatsakis (May 29 2018 at 22:01, on Zulip):

I think that sort of falls under @Reed Koser's "ungreat" issue?

nikomatsakis (May 29 2018 at 22:01, on Zulip):

https://github.com/rust-lang/rust/issues/49397

nikomatsakis (May 29 2018 at 22:01, on Zulip):

(I am thinking it's probably time for us/me to do some diagnostic sprinting, though)

pnkfelix (May 29 2018 at 22:01, on Zulip):

the problem is that @Reed Koser 's issue there has multiple ungreat things

pnkfelix (May 29 2018 at 22:01, on Zulip):

there's the fact that it uses the empty string

nikomatsakis (May 29 2018 at 22:02, on Zulip):

true

nikomatsakis (May 29 2018 at 22:02, on Zulip):

but I think they're all kinda related

pnkfelix (May 29 2018 at 22:02, on Zulip):

Well, you have more experience than I do writing tutorial instructions

pnkfelix (May 29 2018 at 22:02, on Zulip):

I was assuming that it was better

pnkfelix (May 29 2018 at 22:02, on Zulip):

if the issue were more targetted

pnkfelix (May 29 2018 at 22:03, on Zulip):

problems like "present something better than an empty string for this region"

pnkfelix (May 29 2018 at 22:03, on Zulip):

are in my mind separate from

nikomatsakis (May 29 2018 at 22:03, on Zulip):

well partly it's beacuse I don't expect to be able to mentor this one out :)

pnkfelix (May 29 2018 at 22:03, on Zulip):

"present the series of constraints that cause us to relate these two lifetimes"

pnkfelix (May 29 2018 at 22:03, on Zulip):

heh

pnkfelix (May 29 2018 at 22:03, on Zulip):

So that may be getting at something

pnkfelix (May 29 2018 at 22:03, on Zulip):

I think in your head, you know something fundamental that needs to change

pnkfelix (May 29 2018 at 22:04, on Zulip):

in order to even provide the infrastructure that we will need

pnkfelix (May 29 2018 at 22:04, on Zulip):

to produce useful diagnsotics

pnkfelix (May 29 2018 at 22:04, on Zulip):

in my head, even after that fundemental change is made

pnkfelix (May 29 2018 at 22:04, on Zulip):

there is still plumbing that will need to be done

nikomatsakis (May 29 2018 at 22:05, on Zulip):

I think we actually probably have the info

pnkfelix (May 29 2018 at 22:05, on Zulip):

So I was wondering if we could separate the fundamental kernel from the plumbing that hooks that kernel up to all the diagnsotics

nikomatsakis (May 29 2018 at 22:05, on Zulip):

that is, we have the full set of constraints etc

nikomatsakis (May 29 2018 at 22:05, on Zulip):

though we don't have quite as many details as the old code

nikomatsakis (May 29 2018 at 22:05, on Zulip):

that is, we don't have "causes"

nikomatsakis (May 29 2018 at 22:05, on Zulip):

just the set of constraints

nikomatsakis (May 29 2018 at 22:05, on Zulip):

(I'm not entirely convinced the old causes were good though)

nikomatsakis (May 29 2018 at 22:05, on Zulip):

(which is why I didn't recreate them)

nikomatsakis (May 29 2018 at 22:06, on Zulip):

but yes, you're probably right re: plumbing

nikomatsakis (May 29 2018 at 22:06, on Zulip):

I sort of expected for this case to just have to "play around" with various examples

nikomatsakis (May 29 2018 at 22:06, on Zulip):

maybe I'm being too much of a perfectionist

pnkfelix (May 29 2018 at 22:06, on Zulip):

Okay so that may be a question we need to resolve: Are causes a must-have for deploying NLL

nikomatsakis (May 29 2018 at 22:06, on Zulip):

and we should try to reproduce the old "egads I need a pager" style errors

pnkfelix (May 29 2018 at 22:07, on Zulip):

I assume you would agree that this is not acceptable in the long term: https://github.com/rust-lang/rust/blob/master/src/test/ui/in-band-lifetimes/impl/dyn-trait.nll.stderr

nikomatsakis (May 29 2018 at 22:07, on Zulip):

clearly not

pnkfelix (May 29 2018 at 22:07, on Zulip):

would you then interpret that as at least evidence that we need causes?

pnkfelix (May 29 2018 at 22:07, on Zulip):

or do you expect that there is some way to usefully present the constraints?

nikomatsakis (May 29 2018 at 22:08, on Zulip):

I suspect we need some form of causes, yes

pnkfelix (May 29 2018 at 22:08, on Zulip):

in a way that the user would be able to take action?

pnkfelix (May 29 2018 at 22:08, on Zulip):

okay

nikomatsakis (May 29 2018 at 22:08, on Zulip):

but I sort of want to drive the causes from the errors we want to make

pnkfelix (May 29 2018 at 22:08, on Zulip):

I'm going to just say "NLL needs causes" for the issue I file in association with this entry in the stderr-deviations Paper then

Reed Koser (May 29 2018 at 22:13, on Zulip):

I'm willing to work on diagnostics stuff if that's what needs to be done. I have a branch lying around somewhere that sort-of fixed the problems, but the code was pretty gross and the output ended up more verbose than I'd like

Reed Koser (May 29 2018 at 22:14, on Zulip):

*the problems = the specific case the "ungreat" issue was targetting

Reed Koser (May 29 2018 at 22:16, on Zulip):

I haven't had a chance to take a look at the diagnostics triage doc yet. I've been pretty busy for the last couple weeks and that doesn't seem like it'll significantly lighten up any time soon

Reed Koser (May 29 2018 at 22:17, on Zulip):

hopefully I'll be able to find a few hours a week to hack on this stuff though

pnkfelix (May 29 2018 at 23:38, on Zulip):

okay I just finished a review of all the cases in https://paper.dropbox.com/doc/NLL-stderr-diagnostic-deviations-KzroSvNgQ7lip3WQyiLTa that were updated by PR #51025

pnkfelix (May 29 2018 at 23:38, on Zulip):

almost all of the rows labelled "weak test" are now "100!" instead

pnkfelix (May 29 2018 at 23:39, on Zulip):

the only exceptions are: 1. some weak tests that were unrelated to #51025

pnkfelix (May 29 2018 at 23:39, on Zulip):

and 2. https://github.com/rust-lang/rust/blob/master/src/test/ui/span/borrowck-let-suggestion-suffixes.nll.stderr

pnkfelix (May 29 2018 at 23:40, on Zulip):

the latter I think is still a little weak. In NLL it reports 3 errors while AST-borrowck reports 4 errors, and I think this is because the test is not doing a good enough job of poking the dropck

pnkfelix (May 29 2018 at 23:41, on Zulip):

oh, yeah: What prompted this review is that I think I have finished tagging all of the non-"meh" rows with github issues

pnkfelix (May 29 2018 at 23:42, on Zulip):

which means we can probably retire https://paper.dropbox.com/doc/NLL-stderr-diagnostic-deviations-KzroSvNgQ7lip3WQyiLTa itself at any time that we decide its not pulling its weight anymore

pnkfelix (May 29 2018 at 23:42, on Zulip):

(for now it is kind of fun to at least scroll through it and see how many cases are now tagged as :hundred_points: )

pnkfelix (May 31 2018 at 11:17, on Zulip):

/me suddenly realized that an actual spreadsheet may have been much better than a Paper doc for NLL-stderr-diagnostic-deviations

pnkfelix (May 31 2018 at 11:19, on Zulip):

luckily cut-and-paste into a google sheet seems like it actually ... works... (even preserves urls amazingly)

Santiago Pastorino (May 31 2018 at 12:51, on Zulip):

@pnkfelix link?

pnkfelix (May 31 2018 at 12:52, on Zulip):

https://docs.google.com/spreadsheets/d/16Hoy5CASZOYkjZO0p85xrUberY2PnxeAHqxlT4VFurM/edit?usp=sharing

pnkfelix (May 31 2018 at 12:53, on Zulip):

its not meant to be permanent though

Santiago Pastorino (May 31 2018 at 12:53, on Zulip):

can each of these "channels" which I see are called topics have a topic like on IRC or a description

Santiago Pastorino (May 31 2018 at 12:53, on Zulip):

#diagnostics and all these threads are called topics

Santiago Pastorino (May 31 2018 at 12:53, on Zulip):

so my guess is that is not possible

pnkfelix (May 31 2018 at 12:53, on Zulip):

I just wanted to sort it by the issue number to ease creating a summary table at the bottom of the NLL stderr deviations Paper

Santiago Pastorino (May 31 2018 at 12:53, on Zulip):

could be nice to be able to put this spreadsheet in the thread description

pnkfelix (May 31 2018 at 12:54, on Zulip):

let me note again: Its not meant to be permanent

pnkfelix (May 31 2018 at 12:54, on Zulip):

just a sketch area

Santiago Pastorino (May 31 2018 at 12:54, on Zulip):

I just wanted to sort it by the issue number to ease creating a summary table at the bottom of the NLL stderr deviations Paper

:+1:

pnkfelix (May 31 2018 at 12:55, on Zulip):

(though I do admit that if I put more effort into my spreadsheet-fu, I'd be able to make something that automatically generated the counts in that table)

Santiago Pastorino (May 31 2018 at 12:58, on Zulip):

:)

Santiago Pastorino (May 31 2018 at 12:58, on Zulip):

columns are a bit tight (if that's the right way to say what I'm thinking)

Santiago Pastorino (May 31 2018 at 12:58, on Zulip):

some text doesn't fit and it's a bit harder to read it

pnkfelix (May 31 2018 at 13:01, on Zulip):

/me considers deleting the spreadsheet now that he's done with it

Santiago Pastorino (May 31 2018 at 13:17, on Zulip):

ahh ok :)

Santiago Pastorino (May 31 2018 at 13:17, on Zulip):

I'm not using it just wanted to take a look

Santiago Pastorino (May 31 2018 at 13:17, on Zulip):

so don't worry about me

Santiago Pastorino (May 31 2018 at 13:18, on Zulip):

I still need to start with my diagnostics issue

Santiago Pastorino (May 31 2018 at 13:18, on Zulip):

and it's clear what's going on

nikomatsakis (May 31 2018 at 14:04, on Zulip):

let me note again: Its not meant to be permanent

why?

nikomatsakis (May 31 2018 at 14:04, on Zulip):

that is, @pnkfelix, why would we not have a spreadsheet that we aim to keep up to date?

nikomatsakis (May 31 2018 at 14:05, on Zulip):

it's maybe useful to have correlations in both ways I think

pnkfelix (May 31 2018 at 14:05, on Zulip):

we can. but that was not the purpose of that sheet

nikomatsakis (May 31 2018 at 14:05, on Zulip):

or maybe it's too much work, not sure

nikomatsakis (May 31 2018 at 14:05, on Zulip):

ok

pnkfelix (May 31 2018 at 14:05, on Zulip):

(it could well be repurposed for such. I just saw @Santiago Pastorino 's feedback and thought "I dont want to worry about this right now)

Santiago Pastorino (May 31 2018 at 14:37, on Zulip):

hehe don't worry about my feedback, I can just deal with the way it is :)

Santiago Pastorino (May 31 2018 at 14:38, on Zulip):

I just needed to increase cells span a bit

Santiago Pastorino (May 31 2018 at 14:38, on Zulip):

but don't worry about that

Santiago Pastorino (May 31 2018 at 14:38, on Zulip):

I'm not using the spreadsheet

Santiago Pastorino (May 31 2018 at 18:42, on Zulip):

I was starting to go over https://github.com/rust-lang/rust/issues/51031

Santiago Pastorino (May 31 2018 at 18:43, on Zulip):

told Niko that I'm going to try to investigate a bit more about structures involved there because I use to forget or not know what are most of the structures that we use around

pnkfelix (May 31 2018 at 18:43, on Zulip):

great I'll put your name in there

pnkfelix (May 31 2018 at 18:43, on Zulip):

(on the table I've put at the bottom of the paper doc.)

Santiago Pastorino (May 31 2018 at 18:43, on Zulip):

like in that example Context, (Place, Span), Flows, etc

pnkfelix (May 31 2018 at 18:43, on Zulip):

((you know, that paper doc that we also didn't want to become a canonical Database. :) ))

Santiago Pastorino (May 31 2018 at 18:43, on Zulip):

so many things :), going to keep saying stuff and asking questions here

Santiago Pastorino (May 31 2018 at 18:44, on Zulip):

great I'll put your name in there

:+1:

pnkfelix (May 31 2018 at 18:44, on Zulip):

Okay. For once I actually know what all four of those things are

Santiago Pastorino (May 31 2018 at 18:44, on Zulip):

so ...

Santiago Pastorino (May 31 2018 at 18:44, on Zulip):

Context is a Location wrapper that hols a ContextKind which says what's going on in that Location

Santiago Pastorino (May 31 2018 at 18:44, on Zulip):

you can correct me

Santiago Pastorino (May 31 2018 at 18:45, on Zulip):

will try to build an NLL data types Glossary

pnkfelix (May 31 2018 at 18:45, on Zulip):

yes that's right. My memory/intent was that Context would only be used for debugging compiler instrumentation info

pnkfelix (May 31 2018 at 18:46, on Zulip):

as in, knowing the context of a call in a bit of debug! output

Santiago Pastorino (May 31 2018 at 18:46, on Zulip):

ok, btw, one weird thing is that https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/mod.rs#L1375-L1380 is receiving the Context

Santiago Pastorino (May 31 2018 at 18:46, on Zulip):

to pass it in here https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/mod.rs#L1395

Santiago Pastorino (May 31 2018 at 18:47, on Zulip):

and then that's discarded here https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/error_reporting.rs#L598

pnkfelix (May 31 2018 at 18:47, on Zulip):

yeah the Context is not a full call chain

pnkfelix (May 31 2018 at 18:47, on Zulip):

it just reports what part of the MIR we were in when we reached that point

Santiago Pastorino (May 31 2018 at 18:47, on Zulip):

I guess this is to avoid breaking the API

pnkfelix (May 31 2018 at 18:48, on Zulip):

at some point we should be able to discard Context entirely, I think/hope, as long as no one has put in code that actually inspects them

Santiago Pastorino (May 31 2018 at 18:48, on Zulip):

I have seen this happen in several parts of the code

pnkfelix (May 31 2018 at 18:48, on Zulip):

that is, the only thing that should be reading the context is the debug! calls

Santiago Pastorino (May 31 2018 at 18:48, on Zulip):

couldn't be nice to use _context there?

Santiago Pastorino (May 31 2018 at 18:49, on Zulip):

I mean, even when used

pnkfelix (May 31 2018 at 18:49, on Zulip):

you mean name it _context just to make it clear that no one should be reading from it?

Santiago Pastorino (May 31 2018 at 18:49, on Zulip):

because you use it to pass it to a function that doesn't use it

pnkfelix (May 31 2018 at 18:50, on Zulip):

oh I guess some code is reading the context.loc for the purpose of identifying spans

pnkfelix (May 31 2018 at 18:50, on Zulip):

that was not my intent, but we can always replace context with just the loc in the future.

Santiago Pastorino (May 31 2018 at 18:51, on Zulip):

I'm not sure I follow you

Santiago Pastorino (May 31 2018 at 18:51, on Zulip):

all I'm saying is that in check_if_reassignment_to_immutable_state context is just used to pass it to report_illegal_reassignment

pnkfelix (May 31 2018 at 18:52, on Zulip):

sure

Santiago Pastorino (May 31 2018 at 18:52, on Zulip):

but in report_illegal_reassignment context is _context which is just discarded

pnkfelix (May 31 2018 at 18:52, on Zulip):

I'm saying that my intent was that everywhere in the code, the context argument would not have any effect on the compiler's behavior

Santiago Pastorino (May 31 2018 at 18:52, on Zulip):

so I'm saying why don't we use _context in check_if_reassignment_to_immutable_state and pass it to the other function as _context too, kind of in a way to describe that the thing is not used

Santiago Pastorino (May 31 2018 at 18:52, on Zulip):

otherwise just remove from the signature

pnkfelix (May 31 2018 at 18:52, on Zulip):

I have no problem with the idea of naming it _context

Santiago Pastorino (May 31 2018 at 18:53, on Zulip):

but I guess that breaks stuff

pnkfelix (May 31 2018 at 18:53, on Zulip):

but I do not want it removed from the sginature

pnkfelix (May 31 2018 at 18:53, on Zulip):

because I find it useful when adding debug! statemnets

pnkfelix (May 31 2018 at 18:53, on Zulip):

such as here: https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/error_reporting.rs#L560

Santiago Pastorino (May 31 2018 at 18:53, on Zulip):

I understand

pnkfelix (May 31 2018 at 18:53, on Zulip):

not every method has such a debug! statement, but I will sometimes throw them in temporarily

Santiago Pastorino (May 31 2018 at 18:53, on Zulip):

you want it there so you can add debug! calls when you're coding?

pnkfelix (May 31 2018 at 18:53, on Zulip):

exactly

Santiago Pastorino (May 31 2018 at 18:53, on Zulip):

:+1:

pnkfelix (May 31 2018 at 18:54, on Zulip):

and its a lot more effort to rethread the context through everything

Santiago Pastorino (May 31 2018 at 18:54, on Zulip):

that's what I wanted to know :)

pnkfelix (May 31 2018 at 18:54, on Zulip):

so I prefer to keep it

Santiago Pastorino (May 31 2018 at 18:54, on Zulip):

makes sense

pnkfelix (May 31 2018 at 18:54, on Zulip):

but I don't mind naming it _context.

Santiago Pastorino (May 31 2018 at 18:54, on Zulip):

was just trying to understand

pnkfelix (May 31 2018 at 18:54, on Zulip):

I do think we should probably change this code: https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/error_reporting.rs#L269

pnkfelix (May 31 2018 at 18:54, on Zulip):

to not read from context.loc

Santiago Pastorino (May 31 2018 at 18:54, on Zulip):

it may be useful for this kind of things to name it _context to explain that is just for debug purposes or something like that

pnkfelix (May 31 2018 at 18:54, on Zulip):

but ... its a nitpick

Santiago Pastorino (May 31 2018 at 18:55, on Zulip):

you mean, not read from context.loc and pass the loc to the funcion?

pnkfelix (May 31 2018 at 18:55, on Zulip):

yeah that's what I was thinking

pnkfelix (May 31 2018 at 18:55, on Zulip):

but its a really minor detail

Santiago Pastorino (May 31 2018 at 18:55, on Zulip):

:+1:

Santiago Pastorino (May 31 2018 at 18:55, on Zulip):

yes

Santiago Pastorino (May 31 2018 at 18:55, on Zulip):

I see

pnkfelix (May 31 2018 at 18:55, on Zulip):

and we have bigger fish to fry at the moment

Santiago Pastorino (May 31 2018 at 18:55, on Zulip):

thanks, it helps understanding

Santiago Pastorino (May 31 2018 at 18:55, on Zulip):

yeah, I'm not going to do that

Santiago Pastorino (May 31 2018 at 18:56, on Zulip):

just asking to get some understanding of these structs involved

Santiago Pastorino (May 31 2018 at 18:57, on Zulip):

then Place is described like

Santiago Pastorino (May 31 2018 at 18:57, on Zulip):
/// A path to a value; something that can be evaluated without
/// changing or disturbing program state.
Santiago Pastorino (May 31 2018 at 18:58, on Zulip):

I thought more about Place as place in memory

Santiago Pastorino (May 31 2018 at 18:58, on Zulip):

where values are stored

nikomatsakis (May 31 2018 at 18:58, on Zulip):

at some point we should be able to discard Context entirely, I think/hope, as long as no one has put in code that actually inspects them

what is wrong with having a context?

nikomatsakis (May 31 2018 at 18:58, on Zulip):

in particular polonius integration uses it I think ;)

Santiago Pastorino (May 31 2018 at 18:59, on Zulip):

it uses it to get the location

nikomatsakis (May 31 2018 at 18:59, on Zulip):

(I guess I don't know why it's an important invariant that context doesn't affect behavior)

pnkfelix (May 31 2018 at 18:59, on Zulip):

mm

pnkfelix (May 31 2018 at 18:59, on Zulip):

I guess it doesn't matter

nikomatsakis (May 31 2018 at 19:00, on Zulip):

otoh threading an explicit location also seems ok — or even baking it into the flows

nikomatsakis (May 31 2018 at 19:00, on Zulip):

most of the code kind of "implicitly knows" where it is

nikomatsakis (May 31 2018 at 19:00, on Zulip):

via the flows state

nikomatsakis (May 31 2018 at 19:00, on Zulip):

or at least implicitly knows the state of the various bitsets at the point where it is

pnkfelix (May 31 2018 at 19:00, on Zulip):

just once you start reading structure of context, then you have to actually commit to its form to some degree

pnkfelix (May 31 2018 at 19:01, on Zulip):

but if all you are using it for is as a token with useful fmt::Debug output ...

pnkfelix (May 31 2018 at 19:01, on Zulip):

then that leaves the developer with more freedom for what does or doesn't go in it.

pnkfelix (May 31 2018 at 19:01, on Zulip):

But really, I don't actually have much justification.

pnkfelix (May 31 2018 at 19:02, on Zulip):

Attaching the location to the flows state ... that would indeed be interesting...

Santiago Pastorino (May 31 2018 at 19:04, on Zulip):

I thought more about Place as place in memory
where values are stored

is this correct?

Santiago Pastorino (May 31 2018 at 19:05, on Zulip):

confused because

Santiago Pastorino (May 31 2018 at 19:05, on Zulip):
#[derive(Copy, Clone, Debug)]
pub enum Place {
    /// A place referring to a value allocated in the `Memory` system.
    Ptr {
        /// A place may have an invalid (integral or undef) pointer,
        /// since it might be turned back into a reference
        /// before ever being dereferenced.
        ptr: Scalar,
        align: Align,
        extra: PlaceExtra,
    },

    /// A place referring to a value on the stack. Represented by a stack frame index paired with
    /// a Mir local index.
    Local { frame: usize, local: mir::Local },
}
Santiago Pastorino (May 31 2018 at 19:06, on Zulip):
/// A path to a value; something that can be evaluated without
/// changing or disturbing program state.
#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum Place<'tcx> {
    /// local variable
    Local(Local),

    /// static or static mut variable
    Static(Box<Static<'tcx>>),

    /// projection out of a place (access a field, deref a pointer, etc)
    Projection(Box<PlaceProjection<'tcx>>),
}
Santiago Pastorino (May 31 2018 at 19:06, on Zulip):

I was looking at the second one

Santiago Pastorino (May 31 2018 at 20:05, on Zulip):

@nikomatsakis you around?

nikomatsakis (May 31 2018 at 20:06, on Zulip):

sort of

nikomatsakis (May 31 2018 at 20:06, on Zulip):

in a meeting

nikomatsakis (May 31 2018 at 20:06, on Zulip):

but it hasn't started yet :)

Santiago Pastorino (May 31 2018 at 20:07, on Zulip):

what I 'm understanding from your comment here https://github.com/rust-lang/rust/issues/51031 about report_illegal_reassignment is that I needed to call that, but that thing doesn't report the consider using a mut thing.check_access_permissions is the one that does but unsure how do they play together if shouldn't

Santiago Pastorino (May 31 2018 at 20:10, on Zulip):

I'm adding some code to call it and see what happens, compile times killing me

Santiago Pastorino (May 31 2018 at 20:10, on Zulip):

:(

nikomatsakis (May 31 2018 at 20:11, on Zulip):

mm let me check

Santiago Pastorino (May 31 2018 at 20:11, on Zulip):

I think that makes the thing

Santiago Pastorino (May 31 2018 at 20:11, on Zulip):

but I'm getting cannot mutate suggestion

Santiago Pastorino (May 31 2018 at 20:11, on Zulip):

I'm probably passing the wrong stuff

Santiago Pastorino (May 31 2018 at 20:11, on Zulip):

checking

nikomatsakis (May 31 2018 at 20:12, on Zulip):

I may have been confused

Santiago Pastorino (May 31 2018 at 20:13, on Zulip):

the Place need to be a projection with base Local

Santiago Pastorino (May 31 2018 at 20:13, on Zulip):

for this error to show up

Santiago Pastorino (May 31 2018 at 20:16, on Zulip):

I'm not sure about that case, seems like the fix it's not just calling a function, I can just build that thing up and call it a day

nikomatsakis (May 31 2018 at 20:19, on Zulip):

ok @Santiago Pastorino here now

nikomatsakis (May 31 2018 at 20:20, on Zulip):

btw did you ever try gdb?

nikomatsakis (May 31 2018 at 20:20, on Zulip):

hmm so there is this code already

nikomatsakis (May 31 2018 at 20:21, on Zulip):

though it doesn't appear to be executing here...?

nikomatsakis (May 31 2018 at 20:22, on Zulip):

ah, I see why

nikomatsakis (May 31 2018 at 20:22, on Zulip):

are you there @Santiago Pastorino ?

Santiago Pastorino (May 31 2018 at 20:23, on Zulip):

sorry read that wrong, yes here

nikomatsakis (May 31 2018 at 20:23, on Zulip):

well I was debating whether to leave revised notes here or in the issue

nikomatsakis (May 31 2018 at 20:23, on Zulip):

if you're around we can "chat live"

nikomatsakis (May 31 2018 at 20:24, on Zulip):

so it seems like we have this code already, which tries to tell you "which variable is causing the path not to be mutable"

nikomatsakis (May 31 2018 at 20:24, on Zulip):

that code is sort of like a poor version of the suggestion we are trying to add

Santiago Pastorino (May 31 2018 at 20:24, on Zulip):

we can chat live yes

Santiago Pastorino (May 31 2018 at 20:25, on Zulip):

or I can try to take a look at what you're saying

nikomatsakis (May 31 2018 at 20:25, on Zulip):

so that code compares the borrowed path against the path which must be mut

nikomatsakis (May 31 2018 at 20:25, on Zulip):

and does nothing if they are the same

nikomatsakis (May 31 2018 at 20:25, on Zulip):

I'm not .. 100% sure the point of that, but I think what it should do

nikomatsakis (May 31 2018 at 20:26, on Zulip):

is to check if this is a local, find its declaration, and issue a suggestion

nikomatsakis (May 31 2018 at 20:26, on Zulip):

much like what I wrote already

Santiago Pastorino (May 31 2018 at 20:26, on Zulip):

hold on a bit

nikomatsakis (May 31 2018 at 20:27, on Zulip):

(to be clear, I think we can delete the code that's there now)

nikomatsakis (May 31 2018 at 20:27, on Zulip):

(looking through ui tests, it doesn't seem to add much)

Santiago Pastorino (May 31 2018 at 20:27, on Zulip):

back

nikomatsakis (May 31 2018 at 20:28, on Zulip):

example:

error[E0594]: cannot assign to immutable item `x`
  --> $DIR/issue-45983.rs:17:18
   |
LL |     give_any(|y| x = Some(y));
   |                  ^^^^^^^^^^^ cannot mutate
   |
   = note: the value which is causing this path not to be mutable is...: `x`
Santiago Pastorino (May 31 2018 at 20:28, on Zulip):

let me read what you've said

Santiago Pastorino (May 31 2018 at 20:28, on Zulip):

btw did you ever try gdb?

about gdb couldn't, I mean, I can but it takes ages to compile with debug = true

Santiago Pastorino (May 31 2018 at 20:29, on Zulip):

seriously like 30 mins

nikomatsakis (May 31 2018 at 20:29, on Zulip):

k

Santiago Pastorino (May 31 2018 at 20:30, on Zulip):

ok read the stuff

Santiago Pastorino (May 31 2018 at 20:30, on Zulip):

not sure what you want to do

Santiago Pastorino (May 31 2018 at 20:30, on Zulip):

maybe I didn't understand something

Santiago Pastorino (May 31 2018 at 20:30, on Zulip):

do you want the poor version?

nikomatsakis (May 31 2018 at 20:30, on Zulip):

I do not

nikomatsakis (May 31 2018 at 20:30, on Zulip):

I want to replace it

nikomatsakis (May 31 2018 at 20:31, on Zulip):

with this new suggestion

Santiago Pastorino (May 31 2018 at 20:31, on Zulip):

so you meant that the stuff I need is at that place?

nikomatsakis (May 31 2018 at 20:31, on Zulip):

I believe so, yes

nikomatsakis (May 31 2018 at 20:32, on Zulip):

the place_err variable is I think the "reason" for the error

Santiago Pastorino (May 31 2018 at 20:33, on Zulip):

but this thing is not triggering in my example if that's what you mean

nikomatsakis (May 31 2018 at 20:33, on Zulip):

that's because of the if place != place_err

Santiago Pastorino (May 31 2018 at 20:33, on Zulip):

I mean, I'm not getting this poor version

nikomatsakis (May 31 2018 at 20:33, on Zulip):

we would remove that

nikomatsakis (May 31 2018 at 20:33, on Zulip):

at least, that's my theory

Santiago Pastorino (May 31 2018 at 20:33, on Zulip):

ahh

Santiago Pastorino (May 31 2018 at 20:33, on Zulip):

I see

Santiago Pastorino (May 31 2018 at 20:33, on Zulip):

ok

Santiago Pastorino (May 31 2018 at 20:33, on Zulip):

let me try to figure out with all this info

nikomatsakis (May 31 2018 at 20:35, on Zulip):

ok, I'm dumping a few notes in the issue, but maybe I should stop :)

nikomatsakis (May 31 2018 at 20:36, on Zulip):

I didn't put too many details :) here

Santiago Pastorino (May 31 2018 at 20:37, on Zulip):

yep

Santiago Pastorino (May 31 2018 at 20:37, on Zulip):

I've done all what you're suggesting

Santiago Pastorino (May 31 2018 at 20:37, on Zulip):

but was doing in the function we were talking about at the beginning

Santiago Pastorino (May 31 2018 at 20:38, on Zulip):

I'm trying removing the if to see if this thing shows up

Santiago Pastorino (May 31 2018 at 20:38, on Zulip):

and then will do all that

nikomatsakis (May 31 2018 at 20:38, on Zulip):

one thing that is bothering me

nikomatsakis (May 31 2018 at 20:38, on Zulip):

there are many arms to this match

nikomatsakis (May 31 2018 at 20:38, on Zulip):

eg, the one we have been looking at

nikomatsakis (May 31 2018 at 20:38, on Zulip):

but also this one

nikomatsakis (May 31 2018 at 20:39, on Zulip):

these are all reporting errors

nikomatsakis (May 31 2018 at 20:39, on Zulip):

but for different causes

nikomatsakis (May 31 2018 at 20:39, on Zulip):

e.g., we use different wording for x = 3 vs &mut x

nikomatsakis (May 31 2018 at 20:39, on Zulip):

however

nikomatsakis (May 31 2018 at 20:39, on Zulip):

in all cases we probably want to issue the same kinds of suggestions

nikomatsakis (May 31 2018 at 20:40, on Zulip):

I feel like the setup of this method could be changed

nikomatsakis (May 31 2018 at 20:40, on Zulip):

right now, each arm fully constructs an error and invokes err.emit()

nikomatsakis (May 31 2018 at 20:42, on Zulip):

but I think we would rather have something like:

let mut err = match kind {
  Reservation(...) | Write(...) => {
    if no error { return } else { construct_err_object() }
  }
  Reservation(...) | Write(...) => /* some variant of the preivous arm */,
  Reservation(...) | Write(...) => ...,
};

// If we get here, then there *is* an error
match place_err {
    Place::Local(l) => {
        // suggest changing to `mut` variable
    }
    Place::Projection(...) => {
        // suggest changing to `&mut` borrow (see [link 1])
    }
}

err.emit()

link 1

Santiago Pastorino (May 31 2018 at 20:54, on Zulip):

ok, will check this as soon as I have this working

Santiago Pastorino (May 31 2018 at 20:54, on Zulip):

one question

Santiago Pastorino (May 31 2018 at 20:55, on Zulip):

why did you say source_info gives me the span?

Santiago Pastorino (May 31 2018 at 20:55, on Zulip):

wouldn't it be local.span?

nikomatsakis (May 31 2018 at 20:56, on Zulip):

I don't think there is any such field

nikomatsakis (May 31 2018 at 20:56, on Zulip):

but the SourceInfo struct has one

Santiago Pastorino (May 31 2018 at 20:57, on Zulip):

so you're talking about the local_decl

Santiago Pastorino (May 31 2018 at 20:58, on Zulip):

I was trying to get it from the local

Santiago Pastorino (May 31 2018 at 20:58, on Zulip):

Place::Local(local)

Santiago Pastorino (May 31 2018 at 20:58, on Zulip):

that local inside has a span

Santiago Pastorino (May 31 2018 at 20:58, on Zulip):

now I'm confused about what's each thing

nikomatsakis (May 31 2018 at 20:58, on Zulip):

the Local is just an index

nikomatsakis (May 31 2018 at 20:58, on Zulip):

you can think of it as the name of the local variable

nikomatsakis (May 31 2018 at 20:59, on Zulip):

the LocalDecl tells you information about its declaration

Santiago Pastorino (May 31 2018 at 20:59, on Zulip):

so when I navigate to the Local definition I'm probably seeing something that's unrelated

Santiago Pastorino (May 31 2018 at 20:59, on Zulip):
  # pri kind tag               file
  1 F   s    Local             src/librustc/hir/mod.rs
               pub struct Local {
  2 F   s    Local             src/libsyntax/ast.rs
               pub struct Local {
nikomatsakis (May 31 2018 at 21:00, on Zulip):

those things are unrelated

nikomatsakis (May 31 2018 at 21:00, on Zulip):

that is because Local is an index created by the newtype_index macro

nikomatsakis (May 31 2018 at 21:00, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/mir/struct.Local.html

nikomatsakis (May 31 2018 at 21:01, on Zulip):

https://github.com/rust-lang/rust/issues/50337 :)

Santiago Pastorino (May 31 2018 at 21:03, on Zulip):

yes that's in my todo list

Santiago Pastorino (May 31 2018 at 21:03, on Zulip):

but yeah, I remember it comes from there

Santiago Pastorino (May 31 2018 at 21:49, on Zulip):

@nikomatsakis in a local_decl, I see that name is an Option

Santiago Pastorino (May 31 2018 at 21:50, on Zulip):

but if the declaration is_user_variable it should have a name, right?

Santiago Pastorino (May 31 2018 at 21:50, on Zulip):

so it's safe to unwrap?

nikomatsakis (May 31 2018 at 21:50, on Zulip):

I think that is true :)

Santiago Pastorino (May 31 2018 at 21:50, on Zulip):

when can map_or anyway if you prefer

Santiago Pastorino (May 31 2018 at 21:51, on Zulip):

I think the thing is working

Santiago Pastorino (May 31 2018 at 21:51, on Zulip):

will check, compile times are giving me a hard time

pnkfelix (May 31 2018 at 21:58, on Zulip):

it does seem like a lot of stuff here might just be finding fixes to small bugs in fn check_access_permissions

pnkfelix (May 31 2018 at 21:59, on Zulip):

I've been personally musing about whether it actually makes sense to be doing these checks (which seem very AST oriented to me) on the MIR... is there any chance this would be better off as a pass over the HAIR?

Santiago Pastorino (May 31 2018 at 21:59, on Zulip):
[santiago@archlinux rust1 (suggest-use-mut-in-nll)]$ rustc +stage1 src/test/ui/codemap_tests/huge_multispan_highlight.rs
warning: unused variable: `y`
   --> src/test/ui/codemap_tests/huge_multispan_highlight.rs:100:9
    |
100 |     let y = &mut x; //~ ERROR cannot borrow
    |         ^ help: consider using `_y` instead
    |
    = note: #[warn(unused_variables)] on by default

error[E0596]: cannot borrow immutable local variable `x` as mutable
   --> src/test/ui/codemap_tests/huge_multispan_highlight.rs:100:18
    |
12  |     let x = "foo";
    |         - consider changing this to `mut x`
...
100 |     let y = &mut x; //~ ERROR cannot borrow
    |                  ^ cannot borrow mutably

error: aborting due to previous error

For more information about this error, try `rustc --explain E0596`.
pnkfelix (May 31 2018 at 21:59, on Zulip):

ah excellent

Santiago Pastorino (May 31 2018 at 22:00, on Zulip):

running tests

Santiago Pastorino (May 31 2018 at 22:00, on Zulip):

and will push a PR

Santiago Pastorino (May 31 2018 at 22:05, on Zulip):

I was expecting to see something failing but unsure if there's some of this cases being run in nll mode

Santiago Pastorino (May 31 2018 at 22:05, on Zulip):

@pnkfelix remember me, how was to make src/test/ui/codemap_tests/huge_multispan_highlight.rs run in both modes

pnkfelix (May 31 2018 at 22:06, on Zulip):

everything in ui/ should be getting tested under NLL, when x.py runs the tests that say "ui (nll)" at the beginning

Santiago Pastorino (May 31 2018 at 22:06, on Zulip):

ahh ok ok

Santiago Pastorino (May 31 2018 at 22:06, on Zulip):

is happening now

pnkfelix (May 31 2018 at 22:07, on Zulip):

but you'll only see the "ui (nll)" stuf if you get through all the "ui" ones

Santiago Pastorino (May 31 2018 at 22:07, on Zulip):

thing is still running

Santiago Pastorino (May 31 2018 at 22:07, on Zulip):

yeah, now is running nll

pnkfelix (May 31 2018 at 22:07, on Zulip):

(and some of the "ui/" tests do test nll)

pnkfelix (May 31 2018 at 22:07, on Zulip):

((via #![feature(nll)] or // compile-flags: -Z borrowck=mir))

Santiago Pastorino (May 31 2018 at 22:10, on Zulip):

hmm seems like there's something wrong

Santiago Pastorino (May 31 2018 at 22:10, on Zulip):

anyway, I'm going to open a PR to start discussing there

Santiago Pastorino (May 31 2018 at 22:12, on Zulip):

@pnkfelix @nikomatsakis https://github.com/rust-lang/rust/pull/51260

Santiago Pastorino (May 31 2018 at 22:13, on Zulip):

first ... it's suggesting this https://github.com/rust-lang/rust/pull/51260/files#diff-578a4b3f2943cb22662f35f6a89da7c5R22 two times

Santiago Pastorino (May 31 2018 at 22:14, on Zulip):

this https://github.com/rust-lang/rust/pull/51260/files#diff-d19757776de8707f9d197b79b47bd13cR4 seems to be wrong

Santiago Pastorino (May 31 2018 at 22:14, on Zulip):

same here https://github.com/rust-lang/rust/pull/51260/files#diff-1d29243a06fb339b8c5db5217d279a3aR4

Santiago Pastorino (May 31 2018 at 22:15, on Zulip):

need to check it out, tomorrow will take a look at it

pnkfelix (May 31 2018 at 22:15, on Zulip):

yeah both those things seem wrong

Matthew Jasper (May 31 2018 at 22:16, on Zulip):

@Santiago Pastorino In the first one, the "second error" is in the source?

Santiago Pastorino (Jun 01 2018 at 04:01, on Zulip):

@Santiago Pastorino In the first one, the "second error" is in the source?

@Matthew Jasper you're right so it's correct

Santiago Pastorino (Jun 01 2018 at 04:01, on Zulip):

I need to check all this

Santiago Pastorino (Jun 01 2018 at 04:01, on Zulip):

was in a rush and did everything quickly

Santiago Pastorino (Jun 01 2018 at 04:01, on Zulip):

tomorrow will take a look at it

nikomatsakis (Jun 01 2018 at 08:37, on Zulip):

@Santiago Pastorino that PR includes various changes to the stderr files — are those the complete set of effects? (also, you know about --bless, right?)

nikomatsakis (Jun 01 2018 at 08:37, on Zulip):

it looks like an awesome start!

nikomatsakis (Jun 01 2018 at 08:50, on Zulip):

left some comments anyway

pnkfelix (Jun 01 2018 at 09:38, on Zulip):

hey niko, I've been wondering (and I may have said this on a github issue comment somewhere...): Might it make more sense to do these access checks on something closer to the AST, like the HAIR? Right now the suggestion code is going through machinations to try to reconstruct spans of initializing expressions based on searches over the assignment statements.

pnkfelix (Jun 01 2018 at 09:38, on Zulip):

But access checks do not need regions nor control-flow graph info.

pnkfelix (Jun 01 2018 at 09:38, on Zulip):

They are really shallow inspections of the types, right?

pnkfelix (Jun 01 2018 at 09:39, on Zulip):

(maybe I'm overlooking some that do need that more MIR-oriented info...)

pnkfelix (Jun 01 2018 at 09:39, on Zulip):

@nikomatsakis ^

nikomatsakis (Jun 01 2018 at 09:40, on Zulip):

plausibly

nikomatsakis (Jun 01 2018 at 09:40, on Zulip):

we have no such IR at this point

nikomatsakis (Jun 01 2018 at 09:40, on Zulip):

so we'd have to split up the borrow-ck

nikomatsakis (Jun 01 2018 at 09:40, on Zulip):

and/or maybe do these things at typeck time

nikomatsakis (Jun 01 2018 at 09:40, on Zulip):

(which might be fine)

nikomatsakis (Jun 01 2018 at 09:41, on Zulip):

I'd be ok with borrowck getting to live in a world where all variables can be assumed to be mut

nikomatsakis (Jun 01 2018 at 09:41, on Zulip):

oh, I remember why we don't do that

pnkfelix (Jun 01 2018 at 09:41, on Zulip):

well

nikomatsakis (Jun 01 2018 at 09:41, on Zulip):

it's because of the flow sensitivity for let x

pnkfelix (Jun 01 2018 at 09:41, on Zulip):

we need to ensure let x; is initialized once and only once

pnkfelix (Jun 01 2018 at 09:42, on Zulip):

Right. So that is a control-flow graph thing

nikomatsakis (Jun 01 2018 at 09:42, on Zulip):

I guess "how bad is it" is a question :)

nikomatsakis (Jun 01 2018 at 09:42, on Zulip):

like, finding assignments seems... not so hard

pnkfelix (Jun 01 2018 at 09:42, on Zulip):

Anyway either way the current code forfn check_access_permissions is a real mess

nikomatsakis (Jun 01 2018 at 09:43, on Zulip):

otoh if borrowck could just assume all locals are mutable for purposes of borrowing — even if it is also checked assignments for no doubl assignment — that might be really nice

nikomatsakis (Jun 01 2018 at 09:43, on Zulip):

I hate that "treat-local-as-mutable" flag

pnkfelix (Jun 01 2018 at 09:43, on Zulip):

I'm experimenting with trying to simplify it, in much the way that you proposed in some comment

nikomatsakis (Jun 01 2018 at 09:43, on Zulip):

yeah, it is

pnkfelix (Jun 01 2018 at 09:43, on Zulip):

luckily some of the ugliness in the code is easy to undo

pnkfelix (Jun 01 2018 at 09:43, on Zulip):

e.g. there were some refactorings that were ... poorly chosen.

nikomatsakis (Jun 01 2018 at 09:44, on Zulip):

I think borrowck has accumulated some amount of technical debt

nikomatsakis (Jun 01 2018 at 09:44, on Zulip):

sort of amazing for code so young :)

pnkfelix (Jun 01 2018 at 09:44, on Zulip):

and some pyramid of doom stuff that is also easy to fix

nikomatsakis (Jun 01 2018 at 09:44, on Zulip):

but it needs to be smoothed over...

pnkfelix (Jun 01 2018 at 09:44, on Zulip):

Well we had a lot of enthusiastic volunteers

pnkfelix (Jun 01 2018 at 09:44, on Zulip):

during the 2017 impl period

pnkfelix (Jun 01 2018 at 09:44, on Zulip):

and I think the scattered style in the code reflects that mix of volunteers

nikomatsakis (Jun 01 2018 at 09:44, on Zulip):

right, I imagine that part of it is just a byproduct of many people hacking

pnkfelix (Jun 01 2018 at 09:46, on Zulip):

So here's an example of something that I would like to fix and am not sure how best to do so

pnkfelix (Jun 01 2018 at 09:46, on Zulip):

(this is output from a local branch of the code)

pnkfelix (Jun 01 2018 at 09:46, on Zulip):
6 |     fn f(&self) {
  |          ----- help: consider changing this to be a mutable reference: `&mut Foo<'_>`
pnkfelix (Jun 01 2018 at 09:47, on Zulip):

you can see (if you scroll or if you window is large enough), that my attempt to fix rust-lang/rust#51032 is currently suggesting &mut Foo<'_> as the code to write when its highlighting &self

nikomatsakis (Jun 01 2018 at 09:48, on Zulip):

fwiw, this is also a problem in typeck

pnkfelix (Jun 01 2018 at 09:48, on Zulip):

Does the MIR LocalDecl actually carry the info I would need to distinguish this,

nikomatsakis (Jun 01 2018 at 09:48, on Zulip):

we often exclude self from suggestions because we've not got a nice helper for handling it

pnkfelix (Jun 01 2018 at 09:48, on Zulip):

or do I add such info

pnkfelix (Jun 01 2018 at 09:48, on Zulip):

or do we try to put these checks on the HAIR instead

pnkfelix (Jun 01 2018 at 09:49, on Zulip):

fwiw, this is also a problem in typeck

that is interesting. (I assume here you mean mir-typeck)

nikomatsakis (Jun 01 2018 at 09:49, on Zulip):

no

nikomatsakis (Jun 01 2018 at 09:49, on Zulip):

I mean AST type check

pnkfelix (Jun 01 2018 at 09:49, on Zulip):

Really?

pnkfelix (Jun 01 2018 at 09:49, on Zulip):

So the cases where I see it suggested, its because some coder has put in heroics?

nikomatsakis (Jun 01 2018 at 09:50, on Zulip):

presumably

pnkfelix (Jun 01 2018 at 09:50, on Zulip):

hmm

pnkfelix (Jun 01 2018 at 09:50, on Zulip):

Another option here of course

nikomatsakis (Jun 01 2018 at 09:50, on Zulip):

it seems like we can add some info the MIR here

pnkfelix (Jun 01 2018 at 09:50, on Zulip):

would be for me to actually convert the span to a snippet and then inspect that snippet

pnkfelix (Jun 01 2018 at 09:50, on Zulip):

but I really don't like that

nikomatsakis (Jun 01 2018 at 09:50, on Zulip):

yeah, that seems unfortuante

nikomatsakis (Jun 01 2018 at 09:50, on Zulip):

I guess I think that LocalDecl could well carry more info than just "is-user-variable"

nikomatsakis (Jun 01 2018 at 09:51, on Zulip):

e.g., that might be an enum that tells you if this is self

nikomatsakis (Jun 01 2018 at 09:51, on Zulip):

or some pattern binding

pnkfelix (Jun 01 2018 at 09:51, on Zulip):

would be for me to actually convert the span to a snippet and then inspect that snippet

its especially bad because it leads to things like this: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/mod.rs#L1753

pnkfelix (Jun 01 2018 at 09:51, on Zulip):

Okay I think I'm going to look into extending LocalDecl. That at least sounds like the most direct solution to the family of problems I am encountering here.

nikomatsakis (Jun 01 2018 at 09:52, on Zulip):

that said, I don't disagree that some suggestions are easier to make on the expression tree, that must be true :/

nikomatsakis (Jun 01 2018 at 09:52, on Zulip):

but it would be a significant refactoring to move analysis to the HAIR

nikomatsakis (Jun 01 2018 at 09:52, on Zulip):

I'd be more in favor of moving checks to the initial typeck

nikomatsakis (Jun 01 2018 at 09:52, on Zulip):

though I am also wary

nikomatsakis (Jun 01 2018 at 09:53, on Zulip):

particularly because it's not always obvious

nikomatsakis (Jun 01 2018 at 09:53, on Zulip):

e.g., let x = &mut foo.bar.baz

nikomatsakis (Jun 01 2018 at 09:53, on Zulip):

even if you have let mut foo, if bar implements Deref but not DerefMut, this is an error

nikomatsakis (Jun 01 2018 at 09:53, on Zulip):

indeed dodging all these horrible things is part of the point of MIR :)

nikomatsakis (Jun 01 2018 at 09:54, on Zulip):

I forget just how explicit that stuff is in HAIR

nikomatsakis (Jun 01 2018 at 09:54, on Zulip):

much more so than HIR

nikomatsakis (Jun 01 2018 at 09:54, on Zulip):

I don't know, I think it's an interesting idea we should think about

nikomatsakis (Jun 01 2018 at 09:55, on Zulip):

but also sounds like a big distraction

pnkfelix (Jun 01 2018 at 09:55, on Zulip):

true

pnkfelix (Jun 01 2018 at 09:55, on Zulip):

Probably too much work to try to do in short term

pnkfelix (Jun 01 2018 at 09:55, on Zulip):

better to perhaps release something based entirely on MIR

pnkfelix (Jun 01 2018 at 09:55, on Zulip):

in short term

pnkfelix (Jun 01 2018 at 09:55, on Zulip):

with some missing diagnostics or subpar ones

pnkfelix (Jun 01 2018 at 09:56, on Zulip):

and investigate doing it atop HAIR later

nikomatsakis (Jun 01 2018 at 09:57, on Zulip):

yeah; I'd like to make HAIR more of a "first-class" IR

nikomatsakis (Jun 01 2018 at 09:57, on Zulip):

in particular

nikomatsakis (Jun 01 2018 at 09:57, on Zulip):

I also think we should do the "effect check" on HAIR

nikomatsakis (Jun 01 2018 at 09:57, on Zulip):

the unsafe check, that is

nikomatsakis (Jun 01 2018 at 09:58, on Zulip):

gotta go but my hope is to focus on NLL today

Santiago Pastorino (Jun 01 2018 at 13:53, on Zulip):

I'd be more than happy to help with refactoring if you decide so at some point

Santiago Pastorino (Jun 01 2018 at 13:53, on Zulip):

I think this kind of tasks are a very nice way to learn projects in general and the compiler in this case in particular

nikomatsakis (Jun 01 2018 at 14:54, on Zulip):

the problem is I think not knowing exactly what to do yet

pnkfelix (Jun 01 2018 at 14:54, on Zulip):

I'm hoping to have something to share soon

pnkfelix (Jun 01 2018 at 14:55, on Zulip):

its almost certainly going to stomp all over the PR that you put up, @Santiago Pastorino

pnkfelix (Jun 01 2018 at 14:55, on Zulip):

but its good that you put up that PR

pnkfelix (Jun 01 2018 at 14:55, on Zulip):

because it makes me confident that you can give me some solid feedback on this branch. :)

pnkfelix (Jun 01 2018 at 14:56, on Zulip):

(but my branch is much broader; its trying to resolve a lot of the issues we have with suggestions that arise from fn check_access_permissions)

Santiago Pastorino (Jun 01 2018 at 15:16, on Zulip):

@pnkfelix cool :)

Santiago Pastorino (Jun 01 2018 at 15:16, on Zulip):

still couldn't get into this thing

Santiago Pastorino (Jun 01 2018 at 15:16, on Zulip):

will do in ~30mins

pnkfelix (Jun 01 2018 at 15:35, on Zulip):

Okay so here's the start of my big revision to fn check_access_permissions (which necessitated changes more broadly): https://github.com/rust-lang/rust/pull/51275

pnkfelix (Jun 01 2018 at 15:36, on Zulip):

I have to go home now

pnkfelix (Jun 01 2018 at 15:36, on Zulip):

but you can tell from that how many things I'm looking at touching

pnkfelix (Jun 01 2018 at 15:37, on Zulip):

(also ... in one case the error code emitted by the NLL error changed, which surprised me...)

pnkfelix (Jun 01 2018 at 15:37, on Zulip):

(maybe more than one case, not sure yet...)

pnkfelix (Jun 01 2018 at 15:37, on Zulip):

Hopefully I'll have a chance this weekend to follow up on that PR

pnkfelix (Jun 01 2018 at 15:38, on Zulip):

((oh and in some cases the suggestion to use &mut needs to be silenced. that should be readily doable with the changes I made to mir::LocalDecl in that PR...))

Santiago Pastorino (Jun 01 2018 at 16:20, on Zulip):

@nikomatsakis making the changes now

Santiago Pastorino (Jun 01 2018 at 16:20, on Zulip):

https://github.com/rust-lang/rust/pull/51260#discussion_r192332562

Santiago Pastorino (Jun 01 2018 at 16:20, on Zulip):

link 1 is not a link

nikomatsakis (Jun 01 2018 at 17:12, on Zulip):

what the

nikomatsakis (Jun 01 2018 at 17:13, on Zulip):

should be https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/mod.rs#L1723-L1728

nikomatsakis (Jun 01 2018 at 17:20, on Zulip):

not sure what's up w/ github and links

nikomatsakis (Jun 01 2018 at 17:20, on Zulip):

that said, we should compare against what pnkfelix did

Santiago Pastorino (Jun 01 2018 at 17:20, on Zulip):

I was going to mention a couple of things

Santiago Pastorino (Jun 01 2018 at 17:20, on Zulip):

first

Santiago Pastorino (Jun 01 2018 at 17:21, on Zulip):

https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/mod.rs#L1789-L1798

Santiago Pastorino (Jun 01 2018 at 17:21, on Zulip):

err.emit() not being called there

Santiago Pastorino (Jun 01 2018 at 17:21, on Zulip):

unsure why

Santiago Pastorino (Jun 01 2018 at 17:21, on Zulip):

and if I move emit() outside it will be called when it wasn't in that case

Santiago Pastorino (Jun 01 2018 at 17:21, on Zulip):

second

Santiago Pastorino (Jun 01 2018 at 17:21, on Zulip):

match would need to return a pair

Santiago Pastorino (Jun 01 2018 at 17:22, on Zulip):

(err, place_err)

nikomatsakis (Jun 01 2018 at 17:25, on Zulip):

@Santiago Pastorino I don't think that delay_span_bug needs to be emitted; in cases like that, I would expect to just return early and not fall through to the latter code.

Santiago Pastorino (Jun 01 2018 at 17:26, on Zulip):

ok

nikomatsakis (Jun 01 2018 at 17:26, on Zulip):

match would need to return a pair

sure, ok

nikomatsakis (Jun 01 2018 at 17:26, on Zulip):

seems fine...

Santiago Pastorino (Jun 01 2018 at 17:27, on Zulip):

but that case doesn't need the Local and Projection checks we will be doing outside?

Santiago Pastorino (Jun 01 2018 at 17:27, on Zulip):
            Reservation(WriteKind::Move)
            | Write(WriteKind::Move)
            | Reservation(WriteKind::StorageDeadOrDrop)
            | Reservation(WriteKind::MutableBorrow(BorrowKind::Shared))
            | Write(WriteKind::StorageDeadOrDrop)
            | Write(WriteKind::MutableBorrow(BorrowKind::Shared)) => {
                if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) {
                    self.tcx.sess.delay_span_bug(
                        span,
                        &format!(
                            "Accessing `{:?}` with the kind `{:?}` shouldn't be possible",
                            place, kind
                        ),
                    );
                }

                return;
            }
Santiago Pastorino (Jun 01 2018 at 17:28, on Zulip):

so the match place_err won't execute

nikomatsakis (Jun 01 2018 at 17:31, on Zulip):

well, delay_span_bug basically says: if this happens, abort compilation

nikomatsakis (Jun 01 2018 at 17:31, on Zulip):

in doesn't abort immediately

nikomatsakis (Jun 01 2018 at 17:31, on Zulip):

it aborts only if no type errors occur

nikomatsakis (Jun 01 2018 at 17:31, on Zulip):

it means that this condition should not happen,

nikomatsakis (Jun 01 2018 at 17:31, on Zulip):

but might happen if the user's code doesn't type check

nikomatsakis (Jun 01 2018 at 17:32, on Zulip):

so in general we should not report further errors in that case anyway

Santiago Pastorino (Jun 01 2018 at 17:37, on Zulip):

ok

Santiago Pastorino (Jun 01 2018 at 17:38, on Zulip):

interesting, this is dead code https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/mod.rs#L1733-L1735

nikomatsakis (Jun 01 2018 at 17:40, on Zulip):

yes, yes it is :)

nikomatsakis (Jun 01 2018 at 17:40, on Zulip):

not sure if it should be

nikomatsakis (Jun 01 2018 at 17:41, on Zulip):

I don't really get what that if is trying to do

Santiago Pastorino (Jun 01 2018 at 17:42, on Zulip):

what does blame says?

Santiago Pastorino (Jun 01 2018 at 17:46, on Zulip):

added here https://github.com/rust-lang/rust/commit/0c7fc046d3271b8fe31b3c2d168af4f4ab90bcdc

Santiago Pastorino (Jun 01 2018 at 17:47, on Zulip):

it was before

Santiago Pastorino (Jun 01 2018 at 17:47, on Zulip):

looking

nikomatsakis (Jun 01 2018 at 17:48, on Zulip):

@Santiago Pastorino have you looked at @**pnkfelix**'s PR? It definitely changes all this code

nikomatsakis (Jun 01 2018 at 17:48, on Zulip):

I think it incorporates some of your work

Santiago Pastorino (Jun 01 2018 at 17:48, on Zulip):

https://github.com/rust-lang/rust/commit/7d590b533ea93ef916cc82498506a373db1843b6

Santiago Pastorino (Jun 01 2018 at 17:49, on Zulip):

@nikomatsakis no

Santiago Pastorino (Jun 01 2018 at 17:49, on Zulip):

I was compiling this thing

Santiago Pastorino (Jun 01 2018 at 17:49, on Zulip):

changing stuff

Santiago Pastorino (Jun 01 2018 at 17:49, on Zulip):

so ... should I place my work on top of Felix?

Santiago Pastorino (Jun 01 2018 at 17:49, on Zulip):

or should I land this?

nikomatsakis (Jun 01 2018 at 17:50, on Zulip):

I don't know :)

nikomatsakis (Jun 01 2018 at 17:50, on Zulip):

too bad @pnkfelix is not around

nikomatsakis (Jun 01 2018 at 17:50, on Zulip):

this part I think is from your PR

Santiago Pastorino (Jun 01 2018 at 17:51, on Zulip):

I guess I can fix this stuff and provide the PR so we can merge

Santiago Pastorino (Jun 01 2018 at 17:51, on Zulip):

and then work on top of Felix's or I can redo my stuff

Santiago Pastorino (Jun 01 2018 at 17:51, on Zulip):

whatever :)

Santiago Pastorino (Jun 01 2018 at 17:51, on Zulip):

so ... when you were poiting to this https://github.com/rust-lang/rust/blob/61f35e507a56dcdce88bfce99bb2d1eeacb0e9d1/src/librustc_mir/borrow_check/mod.rs#L1723-L1728

Santiago Pastorino (Jun 01 2018 at 17:51, on Zulip):

were you referring to move that outside of this match?

Santiago Pastorino (Jun 01 2018 at 17:51, on Zulip):

or to reuse the match condition?

Santiago Pastorino (Jun 01 2018 at 17:52, on Zulip):

I was reusing the condition

Santiago Pastorino (Jun 01 2018 at 17:52, on Zulip):

but I may have understood wrong

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

were you referring to move that outside of this match?

I think this is what I meant

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

basically, that code is conflating two things

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

pnkfelix's PR kind of pulls them apart

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

the first is the "main" error msg

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

the second is the suggestions for fixing it

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

we want to use distinct main error messages in different cases

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

but the suggestions apply to all cases

nikomatsakis (Jun 01 2018 at 17:53, on Zulip):

right now, the code is applying those suggestions (e.g., make this variable mut) only in particular cases though

nikomatsakis (Jun 01 2018 at 17:54, on Zulip):

my feeling @Santiago Pastorino is that your current PR is subsumed by what @pnkfelix was doing;

nikomatsakis (Jun 01 2018 at 17:54, on Zulip):

but @pnkfelix's PR still could use some add'l cases

nikomatsakis (Jun 01 2018 at 17:55, on Zulip):

not sure whether it makes sense to wait or what

nikomatsakis (Jun 01 2018 at 17:55, on Zulip):

(e.g., I don't think that their PR handles upvars)

Santiago Pastorino (Jun 01 2018 at 17:55, on Zulip):

unsure

Santiago Pastorino (Jun 01 2018 at 17:55, on Zulip):

I can wait for him to finish the stuff and then try to continue with this

Santiago Pastorino (Jun 01 2018 at 17:55, on Zulip):

I'm fine with both things :)

Santiago Pastorino (Jun 01 2018 at 17:57, on Zulip):

but yeah current code it's confusing

nikomatsakis (Jun 01 2018 at 17:58, on Zulip):

hmm

nikomatsakis (Jun 01 2018 at 17:58, on Zulip):

maybe we should look for a bug in a related area?

nikomatsakis (Jun 01 2018 at 17:58, on Zulip):

that is, now that you're getting familiar with those data structures

nikomatsakis (Jun 01 2018 at 17:58, on Zulip):

prob doesn't make sense for you to extend their PR

nikomatsakis (Jun 01 2018 at 17:59, on Zulip):

especially as @pnkfelix may have uncommited edits or something

Santiago Pastorino (Jun 01 2018 at 17:59, on Zulip):

yep

nikomatsakis (Jun 01 2018 at 17:59, on Zulip):

maybe this one? https://github.com/rust-lang/rust/issues/51217

nikomatsakis (Jun 01 2018 at 17:59, on Zulip):

I don't think that is caused by this same fn

Santiago Pastorino (Jun 01 2018 at 18:00, on Zulip):

when @pnkfelix finish the stuff we can check if he added all what I have, if he didn't I add this and done :)

nikomatsakis (Jun 01 2018 at 18:00, on Zulip):

it comes from this call to report_illegal_reassignment right here

nikomatsakis (Jun 01 2018 at 18:01, on Zulip):

hmm

nikomatsakis (Jun 01 2018 at 18:01, on Zulip):

oh, I see that @Matthew Jasper assigned themselves on that one

nikomatsakis (Jun 01 2018 at 18:02, on Zulip):

maybe this one? https://github.com/rust-lang/rust/issues/51195

nikomatsakis (Jun 01 2018 at 18:03, on Zulip):

I've not looked precisely at the conditions in which that msg is emitted

Santiago Pastorino (Jun 01 2018 at 18:04, on Zulip):

ok

Santiago Pastorino (Jun 01 2018 at 18:04, on Zulip):

won't have that much time today

Santiago Pastorino (Jun 01 2018 at 18:05, on Zulip):

but will have it as the next thing to do

nikomatsakis (Jun 01 2018 at 18:05, on Zulip):

ok; first thing to do would be to look at the old borrow checker

nikomatsakis (Jun 01 2018 at 18:05, on Zulip):

and see where that msg is emitted and under which conditions

nikomatsakis (Jun 01 2018 at 18:06, on Zulip):

then we can think about the equivalent in MIR land

Santiago Pastorino (Jun 01 2018 at 18:29, on Zulip):

:+1:

Santiago Pastorino (Jun 01 2018 at 18:30, on Zulip):

I've force pushed to my branch in case there's something useful there

Santiago Pastorino (Jun 01 2018 at 18:30, on Zulip):

I'm talking about the other PR

Santiago Pastorino (Jun 01 2018 at 18:31, on Zulip):

if @pnkfelix covers everything there great :)

Matthew Jasper (Jun 01 2018 at 18:31, on Zulip):

Re 51217, I thought it would be fixed with some of the MIR generation changes that I made for better move permissions errors, but they ended up having undesirable consequences. I'll try to write up where I got to on that issue this weekend.

Santiago Pastorino (Jun 01 2018 at 18:31, on Zulip):

otherwise I can take another look at the thing

Santiago Pastorino (Jun 02 2018 at 12:54, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/issues/46020 I guess the report about this issue is duplicated with https://github.com/rust-lang/rust/issues/51031, maybe I guess would be nice to relate them?

pnkfelix (Jun 04 2018 at 12:46, on Zulip):

(By the way, I would love for anyone to take a stab at figuring out a way to attack rust-lang/rust#51167 ... I guess I could try to throw on some mentoring instructions, but producing good ones might require just as much work as finding the actual implementation of a solution...)

Santiago Pastorino (Jun 04 2018 at 17:36, on Zulip):

@nikomatsakis @pnkfelix should I still tackle this https://github.com/rust-lang/rust/issues/51195 ?

Santiago Pastorino (Jun 04 2018 at 17:36, on Zulip):

Niko told me about it on friday, couldn't do anything yet

Santiago Pastorino (Jun 04 2018 at 17:37, on Zulip):

and I'm about to start to work on something ...

Santiago Pastorino (Jun 04 2018 at 17:37, on Zulip):

I'll wait for thoughts

nikomatsakis (Jun 04 2018 at 18:48, on Zulip):

hey @Santiago Pastorino

nikomatsakis (Jun 04 2018 at 18:48, on Zulip):

that does sound like it would be useful to investigate

nikomatsakis (Jun 04 2018 at 18:49, on Zulip):

I forget what you were doing before, I know we had some issue?

nikomatsakis (Jun 04 2018 at 18:49, on Zulip):

was it that one?

Santiago Pastorino (Jun 04 2018 at 18:49, on Zulip):

no

Santiago Pastorino (Jun 04 2018 at 18:50, on Zulip):

I was with https://github.com/rust-lang/rust/issues/51031

Santiago Pastorino (Jun 04 2018 at 18:50, on Zulip):

the code @pnkfelix was writing it supposed to deal with that, so we closed the PR

Santiago Pastorino (Jun 04 2018 at 18:50, on Zulip):

btw https://github.com/rust-lang/rust/issues/46020 seems like a dup

nikomatsakis (Jun 04 2018 at 18:58, on Zulip):

true

nikomatsakis (Jun 04 2018 at 18:58, on Zulip):

ok so

nikomatsakis (Jun 04 2018 at 18:58, on Zulip):

regarding https://github.com/rust-lang/rust/issues/51195

nikomatsakis (Jun 04 2018 at 18:59, on Zulip):

I guess the thing to do is to run it and dump the MIR output and also kind of get a backtrace — that is, where is the error coming from within the borrow_check/mod.rs execution

nikomatsakis (Jun 04 2018 at 19:00, on Zulip):

I guess most likely it comes when we are checking the StorageDead, but I'm not entirely sure

Santiago Pastorino (Jun 04 2018 at 19:13, on Zulip):

ok

Santiago Pastorino (Jun 04 2018 at 19:13, on Zulip):

thanks

pnkfelix (Jun 04 2018 at 21:36, on Zulip):

@Santiago Pastorino sorry I missed your earlier question; but yes, I do believe rust-lang/rust#51195 is worth investigating, and I do not think such investigation will overlap with big rewrite of fn check_access_permissions.

Santiago Pastorino (Jun 04 2018 at 21:37, on Zulip):

@pnkfelix cool, will do tomorrow

pnkfelix (Jun 05 2018 at 11:55, on Zulip):

if a given diagnostic under NLL gets to the point where it is providing the same info as AST-borrowck, and the only difference is in how the wording is phrased in the diagnostic...

pnkfelix (Jun 05 2018 at 11:56, on Zulip):

... should we perhaps just change the wording to match AST-borrowck (even if it is "inferior" in some sense), just to ease comparison between the two as we continue on this task?

nikomatsakis (Jun 05 2018 at 12:26, on Zulip):

example?

pnkfelix (Jun 05 2018 at 12:29, on Zulip):

example?

let me see, I think the latest version of my check_access_permissions PR should have at least one test that illustrates this

pnkfelix (Jun 05 2018 at 12:35, on Zulip):

okay so consider: https://github.com/pnkfelix/rust/blob/f0570baddc57f10400cfa017986476e14548dfac/src/test/ui/did_you_mean/issue-38147-1.nll.stderr

pnkfelix (Jun 05 2018 at 12:35, on Zulip):

here's the diff of that nll.stderr file against the AST-borrowck stderr: https://gist.github.com/pnkfelix/c766479dea9706660f8f5dba22dacabd

pnkfelix (Jun 05 2018 at 12:36, on Zulip):

If you ask me, saying "cannot borrow as mutable" for self.s.push('x') is "better" than saying "assignment into an immutable reference"

nikomatsakis (Jun 05 2018 at 12:36, on Zulip):

agreed

pnkfelix (Jun 05 2018 at 12:36, on Zulip):

in terms of conveying what is actually happening in the code and what the compiler is objecting to

nikomatsakis (Jun 05 2018 at 12:36, on Zulip):

I'm not crazy about either of them

pnkfelix (Jun 05 2018 at 12:36, on Zulip):

but in terms of our own process going forward

nikomatsakis (Jun 05 2018 at 12:36, on Zulip):

but it's better

nikomatsakis (Jun 05 2018 at 12:36, on Zulip):

I would sort of like to say "cannot write" :)

nikomatsakis (Jun 05 2018 at 12:36, on Zulip):

but that's .. probably a separate conversation

pnkfelix (Jun 05 2018 at 12:36, on Zulip):

it may be "better" to make it exactly match the AST-borrowck output

pnkfelix (Jun 05 2018 at 12:37, on Zulip):

until we turn NLL on by default

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

I'm torn

pnkfelix (Jun 05 2018 at 12:37, on Zulip):

Having said that

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

on the one hand, i've been wanting to do a "review" of borrowck diagnostics

pnkfelix (Jun 05 2018 at 12:37, on Zulip):

its notso bad looking at the diffs

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

and try to agree on consistent, minimal vocabulary

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

and use it everywhere

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

on the other hand, I see the value in matching

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

that said

pnkfelix (Jun 05 2018 at 12:37, on Zulip):

(especially if you use a tool like ediff that highlights within the line)

pnkfelix (Jun 05 2018 at 12:37, on Zulip):

and plus

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

it is easy enough to modify AST to match in cases where we think it's better now

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

that said

nikomatsakis (Jun 05 2018 at 12:37, on Zulip):

some of these diffs seem better for AST

pnkfelix (Jun 05 2018 at 12:37, on Zulip):

getting exact matching on the spans is likely going to be a real rathole

nikomatsakis (Jun 05 2018 at 12:38, on Zulip):

ah, yes, definitely

pnkfelix (Jun 05 2018 at 12:38, on Zulip):

and without exact matching on spans

nikomatsakis (Jun 05 2018 at 12:38, on Zulip):

I think achieving zero diff doesn't have to be a goal

pnkfelix (Jun 05 2018 at 12:38, on Zulip):

its pointless to sweat about exact matching on wording

nikomatsakis (Jun 05 2018 at 12:38, on Zulip):

cannot borrow data mutably in a & reference

nikomatsakis (Jun 05 2018 at 12:38, on Zulip):

that seems better for the AST

pnkfelix (Jun 05 2018 at 12:38, on Zulip):

I definitely agree that the AST wording is better in some cases

pnkfelix (Jun 05 2018 at 12:38, on Zulip):

hmm

pnkfelix (Jun 05 2018 at 12:39, on Zulip):

cannot borrow data mutably in a & reference

we can put that back. The NLL diagnostics were actually attempting to do this before

pnkfelix (Jun 05 2018 at 12:39, on Zulip):

but they were not doing it well and it was complicating the code

pnkfelix (Jun 05 2018 at 12:39, on Zulip):

so I removed it

nikomatsakis (Jun 05 2018 at 12:39, on Zulip):

well

pnkfelix (Jun 05 2018 at 12:39, on Zulip):

but after the cleanup

nikomatsakis (Jun 05 2018 at 12:39, on Zulip):

I also don't love it :)

pnkfelix (Jun 05 2018 at 12:39, on Zulip):

it should be easier to come up with proper wording, I think

nikomatsakis (Jun 05 2018 at 12:39, on Zulip):

I lean towards this:

pnkfelix (Jun 05 2018 at 12:40, on Zulip):

Oh, since I have your attention

nikomatsakis (Jun 05 2018 at 12:40, on Zulip):

we should not try to "blindly match"

nikomatsakis (Jun 05 2018 at 12:40, on Zulip):

rather, I think we should take some time and try to make "new and improved diagnostics" a feature too

nikomatsakis (Jun 05 2018 at 12:40, on Zulip):

but that's assuming it's not too much overhead

nikomatsakis (Jun 05 2018 at 12:40, on Zulip):

to track the diffs for now

pnkfelix (Jun 05 2018 at 12:40, on Zulip):

What is the "right way" to find out if there was a user-written lifetime for a region on a ty::TyRef within rustc_mir::borrow_check ?

nikomatsakis (Jun 05 2018 at 12:41, on Zulip):

in particular we should mostly (for now) try to get it to the point where it's just wording

pnkfelix (Jun 05 2018 at 12:41, on Zulip):

in particular we should mostly (for now) try to get it to the point where it's just wording

yes this is what I'm basically doing for now

pnkfelix (Jun 05 2018 at 12:41, on Zulip):

because that's the "hard part"

pnkfelix (Jun 05 2018 at 12:41, on Zulip):

we can delegate fine tuning

pnkfelix (Jun 05 2018 at 12:41, on Zulip):

to arbitrary people

nikomatsakis (Jun 05 2018 at 12:45, on Zulip):

right

nikomatsakis (Jun 05 2018 at 12:45, on Zulip):

What is the "right way" to find out if there was a user-written lifetime for a region on a ty::TyRef within rustc_mir::borrow_check ?

I think there is no way to do that?

nikomatsakis (Jun 05 2018 at 12:45, on Zulip):

I mean this is the whole problem around user-written annotations, right?

nikomatsakis (Jun 05 2018 at 12:46, on Zulip):

I was thinking some more about it recently

pnkfelix (Jun 05 2018 at 12:46, on Zulip):

hmm okay

nikomatsakis (Jun 05 2018 at 12:46, on Zulip):

maybe it'd be cool to not embed types directly into MIR

nikomatsakis (Jun 05 2018 at 12:46, on Zulip):

but rather via a kind of indirection

nikomatsakis (Jun 05 2018 at 12:46, on Zulip):

so that we can attach annotations to them

pnkfelix (Jun 05 2018 at 12:46, on Zulip):

I'm already adding code to thread down the span of any explicit type provided for a local in the source

pnkfelix (Jun 05 2018 at 12:46, on Zulip):

so that we can highlight that span instead of the span of the identifier

pnkfelix (Jun 05 2018 at 12:47, on Zulip):

so I could similarly thread down information about the type itself, namely if it had an explicit lifetime parameter

nikomatsakis (Jun 05 2018 at 12:47, on Zulip):

that was actually also a use case I had in mind

nikomatsakis (Jun 05 2018 at 12:47, on Zulip):

I'm not sure how you could do that

nikomatsakis (Jun 05 2018 at 12:47, on Zulip):

the info is not in the MIR now

pnkfelix (Jun 05 2018 at 12:47, on Zulip):

right

pnkfelix (Jun 05 2018 at 12:47, on Zulip):

I'd have to add it

nikomatsakis (Jun 05 2018 at 12:47, on Zulip):

but being able to add spans is exactly why I was thinking that maybe we should have the MIR have TyIndex

nikomatsakis (Jun 05 2018 at 12:48, on Zulip):

alternatively, we could embed more than Ty<'tcx>

pnkfelix (Jun 05 2018 at 12:48, on Zulip):

Where TyIndex would be the indirection you mentioned above?

nikomatsakis (Jun 05 2018 at 12:48, on Zulip):

right, the idea would be that the MIR embeds a "Type index" and then we hvae a separate array of "Types" that maps each of those to various bits of info

nikomatsakis (Jun 05 2018 at 12:48, on Zulip):

e.g., span but maybe also the user-given version

nikomatsakis (Jun 05 2018 at 12:48, on Zulip):

(the canonicalized stuff)

nikomatsakis (Jun 05 2018 at 12:49, on Zulip):

I guess in some cases it could be a sparse mapping

pnkfelix (Jun 05 2018 at 12:51, on Zulip):

Can I safely assume that we need not block the diagnostics work on such a change?

nikomatsakis (Jun 05 2018 at 12:51, on Zulip):

I wouldn't block on it, but it maybe something we want to think about

pnkfelix (Jun 05 2018 at 12:51, on Zulip):

e.g. I should feel free in the short term to just make do adding Option<TySourceStuff> to LocalDecl

pnkfelix (Jun 05 2018 at 12:51, on Zulip):

yes I agree we should think about it.

nikomatsakis (Jun 05 2018 at 12:52, on Zulip):

that seems ok

pnkfelix (Jun 05 2018 at 12:53, on Zulip):

also

pnkfelix (Jun 05 2018 at 12:53, on Zulip):

since I still have your attention (maybe)

pnkfelix (Jun 05 2018 at 12:53, on Zulip):

If I wanted to add the NodeId to this TySourceStuff

pnkfelix (Jun 05 2018 at 12:53, on Zulip):

doing so blindly seems to break DepInfo

pnkfelix (Jun 05 2018 at 12:54, on Zulip):

is that something where I need to not use the impl_stable_hash! macro?

pnkfelix (Jun 05 2018 at 12:54, on Zulip):

and instead write the impl explicitly and make it not include the NodeId in the hash it computes?

nikomatsakis (Jun 05 2018 at 12:54, on Zulip):

you should not add the node-id

nikomatsakis (Jun 05 2018 at 12:54, on Zulip):

under any circumstances :)

pnkfelix (Jun 05 2018 at 12:54, on Zulip):

That is the other answer I was expecting. :)

pnkfelix (Jun 05 2018 at 12:54, on Zulip):

okay

pnkfelix (Jun 05 2018 at 12:54, on Zulip):

I don't mind that

nikomatsakis (Jun 05 2018 at 12:54, on Zulip):

you could maybe add the HirId

nikomatsakis (Jun 05 2018 at 12:55, on Zulip):

though .. I'd sort of prefer to avoid it ...

pnkfelix (Jun 05 2018 at 12:55, on Zulip):

It just means I cannot e.g. put in Option<ast::Ty> as the Option<TySourceStuff>

nikomatsakis (Jun 05 2018 at 12:55, on Zulip):

mostly because I'd rather MIR is a standalone IR

nikomatsakis (Jun 05 2018 at 12:55, on Zulip):

oh dear god no

nikomatsakis (Jun 05 2018 at 12:55, on Zulip):

but I guess you meant hir::?

pnkfelix (Jun 05 2018 at 12:55, on Zulip):

I dont think I mean hir::

nikomatsakis (Jun 05 2018 at 12:55, on Zulip):

well I think we've thrown away the AST by this time

pnkfelix (Jun 05 2018 at 12:55, on Zulip):

unless that does carry the user-written name for the liftime?

nikomatsakis (Jun 05 2018 at 12:55, on Zulip):

so it would be hard to use that

pnkfelix (Jun 05 2018 at 12:55, on Zulip):

let me go check

nikomatsakis (Jun 05 2018 at 12:55, on Zulip):

in any case we should throw it away

nikomatsakis (Jun 05 2018 at 12:56, on Zulip):

HIR does carry the info but I think it's the wrong thing to use

nikomatsakis (Jun 05 2018 at 12:56, on Zulip):

we already have the info you want

nikomatsakis (Jun 05 2018 at 12:56, on Zulip):

in the UserAssertTy declarations

pnkfelix (Jun 05 2018 at 12:56, on Zulip):

oh it does have a name

nikomatsakis (Jun 05 2018 at 12:56, on Zulip):

maybe you should move those into LocalDecl

nikomatsakis (Jun 05 2018 at 12:56, on Zulip):

or, I don't know if it's the wrong thing I guess

nikomatsakis (Jun 05 2018 at 12:56, on Zulip):

but it's not obviously the right thing

pnkfelix (Jun 05 2018 at 12:56, on Zulip):

so use a Option<CanonicalTy> in the LocalDecl, let me see...

nikomatsakis (Jun 05 2018 at 12:56, on Zulip):

I guess it depends on what we want to underline though

nikomatsakis (Jun 05 2018 at 12:57, on Zulip):

in particular, the HIR gives much finer-grained span info

nikomatsakis (Jun 05 2018 at 12:57, on Zulip):

so maybe you do want that

nikomatsakis (Jun 05 2018 at 12:57, on Zulip):

but interpreting it can be sort of non-trivial

nikomatsakis (Jun 05 2018 at 12:57, on Zulip):

what msg are you trying to emulate exactly?

pnkfelix (Jun 05 2018 at 12:58, on Zulip):

right now we always suggest types like &mut Foo

pnkfelix (Jun 05 2018 at 12:58, on Zulip):

when the user had written &Foo or &'a Foo

pnkfelix (Jun 05 2018 at 12:58, on Zulip):

so I'm trying to get it to suggest &'a mut Foo

nikomatsakis (Jun 05 2018 at 12:58, on Zulip):

ah

nikomatsakis (Jun 05 2018 at 12:58, on Zulip):

how does AST borrowck do that?

pnkfelix (Jun 05 2018 at 12:58, on Zulip):

let me go see

pnkfelix (Jun 05 2018 at 13:01, on Zulip):

hmm

pnkfelix (Jun 05 2018 at 13:01, on Zulip):

self.tcx.sess.codemap().span_to_snippet(lifetime.span)

pnkfelix (Jun 05 2018 at 13:02, on Zulip):

specifically in: https://github.com/rust-lang/rust/blob/f9157f5b869fdb14308eaf6778d01ee3d0e1268a/src/librustc_borrowck/borrowck/mod.rs#L1131

pnkfelix (Jun 05 2018 at 13:02, on Zulip):

(and its getting the lifetime off of the hir::TyRptr)

nikomatsakis (Jun 05 2018 at 13:03, on Zulip):

ok

nikomatsakis (Jun 05 2018 at 13:03, on Zulip):

so I think HirId is ok

nikomatsakis (Jun 05 2018 at 13:03, on Zulip):

that should be hashable too

nikomatsakis (Jun 05 2018 at 13:03, on Zulip):

in general NodeId is deprecated

nikomatsakis (Jun 05 2018 at 13:04, on Zulip):

though it's been one of those long, slow deprecations

pnkfelix (Jun 05 2018 at 13:06, on Zulip):

okay. I'll try passing down a HirId instead of a Span then

pnkfelix (Jun 05 2018 at 13:06, on Zulip):

and see whether I can reconstruct everything I need from that.

nikomatsakis (Jun 05 2018 at 13:07, on Zulip):

@pnkfelix you can convert one to a NodeId, still sadly needed much of the time

pnkfelix (Jun 05 2018 at 13:07, on Zulip):

okay

pnkfelix (Jun 05 2018 at 13:08, on Zulip):

sorry for my ignorance here

pnkfelix (Jun 05 2018 at 13:08, on Zulip):

but

pnkfelix (Jun 05 2018 at 13:09, on Zulip):

but I was adding ty_span: Span field to hir::Arg

pnkfelix (Jun 05 2018 at 13:09, on Zulip):

as part of the threading

pnkfelix (Jun 05 2018 at 13:09, on Zulip):

down of information

pnkfelix (Jun 05 2018 at 13:09, on Zulip):

hir::Arg already carries a hir_id: HirId, presumably the id of the Arg itself in the map

pnkfelix (Jun 05 2018 at 13:10, on Zulip):

is there a way to map that arg's hir_id to the HirId of the type, if any, attached to that arg?

pnkfelix (Jun 05 2018 at 13:11, on Zulip):

Or in other words: Would an attempt in the PR to add ty_id: HirId to Arg be instantly be "r-'ed" with a note saying "don't do this; use this map over here instead."

nikomatsakis (Jun 05 2018 at 13:11, on Zulip):

hmm.

nikomatsakis (Jun 05 2018 at 13:12, on Zulip):

I guess they are found in the FnDecl https://doc.rust-lang.org/nightly/nightly-rustc/rustc/hir/struct.FnDecl.html

nikomatsakis (Jun 05 2018 at 13:12, on Zulip):

I think that's because of incremental etc

nikomatsakis (Jun 05 2018 at 13:12, on Zulip):

i.e., we wanted to separate out the "fn-local part of it"

nikomatsakis (Jun 05 2018 at 13:12, on Zulip):

from the "interface part"

pnkfelix (Jun 05 2018 at 13:13, on Zulip):

hmm did I overlook trying to use the hir::Ty's there ..

nikomatsakis (Jun 05 2018 at 13:13, on Zulip):

I would imagine you could have the DefId for the corresponding fn on hand, and could fetch the FnDecl — you'd have to know the index of the argument

nikomatsakis (Jun 05 2018 at 13:13, on Zulip):

in any case, hir::Ty of course also has a HirId

pnkfelix (Jun 05 2018 at 13:14, on Zulip):

right, I just wanted to get my hands on the hir::Ty itself or a HirId for the hir::Ty

pnkfelix (Jun 05 2018 at 13:14, on Zulip):

in the context of some code that is enumerating over a series of hir::Arg

nikomatsakis (Jun 05 2018 at 13:14, on Zulip):

yeah it seems like that is not convenient

pnkfelix (Jun 05 2018 at 13:14, on Zulip):

but I need to double check

nikomatsakis (Jun 05 2018 at 13:15, on Zulip):

my preference would be to have that code enumerate over a series of (hir::Arg, hir::Ty) pairs

pnkfelix (Jun 05 2018 at 13:15, on Zulip):

I may have simply overlooked the fact that the hir::Ty was also availalbe to cosnult

nikomatsakis (Jun 05 2018 at 13:15, on Zulip):

which would require fetching the FnDecl etc

nikomatsakis (Jun 05 2018 at 13:15, on Zulip):

I suspect it has enough context

nikomatsakis (Jun 05 2018 at 13:15, on Zulip):

and/or already has its hands on the FnDecl

pnkfelix (Jun 05 2018 at 13:16, on Zulip):

this is context: https://github.com/rust-lang/rust/blob/30cae5870907e7ae9e74a39eee5bcf55ee5d2809/src/librustc_mir/build/mod.rs#L89

pnkfelix (Jun 05 2018 at 13:16, on Zulip):

I know from prior work in past few days

nikomatsakis (Jun 05 2018 at 13:17, on Zulip):

yeah so it has everything it needs

pnkfelix (Jun 05 2018 at 13:17, on Zulip):

that I should be able to extract the FnDecl

pnkfelix (Jun 05 2018 at 13:17, on Zulip):

it doesn't already have the FnDecl; the fn_sig there is different

nikomatsakis (Jun 05 2018 at 13:17, on Zulip):

though it might be a bit of a pain

nikomatsakis (Jun 05 2018 at 13:17, on Zulip):

well, it has everything "in principle" :)

pnkfelix (Jun 05 2018 at 13:17, on Zulip):

Yeah. I had to add some code to lookup an FnDecl from a body's owner

nikomatsakis (Jun 05 2018 at 13:18, on Zulip):

I wonder if e.g. closures will cause you pain here

nikomatsakis (Jun 05 2018 at 13:18, on Zulip):

it seems like it would be ok

nikomatsakis (Jun 05 2018 at 13:18, on Zulip):

to add to hir::Arg the HirId of the type

nikomatsakis (Jun 05 2018 at 13:18, on Zulip):

thinking on it a bit more

pnkfelix (Jun 05 2018 at 13:18, on Zulip):

(or an Option<FnDecl>, I guess. Can't remember. )

nikomatsakis (Jun 05 2018 at 13:18, on Zulip):

what you really don't want is info flow the other way

pnkfelix (Jun 05 2018 at 13:18, on Zulip):

what do you mean by info flow the other way?

nikomatsakis (Jun 05 2018 at 13:18, on Zulip):

but if the type changes, code using the Arg is gonna have to be recompiled

nikomatsakis (Jun 05 2018 at 13:18, on Zulip):

I mean that if the hir::Ty embedded patterns and stuff

pnkfelix (Jun 05 2018 at 13:19, on Zulip):

ah

nikomatsakis (Jun 05 2018 at 13:19, on Zulip):

er, not the hir::Ty but the hir::FnDecl

pnkfelix (Jun 05 2018 at 13:19, on Zulip):

oka yes

pnkfelix (Jun 05 2018 at 13:19, on Zulip):

things that are details of the implementation of the fn body

nikomatsakis (Jun 05 2018 at 13:19, on Zulip):

i am also influenced by all the various forms functions and methods can take in the HIR

pnkfelix (Jun 05 2018 at 13:19, on Zulip):

should not leak out into the hir::FnDecl

nikomatsakis (Jun 05 2018 at 13:19, on Zulip):

they probably all do have a FnDecl hiding somewhere

nikomatsakis (Jun 05 2018 at 13:19, on Zulip):

but at minimum you'd have to do a bunch of mapping to get it, right?

nikomatsakis (Jun 05 2018 at 13:19, on Zulip):

or else write a helper in the hir map

pnkfelix (Jun 05 2018 at 13:21, on Zulip):

or else write a helper in the hir map

yeah that's what I did

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

k, that also seems fine

pnkfelix (Jun 05 2018 at 13:21, on Zulip):

https://github.com/rust-lang/rust/pull/51275/files#diff-2611048bb760f028c808ed4a14b07c00R173

nikomatsakis (Jun 05 2018 at 13:22, on Zulip):

it's times like these I wish we had made progress on "virtual structs"

nikomatsakis (Jun 05 2018 at 13:22, on Zulip):

such a pain

nikomatsakis (Jun 06 2018 at 18:48, on Zulip):

@Santiago Pastorino oh, and... which diagnostic issue were you working on again?

Santiago Pastorino (Jun 06 2018 at 18:57, on Zulip):

I had this https://github.com/rust-lang/rust/issues/51195 one pending

Santiago Pastorino (Jun 06 2018 at 18:58, on Zulip):

didn't do any work yet

nikomatsakis (Jun 06 2018 at 19:52, on Zulip):

ok, let me know if you want any tips, but I basically think my plan there would be to examine when each msg is printed. My theory is that they are printed in basic block order, which is semi-arbitrary.

nikomatsakis (Jun 06 2018 at 19:52, on Zulip):

and that we should just iterate over the basic blocks in a distinct order (I'd guess RPO)

pnkfelix (Jun 06 2018 at 20:40, on Zulip):

@nikomatsakis interesting. So the same change might fix both rust-lang/rust#51195 and rust-lang/rust#51167 ?

nikomatsakis (Jun 06 2018 at 20:40, on Zulip):

oh shoot

nikomatsakis (Jun 06 2018 at 20:40, on Zulip):

no I think I was mixing them up maybe

pnkfelix (Jun 06 2018 at 20:53, on Zulip):

(I definitely think we should prioritize the RPO experiment.)

pnkfelix (Jun 06 2018 at 22:20, on Zulip):

Hmm. Been playing around with fixing rust-lang/rust#51191 but I'm starting to wonder if this is a diagnostic that may do more harm than good?

pnkfelix (Jun 06 2018 at 22:21, on Zulip):

ah well I guess we can revisit the "should we actually do this?" later

Santiago Pastorino (Jun 07 2018 at 16:56, on Zulip):

ok, let me know if you want any tips, but I basically think my plan there would be to examine when each msg is printed. My theory is that they are printed in basic block order, which is semi-arbitrary.

I'm not sure I understood what you meant here

Santiago Pastorino (Jun 07 2018 at 16:57, on Zulip):

@nikomatsakis isn't the issue talking about a note that is not printed?, why is that related to the order of things that are printed?

Santiago Pastorino (Jun 07 2018 at 16:58, on Zulip):

/cc @pnkfelix

nikomatsakis (Jun 08 2018 at 01:07, on Zulip):

I think @Santiago Pastorino I may have been mixing up two issues

Santiago Pastorino (Jun 08 2018 at 01:49, on Zulip):

:thumbs_up:

Santiago Pastorino (Jun 08 2018 at 19:48, on Zulip):

@nikomatsakis what's the connection between src/librustc_mir/borrow_check/error_reporting.rs and src/librustc_mir/util/borrowck_errors.rs ?

nikomatsakis (Jun 08 2018 at 19:50, on Zulip):

got me :)

nikomatsakis (Jun 08 2018 at 19:51, on Zulip):

it looks like there is some setup to let HIR and MIR borrowck share code

Santiago Pastorino (Jun 08 2018 at 19:51, on Zulip):

but there's reporting in both

Santiago Pastorino (Jun 08 2018 at 19:51, on Zulip):

so ...

Santiago Pastorino (Jun 08 2018 at 19:52, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/util/borrowck_errors.rs#L427

Santiago Pastorino (Jun 08 2018 at 19:53, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/error_reporting.rs#L493

Santiago Pastorino (Jun 08 2018 at 19:53, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/error_reporting.rs#L544

Santiago Pastorino (Jun 08 2018 at 19:54, on Zulip):

last two ok

Santiago Pastorino (Jun 08 2018 at 19:54, on Zulip):

the one in borrowck_errors.rs unsure what's about

nikomatsakis (Jun 08 2018 at 19:55, on Zulip):

well, I see this:

> /home/nmatsakis/.cargo/bin/rg --no-heading --color never 'path_does_not_live_long_enough'
src/librustc_borrowck/borrowck/mod.rs:913:                let mut db = self.path_does_not_live_long_enough(error_span, &msg, Origin::Ast);
src/librustc_mir/borrow_check/error_reporting.rs:492:            tcx.path_does_not_live_long_enough(borrow_span, &format!("`{}`", name), Origin::Mir);
src/librustc_mir/borrow_check/error_reporting.rs:513:            tcx.path_does_not_live_long_enough(proper_span, "borrowed value", Origin::Mir);
src/librustc_mir/borrow_check/error_reporting.rs:543:            tcx.path_does_not_live_long_enough(borrow_span, &format!("`{}`", name), Origin::Mir);
src/librustc_mir/borrow_check/error_reporting.rs:569:            tcx.path_does_not_live_long_enough(proper_span, "borrowed value", Origin::Mir);
src/librustc_mir/util/borrowck_errors.rs:421:    fn path_does_not_live_long_enough(self,
Santiago Pastorino (Jun 08 2018 at 19:56, on Zulip):

ahh ok

Santiago Pastorino (Jun 08 2018 at 19:56, on Zulip):

that makes sense

nikomatsakis (Jun 08 2018 at 19:56, on Zulip):

oh, I see, rustc_borrowck depends on rustc_mir

nikomatsakis (Jun 08 2018 at 19:56, on Zulip):

/me did not expect that

nikomatsakis (Jun 08 2018 at 19:56, on Zulip):

I suspect in part it's a bit chaotic

nikomatsakis (Jun 08 2018 at 19:56, on Zulip):

and not well organized

Santiago Pastorino (Jun 08 2018 at 19:57, on Zulip):

so ... if this is for code reuse I got it

nikomatsakis (Jun 08 2018 at 19:57, on Zulip):

but basically borrowck_errors.rs is a way to share code yeah

Santiago Pastorino (Jun 08 2018 at 19:57, on Zulip):

but seems like src/librustc_mir/borrow_check/error_reporting.rs is not reusing then?

nikomatsakis (Jun 08 2018 at 19:57, on Zulip):

but I wouldn't be surprised if we also wound up with some duplication

Santiago Pastorino (Jun 08 2018 at 20:59, on Zulip):
[santiago@archlinux rust1 (diagnostic-suggest-drop-in-reverse)]$ rustc +stage1 -Zborrowck=mir src/test/ui/error-codes/E0597.rs
error[E0597]: `y` does not live long enough
  --> src/test/ui/error-codes/E0597.rs:18:16
   |
18 |     x.x = Some(&y);
   |                ^^ borrowed value does not live long enough
19 |     //~^ `y` does not live long enough [E0597]
20 | }
   | -
   | |
   | borrowed value only lives until here
   | borrow later used here, when `x` is dropped
   |
   = note: values in a scope are dropped in the opposite order they are defined 3
   = note: values in a scope are dropped in the opposite order they are defined 2

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.
Santiago Pastorino (Jun 08 2018 at 21:00, on Zulip):

@nikomatsakis :point_up:

Santiago Pastorino (Jun 08 2018 at 21:00, on Zulip):

you can see what are 3 and 2 here ...

Santiago Pastorino (Jun 08 2018 at 21:00, on Zulip):
[santiago@archlinux rust1 (diagnostic-suggest-drop-in-reverse)]$ git diff
diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs
index 9061af1b68..e8e6eadddb 100644
--- a/src/librustc_mir/borrow_check/error_reporting.rs
+++ b/src/librustc_mir/borrow_check/error_reporting.rs
@@ -496,6 +496,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             format!("`{}` dropped here while still borrowed", name),
         );
         self.explain_why_borrow_contains_point(context, borrow, &mut err);
+        err.note("values in a scope are dropped in the opposite order \
+                  they are defined 1");
         err.emit();
     }

@@ -545,6 +547,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         err.span_label(drop_span, "borrowed value only lives until here");

         self.explain_why_borrow_contains_point(context, borrow, &mut err);
+
+        err.note("values in a scope are dropped in the opposite order \
+                  they are defined 2");
         err.emit();
     }

diff --git a/src/librustc_mir/util/borrowck_errors.rs b/src/librustc_mir/util/borrowck_errors.rs
index d6b3e674f8..85726648b0 100644
--- a/src/librustc_mir/util/borrowck_errors.rs
+++ b/src/librustc_mir/util/borrowck_errors.rs
@@ -424,9 +424,12 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
                                       o: Origin)
                                       -> DiagnosticBuilder<'cx>
     {
-        let err = struct_span_err!(self, span, E0597, "{} does not live long enough{OGN}",
+        let mut err = struct_span_err!(self, span, E0597, "{} does not live long enough{OGN}",
                                    path, OGN=o);

+        err.note("values in a scope are dropped in the opposite order \
+                  they are defined 3");
+
         self.cancel_if_wrong_origin(err, o)
     }

 ```
nikomatsakis (Jun 08 2018 at 21:01, on Zulip):

I'm confused what I am looking at

nikomatsakis (Jun 08 2018 at 21:01, on Zulip):

ah ok

Santiago Pastorino (Jun 08 2018 at 21:01, on Zulip):

both execute

nikomatsakis (Jun 08 2018 at 21:01, on Zulip):

uh.. oookay :)

nikomatsakis (Jun 08 2018 at 21:02, on Zulip):

well probably the latter one (util) creates the err

Santiago Pastorino (Jun 08 2018 at 21:02, on Zulip):

hehe

nikomatsakis (Jun 08 2018 at 21:02, on Zulip):

and then returns it to the other one?

Santiago Pastorino (Jun 08 2018 at 21:02, on Zulip):

unsure ... checking

Santiago Pastorino (Jun 08 2018 at 22:10, on Zulip):

@nikomatsakis about this issue

Santiago Pastorino (Jun 08 2018 at 22:10, on Zulip):

https://github.com/rust-lang/rust/blob/2e104a77cfb6b83f725512210c88dc970426a354/src/test/ui/error-codes/E0597.rs

Santiago Pastorino (Jun 08 2018 at 22:11, on Zulip):

this happens when you assign to a field of a local a borrow of another local

Santiago Pastorino (Jun 08 2018 at 22:11, on Zulip):

so ...

Santiago Pastorino (Jun 08 2018 at 22:12, on Zulip):

https://github.com/rust-lang/rust/blob/2e104a77cfb6b83f725512210c88dc970426a354/src/librustc_mir/borrow_check/error_reporting.rs#L519

Santiago Pastorino (Jun 08 2018 at 22:12, on Zulip):

adding a call to note here with the text ends up firing way more than I want

Santiago Pastorino (Jun 08 2018 at 22:15, on Zulip):

like https://github.com/rust-lang/rust/blob/2e104a77cfb6b83f725512210c88dc970426a354/src/test/ui/issue-46471.rs and a lot more

Santiago Pastorino (Jun 08 2018 at 22:16, on Zulip):

I need to check if this is when assigning to a field of a local a borrow of another local

Santiago Pastorino (Jun 08 2018 at 22:16, on Zulip):

maybe that's something I can get from BorrowData?

Santiago Pastorino (Jun 08 2018 at 22:28, on Zulip):

will try to continue tomorrow or on monday

Santiago Pastorino (Jun 08 2018 at 23:36, on Zulip):

maybe checking that borrow.borrowed_place and borrow.assigned_place are Place::Local ?

nikomatsakis (Jun 09 2018 at 00:40, on Zulip):

I need to check if this is when assigning to a field of a local a borrow of another local

Yeah I was afraid of that

nikomatsakis (Jun 09 2018 at 00:40, on Zulip):

let me take a look at the original

nikomatsakis (Jun 09 2018 at 00:40, on Zulip):

this could be a bit trickier in MIR than in AST land

nikomatsakis (Jun 09 2018 at 00:40, on Zulip):

I think though

nikomatsakis (Jun 09 2018 at 00:40, on Zulip):

we might want to approach this a slightly different way, also

nikomatsakis (Jun 09 2018 at 00:41, on Zulip):

like maybe we don't want to issue precisely the same message, but we can get a similar point across

nikomatsakis (Jun 09 2018 at 00:41, on Zulip):

@Santiago Pastorino what is the actual example again? (Which is the issue # again?)

nikomatsakis (Jun 09 2018 at 00:42, on Zulip):

ok, I see it's https://github.com/rust-lang/rust/issues/51195...

nikomatsakis (Jun 09 2018 at 00:42, on Zulip):

I'm gonna open a new topic instead of this general purpose one =)

Santiago Pastorino (Jun 20 2018 at 17:23, on Zulip):

https://github.com/rust-lang/rust/pull/51651 & https://github.com/rust-lang/rust/pull/51638 were merged

Santiago Pastorino (Jun 20 2018 at 17:23, on Zulip):

@pnkfelix @nikomatsakis I guess you should update the doc

Santiago Pastorino (Jun 20 2018 at 17:23, on Zulip):

and btw ... it surprised me how fast were merged, what's going on with the queue and the system in general?

Santiago Pastorino (Jun 20 2018 at 17:23, on Zulip):

is it faster for some reason?

lqd (Jun 20 2018 at 17:31, on Zulip):

I think the queue is empty now yeah

Santiago Pastorino (Jun 20 2018 at 17:32, on Zulip):

wonder what's going on :P

nikomatsakis (Jun 20 2018 at 19:13, on Zulip):

@Santiago Pastorino you looking for something else to do? =)

Santiago Pastorino (Jun 20 2018 at 19:26, on Zulip):

@nikomatsakis you gave me this one https://github.com/rust-lang/rust/issues/51512

Santiago Pastorino (Jun 20 2018 at 19:26, on Zulip):

but haven't started yet

Santiago Pastorino (Jun 20 2018 at 19:26, on Zulip):

do you want me to do something different?

nikomatsakis (Jun 20 2018 at 19:26, on Zulip):

nope

nikomatsakis (Jun 20 2018 at 19:27, on Zulip):

I just assigned you

Santiago Pastorino (Jun 20 2018 at 19:29, on Zulip):

ok :)

Keith Yeung (Jun 20 2018 at 20:08, on Zulip):

i've been trying hard to continue working on https://github.com/rust-lang/rust/issues/51027, but i fear that there's a lot of things that i need to read up before i can be productive about it

nikomatsakis (Jun 20 2018 at 20:14, on Zulip):

might be good to look at lower hanging fruit to start

nikomatsakis (Jun 20 2018 at 20:15, on Zulip):

one simple thing to investigate might be https://github.com/rust-lang/rust/issues/51167

nikomatsakis (Jun 20 2018 at 20:15, on Zulip):

which doesn't super important but might simplify our lives

nikomatsakis (Jun 20 2018 at 20:15, on Zulip):

the theory was that we could try just visiting the MIR in "reverse post order" when we issue errors

Santiago Pastorino (Jun 20 2018 at 21:58, on Zulip):

why do we have some error messages duplicated?

Santiago Pastorino (Jun 20 2018 at 21:58, on Zulip):

I see that a lot along some of the things I've been fixing

Santiago Pastorino (Jun 20 2018 at 21:58, on Zulip):

like ...

Santiago Pastorino (Jun 20 2018 at 21:58, on Zulip):
[santiago@archlinux rust1 (master)]$ rg "move occurs because" src/lib*
src/librustc_borrowck/borrowck/mod.rs
745:                "move occurs because {} has type `{}`, which does not implement the `Copy` trait",

src/librustc_mir/borrow_check/error_reporting.rs
125:                        "move occurs because {} has type `{}`, \
Santiago Pastorino (Jun 20 2018 at 21:58, on Zulip):

why is there the same error message twice over those two files?

nikomatsakis (Jun 21 2018 at 09:13, on Zulip):

one is the old borrow checker, the other is the new borrow checker

Santiago Pastorino (Jun 21 2018 at 12:10, on Zulip):

ahh ya, facepalm

Santiago Pastorino (Jun 21 2018 at 12:10, on Zulip):

I was confused because I've seen error messages more than twice

nikomatsakis (Jun 21 2018 at 14:12, on Zulip):

@Santiago Pastorino were you seeing ICE failures in E0657.rs and impl_trait_projection.rs?

Santiago Pastorino (Jun 21 2018 at 14:13, on Zulip):

yes

Santiago Pastorino (Jun 21 2018 at 14:13, on Zulip):

I think were those

nikomatsakis (Jun 21 2018 at 14:19, on Zulip):

I'm seeing them too. Even on master. Very odd.

nikomatsakis (Jun 21 2018 at 14:19, on Zulip):

I have no idea why bors passes

Santiago Pastorino (Jun 21 2018 at 15:05, on Zulip):

@nikomatsakis https://gist.github.com/spastorino/d0a75bfac0d2c76c9d5b70753cc1911f

nikomatsakis (Jun 21 2018 at 15:27, on Zulip):

@Santiago Pastorino yep I get the same thing

nikomatsakis (Jun 21 2018 at 15:27, on Zulip):

not with nightly it seems

nikomatsakis (Jun 21 2018 at 15:27, on Zulip):

but with local builds of master

nikomatsakis (Jun 21 2018 at 15:27, on Zulip):

I am investigating

nikomatsakis (Jun 21 2018 at 15:28, on Zulip):

actually wait

nikomatsakis (Jun 21 2018 at 15:28, on Zulip):

I do get it with nightly

pnkfelix (Jul 25 2018 at 12:49, on Zulip):

Okay, here is a perhaps silly question. When you see diagnostic output like this: borrowck-move-out-of-vec-tail.nll.stderr ...

pnkfelix (Jul 25 2018 at 12:49, on Zulip):

Which I'll paste inline for ease of reference:

pnkfelix (Jul 25 2018 at 12:49, on Zulip):
help: to prevent move, use ref or ref mut
   |
LL |                 &[Foo { string: ref a },
   |                                 ^^^^^
help: to prevent move, use ref or ref mut
   |
LL |                   Foo { string: ref b }] => {
   |                                 ^^^^^
pnkfelix (Jul 25 2018 at 12:50, on Zulip):

Do you read those lines that are highlighted as saying "here I have literally transcribed your original source code that you need to edit in some manner"

pnkfelix (Jul 25 2018 at 12:51, on Zulip):

or do you read them as saying "I am highlighting code that does not actually exist in your files. It rather is the hypothetical code that I am suggesting you put in there instead."

pnkfelix (Jul 25 2018 at 12:51, on Zulip):

Because as I understand it, our current diagnostics (at least from these NLL cases) are meant to be interpreted as the latter.

pnkfelix (Jul 25 2018 at 12:52, on Zulip):

... I don't know if I've seen us do highlighting like this in other cases outside of NLL. Is there precedent?

pnkfelix (Jul 25 2018 at 12:52, on Zulip):

(if there is precedent then I'll just accept this as current practice in our diagnostic system.)

nikomatsakis (Jul 25 2018 at 13:16, on Zulip):

it depends but I also find that kind of confusing

nikomatsakis (Jul 25 2018 at 13:16, on Zulip):

I feel like wording like "to prevent move, use ref or ref mut, as shown here:" would help

Last update: Nov 21 2019 at 23:25UTC