Stream: t-compiler

Topic: #60431 failed to get layout; field is proj `::U`


pnkfelix (Jul 11 2019 at 07:38, on Zulip):

Hey @eddyb , there's this bug where layout code ICE's because, I think, there's an unresolved variable in last field of an ADT (which is unsized), we are trying to get layout of &ADT.

pnkfelix (Jul 11 2019 at 07:39, on Zulip):

The LayoutCx<'tcx> does not have access to an InferCtxt<'a, 'tcx>, so I cannot call resolve_vars_if_possible to attempt to address this in a lazy fashon

pnkfelix (Jul 11 2019 at 07:41, on Zulip):

My question to you, @eddyb , is whether it is meant to be an invariant that all types that are fed to the layout code should always have all variables resolved (at least when we are doing layout from say rustc_codegen_llvm)? and if that is meant to be an invariant, is it one we might consider asserting up front in the code?

eddyb (Jul 11 2019 at 07:42, on Zulip):

@pnkfelix we don't have a good place to put it, but it's never correct for a query to have inference variables in its input

eddyb (Jul 11 2019 at 07:42, on Zulip):

this used to be prevented through lifetimes alone, FWIW

eddyb (Jul 11 2019 at 07:43, on Zulip):

but now that we removed the 'gcx/'tcx distinction, the query system has no automated protection against getting random inference variables into queries

eddyb (Jul 11 2019 at 07:45, on Zulip):

@pnkfelix if you're querying for layout from something that uses an inference context, you need to use canonicalization (which idk if anyone other than @nikomatsakis understands... we also need to add that to the const_eval query so we call it from unification during inference)

pnkfelix (Jul 11 2019 at 07:45, on Zulip):

well I personally don't mind trying to enforce such a rule via a dynamic check

pnkfelix (Jul 11 2019 at 07:45, on Zulip):

is "canonicalization" meant to mean something different from "normalization" there?

eddyb (Jul 11 2019 at 07:46, on Zulip):

the other thing you could do is resolve_vars_if_possible + avoiding calling layout_of unless there are no infer vars in the resolved type

eddyb (Jul 11 2019 at 07:46, on Zulip):

@pnkfelix yes, it's the replacement of inference variables with... effectively parameters

pnkfelix (Jul 11 2019 at 07:46, on Zulip):

??

pnkfelix (Jul 11 2019 at 07:47, on Zulip):

(My "??" was in response to the idea of avoiding calling layout_of ...)

eddyb (Jul 11 2019 at 07:47, on Zulip):

well, yeah, you can just assume it returned an error if you have inference variables

pnkfelix (Jul 11 2019 at 07:47, on Zulip):

It sounds to me like I need to trace further back in the control flow to see where someone is missing a call to resolve_vars_if_possible

eddyb (Jul 11 2019 at 07:47, on Zulip):

whatever is calling layout_of probably shouldn't. I can't for the life of me imagine where this might happen

pnkfelix (Jul 11 2019 at 07:48, on Zulip):

The linked issue from this topic shows an example

eddyb (Jul 11 2019 at 07:48, on Zulip):

you should just make sure you have debuginfo-level = 1 in config.toml and just get a backtrace

eddyb (Jul 11 2019 at 07:48, on Zulip):

I guess I can do it if it's a simple repro

pnkfelix (Jul 11 2019 at 07:48, on Zulip):

I can show you a back trace if you want to see one;

pnkfelix (Jul 11 2019 at 07:48, on Zulip):

I wasn't sure if you were on a mobile device and weren't able to look at a backtrace easily

eddyb (Jul 11 2019 at 07:48, on Zulip):

canonicalization is what used to be called "freshening" (I just remembered) - the difference is "canonicalization" is much more principled and it allows queries to reify their effects on inference, so that they're cached - basically it lets you go between two inference contexts and preserve everything

pnkfelix (Jul 11 2019 at 07:48, on Zulip):

oh okay

eddyb (Jul 11 2019 at 07:49, on Zulip):

at the office now. I tend to not touch Zulip on my phone because the app drains the battery like crazy

eddyb (Jul 11 2019 at 07:50, on Zulip):

there, hopefully that's a half-decent explanation :P

eddyb (Jul 11 2019 at 07:53, on Zulip):

@pnkfelix there's nothing about inference variables in the issue though?

pnkfelix (Jul 11 2019 at 07:53, on Zulip):

https://gist.github.com/pnkfelix/ee7813784d3002db14ba2ad59418b30a

pnkfelix (Jul 11 2019 at 07:53, on Zulip):

the inference variables arise, I believe, due to the projection?

eddyb (Jul 11 2019 at 07:53, on Zulip):

it's a proper layout error that rustc_codegen_llvm turns into an ICE

