Stream: t-compiler/wg-nll

Topic: issue-52533-region-err-fully-elaborated-types


davidtwco (Jul 23 2018 at 09:34, on Zulip):

@nikomatsakis figured I'd spin off a new topic for #52533 since the other PR got merged yesterday.

davidtwco (Jul 23 2018 at 11:02, on Zulip):

@nikomatsakis Currently have it outputting the following, will open a WIP PR once I've run tests and see what else it affects:

warning: not reporting region error due to nll
  --> src/test/ui/issue-52533.rs:15:16
   |
15 |     foo(|a, b| b)
   |                ^

warning: unused variable: `a`
  --> src/test/ui/issue-52533.rs:15:10
   |
15 |     foo(|a, b| b)
   |          ^ help: consider using `_a` instead
   |
   = note: #[warn(unused_variables)] on by default

error: unsatisfied lifetime constraints
  --> src/test/ui/issue-52533.rs:15:16
   |
15 |     foo(|a, b| b)
   |           -  - ^ closure was supposed to return data with lifetime `'1` but it is returning data with lifetime `'2`
   |           |  |
   |           |  has type `&'1 u32`
   |           has type `&'2 u32`

error: aborting due to previous error

I have no idea why the span is off-by-one.

davidtwco (Jul 23 2018 at 11:43, on Zulip):

Fixed the span issue but found that this solution fails on some more complex cases, will keep at it.

nikomatsakis (Jul 23 2018 at 13:54, on Zulip):

looks nice

davidtwco (Jul 23 2018 at 13:54, on Zulip):

The approach I was taking probably won't work.

davidtwco (Jul 23 2018 at 13:54, on Zulip):

How did you intend for those elaborated types to be constructed?

davidtwco (Jul 23 2018 at 13:57, on Zulip):

The approach I used above called extract_type_name and then looked through the string for & and added our synthesized region names. That doesn't work when extract_type_name has '_#2r types of regions in it. I could work around that with some string manipulation but that seems like a poor solution.

Haven't worked out how to do it properly yet.

nikomatsakis (Jul 23 2018 at 13:59, on Zulip):

I wasn't sure what I wanted to do

nikomatsakis (Jul 23 2018 at 13:59, on Zulip):

one thing I was considering

nikomatsakis (Jul 23 2018 at 13:59, on Zulip):

is extracting the ppaux code

nikomatsakis (Jul 23 2018 at 13:59, on Zulip):

which is currently a total mess

nikomatsakis (Jul 23 2018 at 13:59, on Zulip):

and, in this version, allow it to be characterized by some helper fns

nikomatsakis (Jul 23 2018 at 13:59, on Zulip):

for example "how do I print this region"

davidtwco (Jul 23 2018 at 13:59, on Zulip):

I had been looking at that, wasn't sure if that was worth doing.

nikomatsakis (Jul 23 2018 at 14:00, on Zulip):

one problem with my idea is that it will be very tempting to try and cleanup the code

nikomatsakis (Jul 23 2018 at 14:00, on Zulip):

and that is a rat hole that never seems to work ;)

nikomatsakis (Jul 23 2018 at 14:00, on Zulip):

otoh hacking in "yet another random helper thing" might .. be one too many

nikomatsakis (Jul 23 2018 at 14:00, on Zulip):

but basically there are two approaches I guess:

nikomatsakis (Jul 23 2018 at 14:01, on Zulip):

we can generate a type that includes regions

nikomatsakis (Jul 23 2018 at 14:01, on Zulip):

(I'd probably put '1 and '2 and '_ for evrything else)

nikomatsakis (Jul 23 2018 at 14:01, on Zulip):

or we can customize the pretty-printing code

nikomatsakis (Jul 23 2018 at 14:01, on Zulip):

I'm torn on it, but the disadvantage I see of synthesizing an actual type

nikomatsakis (Jul 23 2018 at 14:01, on Zulip):

is that it "pollutes" our type system representation

nikomatsakis (Jul 23 2018 at 14:01, on Zulip):

it maybe that all we need is a "region callback"?

davidtwco (Jul 23 2018 at 14:02, on Zulip):

Some way of providing the functions that it uses to generate a new region name for use.

nikomatsakis (Jul 23 2018 at 14:02, on Zulip):

(for the pp code, we traditioanlly use thread-local data to configure it)

davidtwco (Jul 23 2018 at 14:09, on Zulip):

So, I have a slightly yucky, albeit not that long, version that will manipulate the string with &'_#2r type references and replace them with our own.

davidtwco (Jul 23 2018 at 14:39, on Zulip):

Submitted #52648 with what I've got.

davidtwco (Jul 24 2018 at 14:07, on Zulip):

@nikomatsakis I can fix that incorrect closure/function labelling in this PR if you'd like (since it is yet to be reviewed and may need work if we don't like the current approach). I'm not sure how to detect whether or not a external region comes from a function or a closure arguments though?

nikomatsakis (Jul 24 2018 at 14:09, on Zulip):

say a bit more? why does it matter if it comes from the arguments per se?

davidtwco (Jul 24 2018 at 14:12, on Zulip):

It doesn't need to be arguments, I guess. But the case we're seeing in dyn-trait.rs mentions closures incorrectly. That branch is triggered when the outlived_fr is external (from what I remember) - so it seems like outlived_fr can be external in the case of a captured upvar (what that branch was implemented for) or in the case of a argument from a function (since it is also triggering for dyn-trait.rs).

nikomatsakis (Jul 24 2018 at 14:14, on Zulip):

