Stream: t-compiler/wg-nll

Topic: issue-46983


davidtwco (May 31 2018 at 18:44, on Zulip):

@pnkfelix in this comment, you mention that the unit test case added was using -Z verbose - where is it you are seeing that, I can't seem to find that?

pnkfelix (May 31 2018 at 18:45, on Zulip):

hmm. I know I saw -Z verbose in a unit test, but maybe I was wrong to say it was the tst that had been filed in this issue

pnkfelix (May 31 2018 at 18:46, on Zulip):

let me go find what I was talking about

nikomatsakis (May 31 2018 at 18:58, on Zulip):

sometimes I have added those into tests, not always for a reason that is still relevant

davidtwco (May 31 2018 at 19:00, on Zulip):

I've actually not checked the current state of the test - only what the PR added, let me take a look.

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

Okay well I was clearly wrong to say that that PR's unit test had -Z verbose on it. I must have been looking at one of the (many) other NLL tests that do have -Z verbose

davidtwco (May 31 2018 at 19:03, on Zulip):

No, I found it - this one here.

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

there's a bunch of other ones too though, no?

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

in both ui/nll/closure-requirements and in ui/nll/ty-outlives/

davidtwco (May 31 2018 at 19:05, on Zulip):

I've only looked at those in the previous PR.

davidtwco (May 31 2018 at 19:05, on Zulip):

Of which it was only that test, I believe.

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

Ah I see.

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

okay that I could believe, that I was looking at the wrong unit test in the PR, and not even looking at the diff in the first place

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

sometimes I have added those into tests, not always for a reason that is still relevant

I can certainly understand adding -Z verbose to tests.

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

but I think we need a blanket policy that ui/ tests should not have -Z verbose

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

(well, maybe that is too harsh. I don't know.)

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

but I think we need a blanket policy that ui/ tests should not have -Z verbose

why?

davidtwco (May 31 2018 at 20:50, on Zulip):

What message are we expecting in this test - do we just want to have a region name? if so, what would that name be?

davidtwco (May 31 2018 at 20:50, on Zulip):

Been looking through the code that does this error and I think I get it all, just unsure what the intended behavior is - other than not being a blank name.

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

hmm

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

this is one of those tricky cases

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

in that the regionck-based error is not something I'm thrilled with

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

that said, I think there's a case to be made for this being lower priority —

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

in particular, I imagine that when we ship initially, we'll have the regionck error enabled :)

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

that is, this error

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

maybe we should disable the code that is converting that into a warning

davidtwco (May 31 2018 at 20:55, on Zulip):

I see, that is better.

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

it's certainly better than the NLL one

davidtwco (May 31 2018 at 20:56, on Zulip):

Out of curiousity, why was that code disabled - so we can see the errors from NLL? Why would we want to do that when in practice it'd be enabled and we wouldn't see the NLL errors anyway?

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

it was disabled because NLL sometimes accepts things that regionck would reject

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

in other words, NLL does both regionck + borrowck

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

however the less-precise NLL we do now

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

probably doens't have as many cases where regionck gives an error

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

and yet NLL would accept it

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

I imagine they still exist though

davidtwco (May 31 2018 at 20:57, on Zulip):

I see.

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

but still, when we ship, if we are going to run the AST-based checker

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

we have to run regionck

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

I guess it's still plausible we could silence its errors though

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

I'd be interested to know if there are run-pass cases that emit that warning

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

but still pass

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

that we care about :)

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

on the other hand, we probably can dump a similar sort of trace

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

this is one of the things that @pnkfelix and I were discussing

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

in that I would prefer to take a stab at doing better than the AST-based error here

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

but I don't have a concrete idea — we'd have to dig in a bit

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

but that may be worth it

davidtwco (May 31 2018 at 20:58, on Zulip):

Yeah, could be interesting.

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

I mean doing better than those errors has been a long-standing goal

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

let me think...

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

do you happen to have the graphviz of the region constraints handy?

davidtwco (May 31 2018 at 21:00, on Zulip):

I do not.

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

if you run with -Zdump-mir=nll it will get produced

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

but I could do it

davidtwco (May 31 2018 at 21:02, on Zulip):

rustc.main.-.nll.0.regioncx.dot

davidtwco (May 31 2018 at 21:02, on Zulip):

rustc.impl-drop.-.nll.0.regioncx.dot

davidtwco (May 31 2018 at 21:02, on Zulip):

rustc.use_x.-.nll.0.regioncx.dot

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

one sec

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

wait, @David Wood, which test are we talking about again?

davidtwco (May 31 2018 at 21:06, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/test/ui/underscore-lifetime/dyn-trait-underscore.nll.stderr

davidtwco (May 31 2018 at 21:07, on Zulip):

Unless that mir_dump folder had some leftovers from a previous build in it.

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

bah I can't figure out how to branch out a new topic

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

I was confused by the title of this thread :)

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

pnkfelix had some way to branch out all preivous messages

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

but I don't know how @pnkfelix did that

davidtwco (May 31 2018 at 21:08, on Zulip):

rustc.a.-.nll.0.regioncx.dot
rustc.b.-.nll.0.regioncx.dot
rustc.c.-.nll.0.regioncx.dot
rustc.main.-.nll.0.regioncx.dot

davidtwco (May 31 2018 at 21:08, on Zulip):

That's the proper ones.

davidtwco (May 31 2018 at 21:08, on Zulip):

Not sure what happened there.

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

but I think we need a blanket policy that ui/ tests should not have -Z verbose

why?

because ui/ tests are (or should be?) centered around end-user visible behavior, while -Z verbose deliberately distorts the print-outs in a manner that leaks into our UI.

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

I don't agree with that assessment about ui tests I guess :)

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

I think they are just "tests"

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

and all other tests (run-pass, compile-fail, etc) should go away

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

some of them highlight end-user visibile behavior, but others are using #[rustc_foo] attributes to dump internal state...

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

that said, maybe it's a useful distinction to make

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

not sure if I'd do it via the directory vs an attribute or something in the test...

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

(also, fwiw, I collected most of the tests that need to use -Zverbose into a directory)

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

(in particular the ui/nll/closure-requirements/ directory)

Last update: Nov 22 2019 at 00:20UTC