pnkfelix (Jul 11 2019 at 07:54, on Zulip):

You're saying layout is supposed to error here?

eddyb (Jul 11 2019 at 07:54, on Zulip):

'sec

pnkfelix (Jul 11 2019 at 07:54, on Zulip):

I was assuming it was legal to have a DST there

eddyb (Jul 11 2019 at 07:55, on Zulip):

I thought you were talking about https://github.com/rust-lang/rust/blob/master/src/librustc/ty/layout.rs#L1198

eddyb (Jul 11 2019 at 07:55, on Zulip):

whereas what happens is https://github.com/rust-lang/rust/blob/master/src/librustc/ty/layout.rs#L1188

pnkfelix (Jul 11 2019 at 07:56, on Zulip):

yes that looks right

pnkfelix (Jul 11 2019 at 07:56, on Zulip):

in terms of what is currently happening

eddyb (Jul 11 2019 at 07:56, on Zulip):

normalization fails but I don't think this is a layout-specific bug, layout just happens to be the first thing it hits

pnkfelix (Jul 11 2019 at 07:56, on Zulip):

or wait

pnkfelix (Jul 11 2019 at 07:56, on Zulip):

let me check atgain

pnkfelix (Jul 11 2019 at 07:57, on Zulip):

I see

eddyb (Jul 11 2019 at 07:57, on Zulip):

why on earth would normalization fail though :|

pnkfelix (Jul 11 2019 at 07:57, on Zulip):

I misinterpreted what was happening, hmm.

eddyb (Jul 11 2019 at 07:57, on Zulip):

this looks quite trivial

eddyb (Jul 11 2019 at 07:58, on Zulip):

oh nvm I'm wrong about where it's emitted from

eddyb (Jul 11 2019 at 07:59, on Zulip):

this is messed up https://github.com/rust-lang/rust/blob/master/src/librustc/ty/layout.rs#L541-L546

eddyb (Jul 11 2019 at 07:59, on Zulip):

it needs to actually normalize and call struct_tail in a loop

eddyb (Jul 11 2019 at 07:59, on Zulip):

might be a good idea to change struct_tail to take a param_env and normalize itself or something

eddyb (Jul 11 2019 at 08:01, on Zulip):

struct_lockstep_tails probably has the same problem...

pnkfelix (Jul 11 2019 at 08:03, on Zulip):

Okay I will look into that

eddyb (Jul 11 2019 at 08:03, on Zulip):

@pnkfelix hmm I think I know why this is a rare bug: it won't trigger if a pointee is Sized (after deep normalization), which Ty::is_sized checks

eddyb (Jul 11 2019 at 08:04, on Zulip):

it's the "tails" functionality that's problematic

eddyb (Jul 11 2019 at 08:04, on Zulip):

might be a good idea to move it to LayoutCx or something

eddyb (Jul 11 2019 at 08:07, on Zulip):

typeck only uses it 2 times, and the use in wfcheck should be layout-driven anyway

pnkfelix (Jul 11 2019 at 08:08, on Zulip):

move fn struct_lockstep_tails to LayoutCx, you mean, right?

pnkfelix (Jul 11 2019 at 08:09, on Zulip):

oh I guess fn struct_tail too

eddyb (Jul 11 2019 at 08:10, on Zulip):

yeah, both of them

eddyb (Jul 11 2019 at 08:12, on Zulip):

it's one of those things where it needs normalization after substituting generic args into a field type

eddyb (Jul 11 2019 at 08:12, on Zulip):

and what kind of normalization is to be done depends on the user of the resulting type...

pnkfelix (Jul 11 2019 at 08:12, on Zulip):

and the loop can probably be written to just keep going until struct_tail(pointee) == pointee, right?

pnkfelix (Jul 11 2019 at 08:13, on Zulip):

No need to try to be cleverer than that?

eddyb (Jul 11 2019 at 08:13, on Zulip):

if you make struct_tail do the normalization itself, you don't need loops in callers

pnkfelix (Jul 11 2019 at 08:13, on Zulip):

right, I'm just checking about what the condition inside struct_tail would be

eddyb (Jul 11 2019 at 08:13, on Zulip):

not having loops in several places is pretty much the only reason struct_tail exists

pnkfelix (Jul 11 2019 at 08:14, on Zulip):

I guess I shuod look at the actual code for fn struct_tail before trying to guess what the condition would look like

eddyb (Jul 11 2019 at 08:17, on Zulip):

sorry, github was being slow

eddyb (Jul 11 2019 at 08:17, on Zulip):

you'd pretty much want this https://github.com/rust-lang/rust/blob/master/src/librustc/ty/layout.rs#L1185-L1191

eddyb (Jul 11 2019 at 08:20, on Zulip):

since you're in a loop, returning the error is replaced with returning the type as-is and the success case is keeping the loop going