I think that to decide whether to talk about "closure" vs "fn" we have to check the mir_def_id

nikomatsakis (Jul 24 2018 at 14:14, on Zulip):

i.e., the same scenario can occur in either case

nikomatsakis (Jul 24 2018 at 14:14, on Zulip):

in one case, it cannot escape the "closure body"

nikomatsakis (Jul 24 2018 at 14:14, on Zulip):

in one case, it cannot escape the "fn body"

davidtwco (Jul 24 2018 at 14:14, on Zulip):

Right, I wasn't sure how I'd find out which of "closure" or "fn" to refer to it as.

nikomatsakis (Jul 24 2018 at 14:15, on Zulip):

there is a fn called tcx.is_closure that takes a def-id

davidtwco (Jul 24 2018 at 14:15, on Zulip):

I already renamed that function from report_closure_error to report_escaping_data in the PR.

davidtwco (Jul 24 2018 at 14:15, on Zulip):

Sounds good, I'll quickly make that change.

davidtwco (Jul 24 2018 at 14:23, on Zulip):

Fixed, PR now has:

warning: not reporting region error due to nll
  --> $DIR/dyn-trait.rs:32:16
   |
LL |     static_val(x); //~ ERROR cannot infer
   |                ^

error: borrowed data escapes outside of function
  --> $DIR/dyn-trait.rs:32:5
   |
LL | fn with_dyn_debug_static<'a>(x: Box<dyn Debug + 'a>) {
   |                              - `x` is a reference that is only valid in the function body
LL |     static_val(x); //~ ERROR cannot infer
   |     ^^^^^^^^^^^^^ `x` escapes the function body here

error: aborting due to previous error
nikomatsakis (Jul 24 2018 at 14:24, on Zulip):

nice

davidtwco (Jul 24 2018 at 20:46, on Zulip):

@nikomatsakis can you clarify 'grody'?

nikomatsakis (Jul 24 2018 at 20:46, on Zulip):

"hacky" I guess

nikomatsakis (Jul 24 2018 at 20:46, on Zulip):

I'm still torn though on the best way

nikomatsakis (Jul 24 2018 at 20:46, on Zulip):

tbh I didn't fully understand what you wrote

nikomatsakis (Jul 24 2018 at 20:46, on Zulip):

I guess it relied on to_string inserting '_#3r regions with the actual region numbers?

nikomatsakis (Jul 24 2018 at 20:46, on Zulip):

or did I misunderstand what I was reading

nikomatsakis (Jul 24 2018 at 20:47, on Zulip):

I think I still favor some kind of hacked up variant on pretty-printing that knows about this desire to control how regions are printed out

davidtwco (Jul 24 2018 at 20:49, on Zulip):

No, you're right, it looks for the '_# and removes that, leaving the & that it looks for.

davidtwco (Jul 24 2018 at 20:49, on Zulip):

Just wasn't familiar with the word.

nikomatsakis (Jul 24 2018 at 20:50, on Zulip):

another option we could do is to add a new kind of region variant

nikomatsakis (Jul 24 2018 at 20:50, on Zulip):

but that seems like it will have pretty broad impact

nikomatsakis (Jul 24 2018 at 20:50, on Zulip):

hmm an interesting thing is that all the regions in this case are going to be ReVar

nikomatsakis (Jul 24 2018 at 20:50, on Zulip):

well I guess that doesn't help so much

nikomatsakis (Jul 24 2018 at 20:51, on Zulip):

but I was imaginging you could have some kind of callback for how to print ReVar

davidtwco (Jul 24 2018 at 20:51, on Zulip):

It seems like some enhancements to the pretty printer seem to be the preferred approach.

nikomatsakis (Jul 24 2018 at 20:51, on Zulip):

but I guess that's no different than having a callback for all regions

nikomatsakis (Jul 24 2018 at 20:51, on Zulip):

I think that is my preferred approach so far, what do you think?

nikomatsakis (Jul 24 2018 at 20:52, on Zulip):

somehow controlling the string as it is created seems nicer than trying to match on it afterwards

davidtwco (Jul 24 2018 at 20:52, on Zulip):

Yeah, that seems worth a shot.

nikomatsakis (Jul 25 2018 at 10:47, on Zulip):

@David Wood so I'm looking at the categorization code

nikomatsakis (Jul 25 2018 at 10:47, on Zulip):

I've changed in my PR some more logic to use it

nikomatsakis (Jul 25 2018 at 10:47, on Zulip):

it is producing this diff, which seems suboptimal

nikomatsakis (Jul 25 2018 at 10:47, on Zulip):
---- [ui (nll)] ui/lifetime-errors/ex2c-push-inference-variable.rs stdout ----
diff of stderr:

5          |             ^^^
6
7       error[E0623]: lifetime mismatch
-         --> $DIR/ex2c-push-inference-variable.rs:17:5
+         --> $DIR/ex2c-push-inference-variable.rs:16:13
9          |
10      LL | fn foo<'a, 'b, 'c>(x: &'a mut Vec<Ref<'b, i32>>, y: Ref<'c, i32>) {
11         |                                   ------------      ------------ these two types are declared with different lifetimes...

12      LL |     let z = Ref { data: y.data };
-       LL |     x.push(z); //~ ERROR lifetime mismatch
-          |     ^^^^^^^^^ ...but data from `y` flows into `x` here
+          |             ^^^^^^^^^^^^^^^^^^^^ ...but data from `y` flows into `x` here
15
16      error: aborting due to previous error
davidtwco (Jul 25 2018 at 10:48, on Zulip):

Interesting.

nikomatsakis (Jul 25 2018 at 10:48, on Zulip):

I think this is because we have an assignment (from the aggregate)

nikomatsakis (Jul 25 2018 at 10:48, on Zulip):

and that is ranked higher than the call etc

davidtwco (Jul 25 2018 at 10:48, on Zulip):

I guess that's just adding a more specific category or changing the order?

nikomatsakis (Jul 25 2018 at 10:49, on Zulip):

I'm trying to think: I think that in general assignments from aggregates are unlikely to be good candidates

nikomatsakis (Jul 25 2018 at 10:49, on Zulip):

since they tend to "propagate" regions rather than constrain them

nikomatsakis (Jul 25 2018 at 10:49, on Zulip):

(there are exceptions of course)

nikomatsakis (Jul 25 2018 at 10:49, on Zulip):

I guess I could add a new category to detect this sort of case

davidtwco (Jul 25 2018 at 10:49, on Zulip):

I believe I added that because it was enough to make one of the previous tests in a PR work.

nikomatsakis (Jul 25 2018 at 10:50, on Zulip):

right

nikomatsakis (Jul 25 2018 at 10:50, on Zulip):

I think that was needed because the closure escape messages

nikomatsakis (Jul 25 2018 at 10:50, on Zulip):

were not triggering because they are tied to the category

nikomatsakis (Jul 25 2018 at 10:50, on Zulip):

I do wonder about that though

nikomatsakis (Jul 25 2018 at 10:50, on Zulip):

e.g., does the category really matter?

nikomatsakis (Jul 25 2018 at 10:50, on Zulip):

I guess it depends on the phrasing of the message

davidtwco (Jul 25 2018 at 10:50, on Zulip):

I guess that's the pain point with categories. We can re-order them, but that could have lots of consequences to tests, or add more specific ones, but then it just becomes a mechanism for special casing things.

nikomatsakis (Jul 25 2018 at 10:51, on Zulip):

it seems to me that this is somewhat inevitable

davidtwco (Jul 25 2018 at 10:52, on Zulip):

I've been messing with the pretty printing, it's something.

nikomatsakis (Jul 25 2018 at 10:52, on Zulip):

I mean ultimately we're dealing in heuristics, so yeah... it's gonna be fine-tunning and relying on having a decent set of tests

nikomatsakis (Jul 25 2018 at 10:52, on Zulip):

which we... may or may not have :)