pnkfelix (Jul 11 2019 at 08:20, on Zulip):

okay. I'll look into that then

pnkfelix (Jul 11 2019 at 08:20, on Zulip):

Not sure how much time I want to spend trying to figure out the test cases to exercise the various calls to struct_tail

pnkfelix (Jul 11 2019 at 08:20, on Zulip):

but maybe it will become obvious

eddyb (Jul 11 2019 at 08:21, on Zulip):

pretty much layout and unsizing casts

eddyb (Jul 11 2019 at 08:23, on Zulip):

you might even get away with <Foo as Trait>::Bar in a field type, with no parameters involved, if there's no early normalization happening there (but for good measure you should probably use parameters to block normalization from happening before the last moment)

pnkfelix (Jul 11 2019 at 08:28, on Zulip):

Should the loop now also be incrementing some recursion counter, at least in the projection/opaque case?

pnkfelix (Jul 11 2019 at 08:28, on Zulip):

/me thinks

eddyb (Jul 11 2019 at 08:29, on Zulip):

it's not recursing though, unless you think someone could make an infinite loop

pnkfelix (Jul 11 2019 at 08:29, on Zulip):

that's what I was worried about, something ike

eddyb (Jul 11 2019 at 08:30, on Zulip):

I'd suggest trying it and seeing if there are other parts of the compiler catching it

eddyb (Jul 11 2019 at 08:30, on Zulip):

worst case we can make it a query in which case cycles would be detected automatically

pnkfelix (Jul 11 2019 at 08:30, on Zulip):

struct Inf { last: Foo::Assoc, } where impl Trait for Foo { Assoc = Inf; }, or some such garbage

eddyb (Jul 11 2019 at 08:31, on Zulip):

you'd need a type parameter, pretty sure that's just banned as-is

pnkfelix (Jul 11 2019 at 08:31, on Zulip):

okay, we'll see.

pnkfelix (Jul 11 2019 at 08:36, on Zulip):