nikomatsakis (Jul 25 2018 at 11:09, on Zulip):

@David Wood ok I'm studying this a bit more, I may have an idea for an alternative to sorting the constraints :)

davidtwco (Jul 25 2018 at 11:09, on Zulip):

Oh?

nikomatsakis (Jul 25 2018 at 11:18, on Zulip):

grr, idk, it's tricky. It feels like what we want to do somehow is to classify not so much as "boring or interesting" but to try and classify the role that each constriant plays along the path. That is, some constraints serve to "forward" one side or the other along. But somewhere in the middle comes at least one constraint that serves to put the one side in tension with the other. How to distinguish that, I'm still not entirely sure. One thing I was looking at is whether it might be interesting to look at locations with multiple constraints associated with them. e.g., looking at the example I gave above, the path is

DEBUG 2018-07-25T11:12:01Z: rustc_mir::borrow_check::nll::region_infer::error_reporting: best_blame_constraint: path=[
    "ConstraintIndex(27): (\'_#3r: \'_#14r) due to All",
    "ConstraintIndex(1): (\'_#14r: \'_#7r) due to Boring(bb0[5])",
    "ConstraintIndex(9): (\'_#7r: \'_#6r) due to Boring(bb0[5])",
    "ConstraintIndex(8): (\'_#6r: \'_#18r) due to Boring(bb0[5])",
    "ConstraintIndex(11): (\'_#18r: \'_#8r) due to Boring(bb0[6])",
    "ConstraintIndex(10): (\'_#8r: \'_#17r) due to Interesting(bb0[6])",
    "ConstraintIndex(16): (\'_#17r: \'_#21r) due to Boring(bb0[11])",
    "ConstraintIndex(21): (\'_#21r: \'_#10r) due to Interesting(bb0[12])",
    "ConstraintIndex(19): (\'_#10r: \'_#20r) due to Interesting(bb0[12])",
    "ConstraintIndex(13): (\'_#20r: \'_#16r) due to Boring(bb0[9])",
    "ConstraintIndex(5): (\'_#16r: \'_#13r) due to Interesting(bb0[2])",
    "ConstraintIndex(24): (\'_#13r: \'_#2r) due to All"
]

where bb0[12] is the thing I want us to select.

nikomatsakis (Jul 25 2018 at 11:18, on Zulip):

but of course other things appear more than once (eg., bb0[5], albeit in a "boring" capacity)

nikomatsakis (Jul 25 2018 at 11:19, on Zulip):

in this particular case, the region #20 acts as this sort of 'intermediary'

nikomatsakis (Jul 25 2018 at 11:20, on Zulip):

it is equated with #10 and #13

nikomatsakis (Jul 25 2018 at 11:21, on Zulip):

I'm trying to think how we can distinguish these "carriers" actions from the others...

nikomatsakis (Jul 25 2018 at 11:21, on Zulip):

idk.

davidtwco (Jul 25 2018 at 11:22, on Zulip):

I'm not sure. I kind of follow what you're suggesting.

nikomatsakis (Jul 25 2018 at 11:24, on Zulip):

what I was getting at with the 'multiple constraints' at one location is this

nikomatsakis (Jul 25 2018 at 11:25, on Zulip):

if you look at the MIR, you have things like

_1 = _4

or

_1 = &mut _4

which basically "propagate" regions from _4 into _1

nikomatsakis (Jul 25 2018 at 11:25, on Zulip):

the call in constrast is

_0 = call Vec::push(_1, _2)

which, in this case, takes the vector _1 type and relates it to the element type _2

nikomatsakis (Jul 25 2018 at 11:25, on Zulip):

so another way to look at is "where did the regions come from"

nikomatsakis (Jul 25 2018 at 11:26, on Zulip):

ah, hmm, interesting. So for example in this case we have an edge from _10 to _20 -- where _10 appears in one of the call arguments

nikomatsakis (Jul 25 2018 at 11:26, on Zulip):

and another edge from _20 to _16, which appears in another argument

nikomatsakis (Jul 25 2018 at 11:26, on Zulip):

that seems like the interesting bit

nikomatsakis (Jul 25 2018 at 11:26, on Zulip):

the _20 itself appears in the type arguments of Vec::push

nikomatsakis (Jul 25 2018 at 11:27, on Zulip):

e.g., the fully elaborated thing is like Vec::push::<&'_20r u32>(_1, _2)

nikomatsakis (Jul 25 2018 at 11:27, on Zulip):

so it feels like if we can look for locations where there is a path from one operand to another

nikomatsakis (Jul 25 2018 at 11:27, on Zulip):

that is an interesting spot

nikomatsakis (Jul 25 2018 at 11:36, on Zulip):

I'll give it a try

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

ok @David Wood I went a different direction but it works well for my test case

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

now I have to see how much it messes up other stuff :P

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

here is the code I added, which explains the reasoning:

        // To find the best span to cite, we first try to look for the
        // final constraint that is interesting and where the `sup` is
        // not unified with the ultimate target region. The reason
        // for this is that we have a chain of constraints that lead
        // from the source to the target region, something like:
        //
        //    '0: '1 ('0 is the source)
        //    '1: '2
        //    '2: '3
        //    '3: '4
        //    '4: '5
        //    '5: '6 ('6 is the target)
        //
        // Some of those regions are unified with `'6` (in the same
        // SCC).  We want to screen those out. After that point, the
        // "closest" constraint we have to the end is going to be the
        // most likely to be the point where the value escapes -- but
        // we still want to screen for an "interesting" point to
        // highlight (e.g., a call site or something).
        let target_scc = self.constraint_sccs.scc(target_region);
        let best_choice = (0..path.len()).rev().find(|&i| {
            let constraint = &self.constraints[path[i]];

            let constraint_sup_scc = self.constraint_sccs.scc(constraint.sup);
            if constraint_sup_scc == target_scc {
                return false;
            }

            match categorized_path[i].0 {
                ConstraintCategory::Boring => false,
                _ => true,
            }
        });
nikomatsakis (Jul 25 2018 at 13:32, on Zulip):

so far it seems strictly better

nikomatsakis (Jul 25 2018 at 13:33, on Zulip):

e.g.,

 error: unsatisfied lifetime constraints
-  --> $DIR/propagate-approximated-fail-no-postdom.rs:55:21
+  --> $DIR/propagate-approximated-fail-no-postdom.rs:57:13
    |
 LL |         |_outlives1, _outlives2, _outlives3, x, y| {
    |          ----------              ---------- lifetime `'2` appears in this argument
    |          |
    |          lifetime `'1` appears in this argument
-LL |             // Only works if 'x: 'y:
-LL |             let p = x.get(); //~ ERROR
-   |                     ^^^^^^^ argument requires that `'1` must outlive `'2`
+...
+LL |             demand_y(x, y, p) //~ ERROR
+   |             ^^^^^^^^^^^^^^^^^ argument requires that `'1` must outlive `'2`
nikomatsakis (Jul 25 2018 at 13:33, on Zulip):

definitely seems better to highlight the call to demand_y and not the call to x.get()

nikomatsakis (Jul 25 2018 at 13:34, on Zulip):

another example

 error[E0621]: explicit lifetime required in the type of `y`
-  --> $DIR/issue-40288-2.rs:16:31
+  --> $DIR/issue-40288-2.rs:17:9
    |
 LL | fn lifetime_transmute_slice<'a, T: ?Sized>(x: &'a T, y: &T) -> &'a T {
    |                                                      - consider changing the type of `y` to `&'a T`
 ...
-LL |         let slice: &mut [_] = &mut out;
-   |                               ^^^^^^^^ lifetime `'a` required
+LL |         slice[0] = y;
+   |         ^^^^^^^^^^^^ lifetime `'a` required
nikomatsakis (Jul 25 2018 at 13:35, on Zulip):

heehee, this is an extreme one

 error[E0623]: lifetime mismatch