I guess the answer is that it (the example I'm worried about) ICE's elsewhere: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f0829f8745f6af3f9283fb13bfc4164d

pnkfelix (Jul 11 2019 at 08:39, on Zulip):

Other examples give me more confidence, though; We do detect the cycle nicely here: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=2ccb4c4d56ef21595a5d5c52223ba3ac

eddyb (Jul 11 2019 at 08:40, on Zulip):

lol, some const_eval ICEs are way off

eddyb (Jul 11 2019 at 08:40, on Zulip):

probably because they assume too much

eddyb (Jul 11 2019 at 08:41, on Zulip):

@pnkfelix dbg! doesn't borrow the value like println! does https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=96f0c2536d87b5e9a5b85c9f5b66a5b5

pnkfelix (Jul 11 2019 at 08:41, on Zulip):

ah nice

eddyb (Jul 11 2019 at 08:42, on Zulip):

what these cycle errors from layout tell me is that struct_tail needs to be a query like raw_layout :(

eddyb (Jul 11 2019 at 08:43, on Zulip):

at least then it's gonna be cached :P

pnkfelix (Jul 11 2019 at 09:30, on Zulip):

Sigh: trying to normalize projections for all current calls to struct_tail and struct_lockstep_tails causes compiler to ICE on three ui tests.

pnkfelix (Jul 11 2019 at 09:40, on Zulip):

ah, I need to be careful about whether I call normalize_erasing_regions, which is only for use after type-check. Hmm.

pnkfelix (Jul 11 2019 at 09:43, on Zulip):

Hmm; The comment above fn normalize implies that even though it returns Result, you really aren't supposed to use it if you don't expect Ok ?

eddyb (Jul 11 2019 at 09:43, on Zulip):

ugh :/

pnkfelix (Jul 11 2019 at 09:44, on Zulip):

I wonder if that comment is out-of-date...

eddyb (Jul 11 2019 at 09:44, on Zulip):

@pnkfelix the wfcheck case should be fine to use the layout region-erasing thing, since it doesn't use the resulting type, just checks if it's extern type

eddyb (Jul 11 2019 at 09:44, on Zulip):

and the librustc_typeck/check/mod.rs use of struct_tail should probably just inline the non-normalizing loop

eddyb (Jul 11 2019 at 09:45, on Zulip):

or use the normalization facilities it has itself

eddyb (Jul 11 2019 at 09:45, on Zulip):

(but not call the layout struct_tail)

pnkfelix (Jul 11 2019 at 09:45, on Zulip):

I'm currently seeing an ICE from this use of struct_tail in fn rvalue_hint

pnkfelix (Jul 11 2019 at 09:45, on Zulip):

ah okay you just addressed that

pnkfelix (Jul 11 2019 at 09:47, on Zulip):

I was considering making the struct_tail method take a parameter indicting whether to normalize or not (or even have it be parameterized over how normalization should be done...)

eddyb (Jul 11 2019 at 09:47, on Zulip):

not sure it's worth it

eddyb (Jul 11 2019 at 09:47, on Zulip):

unless you take a closure and typeck can do whatever it wants, even resolve inference vars

eddyb (Jul 11 2019 at 09:48, on Zulip):

(but it'd be tedium for all the other call sites)

pnkfelix (Jul 11 2019 at 09:49, on Zulip):

well there could be two methods

pnkfelix (Jul 11 2019 at 09:49, on Zulip):

that call a common helper

eddyb (Jul 11 2019 at 09:49, on Zulip):

@pnkfelix hmm what about struct_tails_with_normalize for the generic version and struct_tails_erasing_lifetimes for the one that almost everything should use?

pnkfelix (Jul 11 2019 at 09:49, on Zulip):

yeah maybe

eddyb (Jul 11 2019 at 09:50, on Zulip):

(impl Fn(Ty<'tcx>) -> Ty<'tcx> should be enough I'm guessing)

eddyb (Jul 12 2019 at 09:35, on Zulip):

@pnkfelix not trying to waste of your time, I think we can get away without rephrasing https://github.com/rust-lang/rust/pull/62585#discussion_r302905131

eddyb (Jul 12 2019 at 09:35, on Zulip):

just changing "checking" to "inference"

pnkfelix (Jul 12 2019 at 09:35, on Zulip):

heh I just finished making a change there

eddyb (Jul 12 2019 at 09:36, on Zulip):

push and I'll look

pnkfelix (Jul 12 2019 at 09:36, on Zulip):

I actually went with "for example, during codegen"

pnkfelix (Jul 12 2019 at 09:36, on Zulip):

i'll show you

eddyb (Jul 12 2019 at 09:36, on Zulip):

(I don't like to name codegen in particular because it's just part of the majority)

pnkfelix (Jul 12 2019 at 09:37, on Zulip):

it seemed like the vast majority of usage. :)

eddyb (Jul 12 2019 at 09:37, on Zulip):

miri and lints also use layout

eddyb (Jul 12 2019 at 09:38, on Zulip):

@pnkfelix ah whatever it's fine

pnkfelix (Jul 12 2019 at 09:38, on Zulip):

miri == interpreter ... arguably just another kind of codegen. :)

eddyb (Jul 12 2019 at 09:38, on Zulip):

this sort of thing is searcheable for anyway

pnkfelix (Jul 12 2019 at 09:38, on Zulip):

anyway I was trying to take care not to saay that it was only usable in codegen

eddyb (Jul 12 2019 at 09:38, on Zulip):

yeah it makes sense

eddyb (Jul 12 2019 at 09:39, on Zulip):

(particularly that codegen doesn't care about lifetimes, that's specific enough that I don't mind)

eddyb (Jul 12 2019 at 09:39, on Zulip):

remove the } indent fix and r=me

eddyb (Jul 12 2019 at 09:39, on Zulip):

and enjoy your time off :P

pnkfelix (Jul 12 2019 at 09:40, on Zulip):

yeah I wanted to double check about that indent fix

pnkfelix (Jul 12 2019 at 09:40, on Zulip):

I want to see what rustfmt does there

pnkfelix (Jul 12 2019 at 09:40, on Zulip):

its something with the match guard up above it

eddyb (Jul 12 2019 at 09:40, on Zulip):

are using "only reformat those lines"?

pnkfelix (Jul 12 2019 at 09:40, on Zulip):

oh I see, yeah that's what happened

pnkfelix (Jul 12 2019 at 09:41, on Zulip):

ugh

eddyb (Jul 12 2019 at 09:41, on Zulip):

because I picked up a script from someone else that does it off of git diffs and ran into this sort of nonsense all over the place

pnkfelix (Jul 12 2019 at 09:41, on Zulip):

I dont want to cause that much rightward drift, let me see

eddyb (Jul 12 2019 at 09:41, on Zulip):

so I had to manually look at every single change

pnkfelix (Jul 12 2019 at 09:42, on Zulip):

hmm okay I didn't even touch the match guard, did I

pnkfelix (Jul 12 2019 at 09:42, on Zulip):

so I really should put it back the way it was before. Will do.

nikomatsakis (Jul 12 2019 at 10:40, on Zulip):

@pnkfelix (just checking in, it seems like canonicalization is not relevant here?)

pnkfelix (Jul 12 2019 at 10:41, on Zulip):

it wasn't, no

pnkfelix (Jul 12 2019 at 10:41, on Zulip):

I completely misdiagnosed the bug when I was first looking at it

Last update: Nov 22 2019 at 04:40UTC