-  --> $DIR/ex3-both-anon-regions-3.rs:11:33
+  --> $DIR/ex3-both-anon-regions-3.rs:12:5
    |
 LL | fn foo(z: &mut Vec<(&u8,&u8)>, (x, y): (&u8, &u8)) {
-   |                     ---         ^       --- these two types are declared with different lifetimes...
-   |                                 |
-   |                                 ...but data flows into `z` here
+   |                     ---                 --- these two types are declared with different lifetimes...
+LL |     z.push((x,y)); //~ ERROR lifetime mismatch
+   |     ^^^^^^^^^^^^^ ...but data flows into `z` here
nikomatsakis (Jul 25 2018 at 13:35, on Zulip):

ok, enough, this seems good.

nikomatsakis (Jul 25 2018 at 13:35, on Zulip):

also, that last message would benefit from the "give names to things" treatment I think

nikomatsakis (Jul 25 2018 at 13:35, on Zulip):

but that's the old NiceRegionError code

nikomatsakis (Jul 25 2018 at 13:35, on Zulip):

ought to be fwd-ported

nikomatsakis (Jul 25 2018 at 13:35, on Zulip):

or maybe the new errors are even getting just better

davidtwco (Jul 25 2018 at 13:38, on Zulip):

I assume then by "give names to things" you don't mean the "has type" label from this PR?

nikomatsakis (Jul 25 2018 at 13:41, on Zulip):

no I mean the existing logic, in this case

nikomatsakis (Jul 25 2018 at 13:41, on Zulip):

e.g.,

nikomatsakis (Jul 25 2018 at 13:41, on Zulip):
 error[E0623]: lifetime mismatch
+  --> $DIR/ex3-both-anon-regions-3.rs:12:5
    |
 LL | fn foo(z: &mut Vec<(&u8,&u8)>, (x, y): (&u8, &u8)) {
+   |                     ---                 --- let's call the lifetimes of these references `'1` and `'2`, respectively
+LL |     z.push((x,y)); //~ ERROR lifetime mismatch
+   |     ^^^^^^^^^^^^^ data with lifetime `'2` flows into `'1` here
nikomatsakis (Jul 25 2018 at 13:42, on Zulip):

something like that, idk

nikomatsakis (Jul 25 2018 at 13:42, on Zulip):

in any case "data flows into" is not very good

nikomatsakis (Jul 25 2018 at 13:42, on Zulip):

also, I do think we want to kind of consolidate our terminology. e.g., this talks about "data flowing"

nikomatsakis (Jul 25 2018 at 13:42, on Zulip):

but I think other errors have other terms

nikomatsakis (Jul 25 2018 at 13:42, on Zulip):

we're too diverse

nikomatsakis (Jul 25 2018 at 13:43, on Zulip):

anyway if you're curious here is the commit, including the affected tests

davidtwco (Jul 25 2018 at 13:44, on Zulip):

That's good.

nikomatsakis (Jul 25 2018 at 13:44, on Zulip):

this is an interesting one: https://github.com/rust-lang/rust/pull/52488/commits/0d918a8575cfc7d7ec97b86e87c11e023ac3a37c#diff-6ae66f5f7de7aa4de7eb84da3a8cad33

nikomatsakis (Jul 25 2018 at 13:44, on Zulip):

your new "data escapes" error triggers now

nikomatsakis (Jul 25 2018 at 13:44, on Zulip):

whereas it didn't before

nikomatsakis (Jul 25 2018 at 13:44, on Zulip):

oh hmm https://github.com/rust-lang/rust/pull/52488/commits/0d918a8575cfc7d7ec97b86e87c11e023ac3a37c#diff-718caa2ac9a63a2e0197ac83499c18e0 is the one sort of regression I guess

nikomatsakis (Jul 25 2018 at 13:45, on Zulip):

the problem seems to be the span of the return as much as anything

nikomatsakis (Jul 25 2018 at 13:45, on Zulip):

but I guess more to the point the "return" is considered a kind of interesting statement but really the cast would be more interesting

nikomatsakis (Jul 25 2018 at 13:46, on Zulip):

i.e., the thing that produced the value being returned

davidtwco (Jul 25 2018 at 13:46, on Zulip):

I'm struggling with this pretty printer work. I've got a way to generalize it such that I'll eventually be able to create a RegionInferencePrinterContext that overrides the correct functions. But trying to generalize it piece by piece is a struggle.

nikomatsakis (Jul 25 2018 at 13:47, on Zulip):

:/

nikomatsakis (Jul 25 2018 at 13:47, on Zulip):

it could be the wrong path

nikomatsakis (Jul 25 2018 at 13:47, on Zulip):

although I suspect it's "just" that the code is a terrible mess

nikomatsakis (Jul 25 2018 at 13:47, on Zulip):

i.e., if the pretty printer were nicer, it would work

nikomatsakis (Jul 25 2018 at 13:47, on Zulip):

but of course we have to work with the code we have :)

nikomatsakis (Jul 25 2018 at 13:48, on Zulip):

how exactly are you trying to introduce this context?

davidtwco (Jul 25 2018 at 13:54, on Zulip):

I've made a trait that is a PrintContext, then going to make a DebugPrintContext, DisplayPrintContext to handle the current cases. The existing print_debug function would just create a DebugPrintContext and then do the printing with that.

Printing would be handled by implementing PrintWithContext<DebugPrintContext> (with the appropriate context) - so you'd have a implementation of print_with_context(..) for every context. That lets us introduce new contexts that handle how individual types are printed. I was then going to use specialization to fallback to a Print trait that prints without the need for a context.

I think it would work relatively alright, such that we could create a RegionInferPrintContext that overrides just one of the functions that the trait requires which will control the region naming. For RegionInferPrintContext specifically, the default impl would fallback to DisplayPrintContext rather than Print.

I don't know if this is a decent approach or not.

davidtwco (Jul 25 2018 at 13:55, on Zulip):

Since it's all set up in repos, I'm just trying to change the underlying traits it impls without needing to change the actual printing code.

davidtwco (Jul 25 2018 at 13:55, on Zulip):

Which is mostly working, but I'm left with a couple unsatisfied trait bounds that I haven't quite worked out.

davidtwco (Jul 25 2018 at 13:56, on Zulip):

(at this point I've not split the existing context into two, because I wanted to just get it using the PrintWithContext types first with one giant debug + display context that was there before).

davidtwco (Jul 25 2018 at 13:57, on Zulip):

(after changing to two contexts, I will need to modify the print code slightly but that shouldn't be too bad).

nikomatsakis (Jul 25 2018 at 14:10, on Zulip):

sounds sort of neat

nikomatsakis (Jul 25 2018 at 14:10, on Zulip):

my usual complaint about that code has been that I want {:?} and -Zverbose to be equivalent

nikomatsakis (Jul 25 2018 at 14:10, on Zulip):

which is totally unrelated ;)

nikomatsakis (Jul 25 2018 at 14:11, on Zulip):

if you want some help with the bounds feel free to push commit etc

nikomatsakis (Jul 25 2018 at 14:11, on Zulip):

meanwhile i'm debating what to do about dyn-trait-underscore. I wonder if we should ignore Return -- but that seems problematic

nikomatsakis (Jul 25 2018 at 14:11, on Zulip):

the problem in this case is that there are two casts, one after the other esssentially

nikomatsakis (Jul 25 2018 at 14:12, on Zulip):

anyway I guess we can land as is and improve

davidtwco (Jul 25 2018 at 14:14, on Zulip):

That's probably a decent idea.

nikomatsakis (Jul 25 2018 at 14:15, on Zulip):

I was also considering ignoring things with multi-line spans

nikomatsakis (Jul 25 2018 at 14:15, on Zulip):

total hack though :)

davidtwco (Jul 25 2018 at 14:17, on Zulip):

I think I've gotten past that bounds issue.

davidtwco (Jul 25 2018 at 14:17, on Zulip):

One step closer.

davidtwco (Jul 25 2018 at 14:18, on Zulip):

It's way too warm here.

nikomatsakis (Jul 25 2018 at 14:22, on Zulip):

heh, we're sitting by the harbor now, nice breeze...

nikomatsakis (Jul 25 2018 at 14:22, on Zulip):

just come to an Island :P

davidtwco (Jul 25 2018 at 15:50, on Zulip):

This pretty printer definitely needs tidied up, but I don't know it's the effort is worth it to remove that ugly string manipulation. There's _so_ much here.

I've got it separated out into two contexts - debug and display - that have the same code in them but that can now be changed per-context. I can remove all the display-specific code from the debug context, etc. Both impls are still constructed together through the macros though, so I can't start separating things out into easier to grok modules.

nikomatsakis (Jul 25 2018 at 16:17, on Zulip):

I think I had envisioned something far hackier

davidtwco (Jul 25 2018 at 16:17, on Zulip):

I took a break for a bit and ran the tests instead of just doing check.

davidtwco (Jul 25 2018 at 16:17, on Zulip):

Regressed in tons with this attempted refactor.

nikomatsakis (Jul 25 2018 at 16:17, on Zulip):

e.g.,

scoped_thread_local! {
    REGION_CALLBACK: ....
}
nikomatsakis (Jul 25 2018 at 16:18, on Zulip):

and then in the Display impl for Region

nikomatsakis (Jul 25 2018 at 16:18, on Zulip):

something like

REGION_CALLBACK.with(|c| if let Some(c) = c {
    c(..)
} else {
    // existing code
})
davidtwco (Jul 25 2018 at 16:18, on Zulip):

I think this is where the regions are numbered now.

nikomatsakis (Jul 25 2018 at 16:19, on Zulip):

ah, yes, that's where it does the regions like for<'r> fn(&'r u32)

nikomatsakis (Jul 25 2018 at 16:19, on Zulip):

that...could probably just stay the same I guess?

davidtwco (Jul 25 2018 at 16:19, on Zulip):

Hmm... I'll need to take a fresh look at this.

nikomatsakis (Jul 25 2018 at 16:20, on Zulip):

I was imagining invoking the hook here https://github.com/rust-lang/rust/blob/master/src/librustc/util/ppaux.rs#L733-L738

nikomatsakis (Jul 25 2018 at 16:20, on Zulip):

it sounds to me like you may have reached the point where it makes sense to stop and consider

nikomatsakis (Jul 25 2018 at 16:20, on Zulip):

hard to say; as you say, it's clear that this code could be nicer

nikomatsakis (Jul 25 2018 at 16:20, on Zulip):

but it does feel like a tar pit

davidtwco (Jul 25 2018 at 16:23, on Zulip):

I'm not sure. I've pushed my attempted refactor to another branch so it's not completely lost but I don't think that's the way to go.

davidtwco (Jul 25 2018 at 16:24, on Zulip):

I'll give something like you suggested a shot.

davidtwco (Jul 25 2018 at 16:26, on Zulip):

Though, not exactly sure how to do that with scoped_thread_local!.

nikomatsakis (Jul 25 2018 at 16:35, on Zulip):

oh, that's something we use in rustc, I kind of forget how it works though

nikomatsakis (Jul 25 2018 at 16:35, on Zulip):

it lets you set a value just for a time

nikomatsakis (Jul 25 2018 at 16:37, on Zulip):

you can just do it with thread_local too

nikomatsakis (Jul 25 2018 at 16:37, on Zulip):

we could even sort of hardcode the behavior

nikomatsakis (Jul 25 2018 at 16:37, on Zulip):

e.g., thread_local! { static highlight_region: Cell<Option<ty::RegionVid>> = ... }

nikomatsakis (Jul 25 2018 at 16:37, on Zulip):

and then just open code what we want

nikomatsakis (Jul 25 2018 at 16:37, on Zulip):

into the ppaux code

nikomatsakis (Jul 25 2018 at 16:38, on Zulip):

(if that is set to Some, then we would print '_ for all regions except the highlight-region)

davidtwco (Jul 25 2018 at 16:58, on Zulip):

@nikomatsakis Alright, let me clarify this:

I should put a scoped_thread_local!(..) in the ppaux module that has the RegionVid of the region we wish to highlight. In order for that function to generate the region name, it'll also need the counter: &mut usize.

In the new function from this PR, I can call highlight_region.set(&(region, counter), || { ... }) and in the closure, call the function that will create the pretty printed type. Then, I store that string outside of the closure so that I can return it for the rest of the error handling.

In the ppaux module, at the start of the define_print! for RegionKind, I can use highlight_region.with(|(region, counter)| ..) to compute the new region name in place of the existing logic, and get that outside of the closure to return that from there. To do this, I'll need to check if the RegionKind we're pretty printing is the RegionVid in our scoped thread local variable, somehow.

Is that roughly it?

davidtwco (Jul 25 2018 at 17:04, on Zulip):

Going to take a break, will likely return to this in a few hours.

nikomatsakis (Jul 25 2018 at 17:17, on Zulip):

I'd probably just use a regular thread_local in this case

nikomatsakis (Jul 25 2018 at 17:17, on Zulip):

something like:

thread_local! {
    static HIGHLIGHT_REGION: Cell<Option<(RegionVid, usize)>> = Cell::new(None)
}
nikomatsakis (Jul 25 2018 at 17:18, on Zulip):

then I would add a helper like

with_highlight_region<R>(r: RegionVid, counter: usize, op: impl FnOnce() -> R) -> R {
    HIGHLIGHT_REGION.with(|hr| {
        assert_eq!(hr.get(), None);
        hr.set(Some((r, counter)));
        let r = op();
        hr.set(None);
        r
    })
}
nikomatsakis (Jul 25 2018 at 17:19, on Zulip):

and at the start of the fn you can do

fn get_highlight_region() -> Option<(RegionVid, usize)> {
    HIGHLIGHT_REGION.with(|hr| hr.get())
}
davidtwco (Jul 26 2018 at 10:31, on Zulip):

@nikomatsakis Alright, got that working. Pushed a commit with it. Interestingly, that didn't work for one or two tests with regressed to what we had pre-this PR.

davidtwco (Jul 26 2018 at 10:45, on Zulip):

Yeah, it seems like in a handful of cases, the fmt function that I had to change to use HIGHLIGHT_REGION doesn't get called.

nikomatsakis (Jul 26 2018 at 11:44, on Zulip):

huh

nikomatsakis (Jul 26 2018 at 11:44, on Zulip):

that's pushed @David Wood to the PR?

davidtwco (Jul 26 2018 at 11:45, on Zulip):

It is.

davidtwco (Jul 26 2018 at 11:46, on Zulip):

The top two diffs for tests in this commit: https://github.com/rust-lang/rust/pull/52648/commits/70ed4e2c9789c1f8d81549ab4e68b03fcae24578

davidtwco (Jul 26 2018 at 11:47, on Zulip):

I believe it happens on types where by default (without any of the variations on this PR) it would just print &i32 rather than &'_#3r i32 or something like that.

davidtwco (Jul 26 2018 at 11:47, on Zulip):

I'm not sure what case causes that.

nikomatsakis (Jul 26 2018 at 11:49, on Zulip):

I see, yes, I think if the highlight region is set we will want to suppress or alter some of that logic

nikomatsakis (Jul 26 2018 at 11:49, on Zulip):

let me see..

nikomatsakis (Jul 26 2018 at 11:50, on Zulip):

weird it seems to always print the region here

davidtwco (Jul 26 2018 at 11:50, on Zulip):

Yeah, I found that.

nikomatsakis (Jul 26 2018 at 11:50, on Zulip):

@David Wood in those examples, the variable's type is &u32?

davidtwco (Jul 26 2018 at 11:51, on Zulip):

Yes, I said i32 as an example.

nikomatsakis (Jul 26 2018 at 11:53, on Zulip):

can you print out what string they do get?

nikomatsakis (Jul 26 2018 at 11:53, on Zulip):

it might be nice to also print the {:?} output

davidtwco (Jul 26 2018 at 11:54, on Zulip):

IIRC it is just &u32 without any lifetimes/region at all.

nikomatsakis (Jul 26 2018 at 11:56, on Zulip):

can you try with -Zverbose?

davidtwco (Jul 26 2018 at 12:05, on Zulip):

Let me check, just building after addressing some of the comments on the MR.

davidtwco (Jul 26 2018 at 12:16, on Zulip):

@nikomatsakis Works as before with -Z verbose.

nikomatsakis (Jul 26 2018 at 12:17, on Zulip):

ok so the region is there

davidtwco (Jul 26 2018 at 12:17, on Zulip):

(Also, is there a reason I can't see ppaux logging with RUST_LOG=rustc::util?)

nikomatsakis (Jul 26 2018 at 12:17, on Zulip):

not that I know of

nikomatsakis (Jul 26 2018 at 12:17, on Zulip):

oh wait

nikomatsakis (Jul 26 2018 at 12:18, on Zulip):

you need to customize this code here

davidtwco (Jul 26 2018 at 12:18, on Zulip):

If I have that in the RUST_LOG and -Z verbose then I get a core dump.

nikomatsakis (Jul 26 2018 at 12:18, on Zulip):

I'm sort of surprised it ever works :)

nikomatsakis (Jul 26 2018 at 12:18, on Zulip):

this line in particular looks problematic: https://github.com/davidtwco/rust/blob/70ed4e2c9789c1f8d81549ab4e68b03fcae24578/src/librustc/util/ppaux.rs#L782

davidtwco (Jul 26 2018 at 12:20, on Zulip):

Giving it a go now having changed the is_verbose check.

nikomatsakis (Jul 26 2018 at 12:25, on Zulip):

ok

davidtwco (Jul 26 2018 at 12:29, on Zulip):

Doing if is_verbose && get_highlight_region().is_none() makes it stop doing has_type when -Z verbose - now it never does it.

davidtwco (Jul 26 2018 at 12:30, on Zulip):

I guess it should be if is_verbose || get_highlight_region().is_some()

nikomatsakis (Jul 26 2018 at 12:30, on Zulip):

that would what I expect, yes :)

davidtwco (Jul 26 2018 at 12:30, on Zulip):

I blame the heat.

davidtwco (Jul 26 2018 at 12:30, on Zulip):

It's very warm.

davidtwco (Jul 26 2018 at 12:30, on Zulip):

Thinking is hard when warm.

DPC (Jul 26 2018 at 12:31, on Zulip):

xD

davidtwco (Jul 26 2018 at 12:38, on Zulip):

@nikomatsakis updated that PR, third time lucky.

nikomatsakis (Jul 26 2018 at 12:42, on Zulip):

left one more nit :) but the results are looking good

nikomatsakis (Jul 26 2018 at 12:42, on Zulip):

I definitely think I like this better than the text manipulation

davidtwco (Jul 26 2018 at 12:58, on Zulip):

@nikomatsakis Not sure I follow what your nit is asking for, particularly the second comment.

nikomatsakis (Jul 26 2018 at 12:58, on Zulip):

My goal would be that we print (e.g.) &Foo<'_, '1>

nikomatsakis (Jul 26 2018 at 12:58, on Zulip):

I think that this PR will print...something different now, maybe &Foo<'1>

nikomatsakis (Jul 26 2018 at 12:58, on Zulip):

(we should add a test for this, I can help with that if you want)

nikomatsakis (Jul 26 2018 at 12:59, on Zulip):

what I was suggesting is that — in highlight mode — RegionVid would return either '1 or '_

davidtwco (Jul 26 2018 at 12:59, on Zulip):

Ah, you want '_ in the type parameters but elsewhere?

nikomatsakis (Jul 26 2018 at 12:59, on Zulip):

then, in the TyRef ppaux code, we would look and see if the pretty-printed version of our region is "'_" and — if so — not print it

nikomatsakis (Jul 26 2018 at 12:59, on Zulip):

right now we look for ""

nikomatsakis (Jul 26 2018 at 12:59, on Zulip):

but &u32 and &'_ u32 should always be equivalent

nikomatsakis (Jul 26 2018 at 13:00, on Zulip):

Ah, you want '_ in the type parameters but elsewhere?

right :)

davidtwco (Jul 26 2018 at 13:00, on Zulip):

Alright, that all makes sense.

nikomatsakis (Jul 26 2018 at 13:03, on Zulip):

if you have a build handy, maybe try this test?

#![feature(nll)]
#![allow(warnings)]

struct Foo<'a, 'b, T: 'a + 'b> { x: &'a T, y: &'b T }

fn gimme(_: impl for<'a, 'b, 'c> FnOnce(&'a Foo<'a, 'b, u32>, &'a Foo<'a, 'c, u32>) -> &'a Foo<'a, 'b, u32>) { }

fn main() {
    gimme(|x, y| y)
}
nikomatsakis (Jul 26 2018 at 13:04, on Zulip):

I think that would show the types in question

nikomatsakis (Jul 26 2018 at 13:04, on Zulip):

also, instead of "free region requires that '1 must outlive '2", maybe we should just say "here, '1 must outlive '2"

nikomatsakis (Jul 26 2018 at 13:05, on Zulip):

or just "requires '1 to outlive '2"

nikomatsakis (Jul 26 2018 at 13:05, on Zulip):

something like that

davidtwco (Jul 26 2018 at 13:43, on Zulip):

Alright, pushed changes that make that case work fine and remove the "free region" part of messages.

davidtwco (Jul 26 2018 at 13:43, on Zulip):

@nikomatsakis

nikomatsakis (Jul 26 2018 at 13:45, on Zulip):

@David Wood :snowboarder: nice!

nikomatsakis (Jul 26 2018 at 13:47, on Zulip):

looks great

csmoe (Jul 26 2018 at 13:49, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/issues/52708 this issue seems to only involve some clean-up and matches my level, may I take care of it?

nikomatsakis (Jul 26 2018 at 13:50, on Zulip):

@csmoe certainly, leave a comment

davidtwco (Jul 26 2018 at 14:08, on Zulip):

Fixed the tidy error.

davidtwco (Jul 26 2018 at 16:15, on Zulip):

Tests passed.

davidtwco (Jul 27 2018 at 11:16, on Zulip):

Rebased this, needs another r+ after tests pass.

Last update: Nov 22 2019 at 00:55UTC