Stream: wg-traits

Topic: #46989 fn pointers and universes


nikomatsakis (Feb 13 2019 at 14:21, on Zulip):

So the universes PR changed behavior around fn pointers, as described in #46989 -- @Aaron Turon or perhaps @tmandry / @Sunjay Varma, I was wondering if one of you might like to investigate a bit (I can help but I'm kind of busy today). I'd like to understand why this behavior changed.

nikomatsakis (Feb 13 2019 at 14:21, on Zulip):

As I wrote in that issue, I'm also contemplating if we can "partially roll back" the universe changes temporarily to give us more room to investigate the changed behaviors.

Aaron Turon (Feb 13 2019 at 21:27, on Zulip):

I plan to start looking into this issue either this afternoon or tomorrow

nikomatsakis (Feb 13 2019 at 21:39, on Zulip):

@Aaron Turon cool, feel free to drop notes here as you progress

Aaron Turon (Feb 13 2019 at 21:40, on Zulip):

will do!

tmandry (Feb 13 2019 at 22:06, on Zulip):

I had opened up and poked at the examples this morning.
noticing now that the two examples in the OP still don't compile, but with a different error message now (playground):

4 |     Bar(fn(&i32)), // Fails
  |         ^^^^^^^^ one type is more general than the other
  |
  = note: expected type `for<'r> fn(&'r i32)`
             found type `fn(&i32)`

..so this at least feels like inconsistent behavior between this example and @nagisa's example

tmandry (Feb 13 2019 at 22:22, on Zulip):

@Aaron Turon feel free to take it from here, just noting that

Aaron Turon (Feb 13 2019 at 22:26, on Zulip):

@tmandry we could also try to tackle it together! from the looks of it though i won't be able to start until tomorrow

tmandry (Feb 13 2019 at 22:30, on Zulip):

okay, well I'm curious enough about it that I want to dig in to different aspects of the issue that I don't understand, but I may not have time to do a full analysis

lqd (Feb 13 2019 at 22:32, on Zulip):

@tmandry FYI the playground link is missing the "Share" gist id

tmandry (Feb 13 2019 at 22:33, on Zulip):

@lqd d'oh, fixed, thanks

lqd (Feb 13 2019 at 22:35, on Zulip):

it reminds me of #57875, because of the Fn's error message

lqd (Feb 13 2019 at 22:42, on Zulip):

(and if so, maybe this'll help: my previous investigation on this issue lead to the match in try_report_placeholder_conflict it was a SubSupConflict like half of these match arms are about, but with PolyTraitRefs instead of TraitRefs which were causing similar errors in #57362 — but of course it's different here, and only part of the story since the universes PR landed; not very related to why the universes PR changed previous fn pointer behaviour)

tmandry (Feb 13 2019 at 23:04, on Zulip):

Interesting example that works on stable but regresses on beta: playground

tmandry (Feb 13 2019 at 23:04, on Zulip):

This is in fact exactly the case that was discussed as part of #44224 regression in method dispatch, where the decision was made to go with the current behavior of stable

tmandry (Feb 13 2019 at 23:08, on Zulip):

@SimonSapin notes in tracking issue for #56105 universe transition that this changes back to the "old" behavior, before #44224, re-breaking some code in servo

tmandry (Feb 13 2019 at 23:14, on Zulip):

Okay, I left a comment on the issue summarizing this.

tmandry (Feb 13 2019 at 23:38, on Zulip):

I'm planning to stay in the realm of "what is the right behavior" rather than "why does it behave this way" for now, btw :)

tmandry (Feb 13 2019 at 23:43, on Zulip):

I think at this point it's pretty clearly a regression

nikomatsakis (Feb 14 2019 at 17:34, on Zulip):

Interesting example that works on stable but regresses on beta: playground

that was expected breakage, @tmandry

nikomatsakis (Feb 14 2019 at 17:35, on Zulip):

@SimonSapin notes in tracking issue for #56105 universe transition that this changes back to the "old" behavior, before #44224, re-breaking some code in servo

and this too was expected

nikomatsakis (Feb 14 2019 at 17:35, on Zulip):

now, whether that's...ok is a different question

nikomatsakis (Feb 14 2019 at 17:35, on Zulip):

I should have reached out more aggressively to Simon Sapin, since I knew that was likely to happen

nikomatsakis (Feb 14 2019 at 17:36, on Zulip):

But indeed breaking that code was part of the point of the PR

nikomatsakis (Feb 14 2019 at 17:36, on Zulip):

One thing I'm wondering now, though, is whether to do a temporary (or maybe not) change to restore older behavior. I can certainly imagine various changes one could use to do that.

nikomatsakis (Feb 14 2019 at 17:36, on Zulip):

I think literally backing out the PR would be hard

tmandry (Feb 14 2019 at 17:43, on Zulip):

Okay, glad that's clear. It still seems like the behavior isn't entirely self-consistent (see this playground)

nikomatsakis (Feb 14 2019 at 17:44, on Zulip):

Yes, that is basically the bug I wanted to be investing in #57639, right?

tmandry (Feb 14 2019 at 17:47, on Zulip):

I'm not sure =)

tmandry (Feb 14 2019 at 17:48, on Zulip):

As in I haven't actually looked into that issue

nikomatsakis (Feb 15 2019 at 20:39, on Zulip):

So, I thought that this was maybe going to be fixed by https://github.com/rust-lang/rust/pull/58056, but I just did a build and I might be wrong

nikomatsakis (Feb 15 2019 at 20:40, on Zulip):

(cc @Aaron Turon)

nikomatsakis (Feb 15 2019 at 20:40, on Zulip):

let me dump out some debug info and see what I see

nikomatsakis (Feb 15 2019 at 21:00, on Zulip):

I'm going to live-dump some notes here, in case anyone cares

nikomatsakis (Feb 15 2019 at 21:00, on Zulip):

so I'm going through the logs for this example

nikomatsakis (Feb 15 2019 at 21:01, on Zulip):
trait Foo {

}

impl<A> Foo for fn(A) { }

fn assert_foo<T: Foo>() {}

fn main() {
    assert_foo::<fn(&i32)>();
}
nikomatsakis (Feb 15 2019 at 21:03, on Zulip):

I find these lines:

DEBUG 2019-02-15T20:44:53Z: rustc::traits::select: assembled 1 candidates for TraitObligationStack(Obligation(predicate=Binder(TraitPredicate(<for<'r> fn(&ReLateBound(DebruijnInde\
x(0), BrNamed(crate0:DefIndex(0:0), 'r)) i32) as Foo>)),cause=ObligationCause { span: /home/nmatsakis/tmp/issue-46989.rs:1:1: 1:1, body_id: HirId { owner: DefIndex(0:0), local_id: 0 }, code: MiscObligation },param_env=ParamEnv { caller_bounds: [], reveal: UserFacing, def_id: None },depth=0)): [ImplCandidate(DefId(0/0:4 ~ issue_46989[317d]::{{impl}}[0]))]
DEBUG 2019-02-15T20:44:53Z: rustc::traits::select: CACHE MISS: SELECT(Binder(TraitPredicate(<for<'r> fn(&ReLateBound(DebruijnIndex(0), BrNamed(crate0:DefIndex(0:0), 'r)) i32) as Foo>)))=Ok(Some(ImplCandidate(DefId(0/0:4 ~ issue_46989[317d]::{{impl}}[0]))))

which seem to be the point where we are doing the trait matching. We select the impl candidate. We go to the confirmation step, where we equate the types:

 rustc::infer::at: eq(<for<'r> fn(&ReLateBound(DebruijnIndex(0), BrNamed(crate0:DefIndex(0:0), 'r)) i32) as Foo> == <fn(_#0t) as Foo>)

this ought to fail, but I guess it does't... let me trace that out now

nikomatsakis (Feb 15 2019 at 21:04, on Zulip):

we come to the point where we equate the types:

rustc::infer::equate: Equate.tys(for<'r> fn(&ReLateBound(DebruijnIndex(0), BrNamed(crate0:DefIndex(0:0), 'r)) i32), fn(_#0t))
nikomatsakis (Feb 15 2019 at 21:06, on Zulip):

to equate fn types, we presently first check that T1 <: T2 and T2 <: T1

nikomatsakis (Feb 15 2019 at 21:09, on Zulip):

ha,ok, I think I see what is happening

nikomatsakis (Feb 15 2019 at 21:20, on Zulip):

actually @Aaron Turon this is an interesting example, if I'm not mistaken, and comes back to the question raised by @Sunjay Varma in our call. Let me try to walk it through. We start out with

for<'a> fn(&'a u32) == fn(?0)

where ?0 is a type inference variable. The way our code handles equating two fn types is to convert that into a pair of constraints:

for<'a> fn(&'a u32) <: fn(?0)
fn(?0) <: for<'b> fn(&'b u32) // `'a` alpha renamed to `'b` for clarity

when we process the first one, we instantiate the 'a with a fresh inference variable (as it appears on the subtype), so we get

fn(&'?1 u32) <: fn(?0) // which is true if...
?0 <: &'?1 u32

we then instantiate ?0 with a value of &'?2 u32 and relate those two types:

&'?2 u32 <: &'?1 u32 // which is true if...
'?2: '?1 // resulting in this region constraint

Next, we go to process the other direction. But now we know what ?0 is, so we can expand that:

fn(&'?2 u32) <: for<'b> fn(&'b u32)

Here, we replace 'b with a placeholder:

fn(&'?2 u32) <: fn(&'!b u32)
&'!b u32 <: &'?2 u32
'!b: '?2

Thus we wind up -- ultimately -- with the following region obligations:

'?2: '?1
'!b: '?2

Or, in the actual debug output:

DEBUG 2019-02-15T20:44:53Z: rustc::infer::lexical_region_resolve: ----() Start constraint listing (context=DefId(0/0:6 ~ issue_46989[317d]::main[0])) ()----
DEBUG 2019-02-15T20:44:53Z: rustc::infer::lexical_region_resolve: Constraint 0 => VarSubVar('_#0r, '_#1r)
DEBUG 2019-02-15T20:44:53Z: rustc::infer::lexical_region_resolve: Constraint 1 => VarSubReg('_#1r, RePlaceholder(Placeholder { universe: U3, name: BrAnon(0) }))
DEBUG 2019-02-15T20:44:53Z: rustc::infer::lexical_region_resolve: ----() End constraint listing (context=DefId(0/0:6 ~ issue_46989[317d]::main[0])) ()---

This, as it happens, is eminently solveable == '0 and '1 can be inferred to empty regions.

nikomatsakis (Feb 15 2019 at 21:21, on Zulip):

Now, why is that allowed.. the problem is that these "temporary" regions we create during the subtyping never get related to any actual point in the program

nikomatsakis (Feb 15 2019 at 21:24, on Zulip):

But yeah basically this comes down to

for<'a> fn(&'a u32) == fn(&'empty u32)
nikomatsakis (Feb 15 2019 at 21:24, on Zulip):

and hence we infer A to be &'empty u32

nikomatsakis (Feb 15 2019 at 21:25, on Zulip):

which seems...not obviously wrong, but also like it maybe interacts poorly with other things

nikomatsakis (Feb 15 2019 at 21:25, on Zulip):

I'm getting nervous if there is some interaction with projection

nikomatsakis (Feb 15 2019 at 21:30, on Zulip):

but I guess that's not really true, in this particular case at least, as 'empty is really the only option

nikomatsakis (Feb 15 2019 at 21:31, on Zulip):

but this comes back @Aaron Turon to your question here. i.e., I was wrong to limit this to 'lifetime parameters', really we would want to generalize to any parameter.

nikomatsakis (Feb 15 2019 at 21:33, on Zulip):

This might even be necessary for soundness, if we were to generalize. i.e., if you had some setup such that you could infer to a range of other values?

nikomatsakis (Feb 15 2019 at 21:33, on Zulip):

I'm getting more and more convinced that we should revert this PR and discuss it more

Aaron Turon (Feb 15 2019 at 22:14, on Zulip):

hm, ok -- i don't totally follow the last run of comments, but it does seem good to assess reverting

Aaron Turon (Feb 19 2019 at 23:44, on Zulip):

@nikomatsakis so i've been trying to see what kinds of problems i can create around the 'empty concept. in particular, i'm trying to find a way to force the compiler to infer that some lifetime parameter must be 'empty and then use that for nefarious purposes

Aaron Turon (Feb 19 2019 at 23:44, on Zulip):

the closest I've come is:

trait CastTo<B> {
    fn cast(self) -> B;
}
impl<'a, 'b> CastTo<&'b String> for &'a String where 'a: 'b {
    fn cast(self) -> &'b String {
        self
    }
}

fn foo<'a, 'b>(x: &'a String) -> &'b String where for<'z> &'z String: CastTo<&'b String> {
    x.cast()
}

fn main() {
    let empty = {
        let x = "hello".to_string();
        foo(&x)
    };
    println!("{}", empty)
}
Aaron Turon (Feb 19 2019 at 23:45, on Zulip):

that doesn't compile in stable or nightly, but the error message is different in the two cases (probably not for any deep reasons)

Aaron Turon (Feb 19 2019 at 23:45, on Zulip):

but anyway, perhaps you can see what i was trying to do here, namely to get rustc to infer that 'b must in fact be 'empty

Aaron Turon (Feb 19 2019 at 23:45, on Zulip):

then use that to sidestep region checking

Aaron Turon (Feb 19 2019 at 23:46, on Zulip):

even if i could get the compiler to make that inference, though, it's not clear to me whether it really would sidestep region checking

Aaron Turon (Feb 19 2019 at 23:47, on Zulip):

i'm not sure exactly how 'empty is treated

nikomatsakis (Feb 19 2019 at 23:47, on Zulip):

that doesn't compile in stable or nightly, but the error message is different in the two cases (probably not for any deep reasons)

yeah those errors are basically equivalent

nikomatsakis (Feb 19 2019 at 23:47, on Zulip):

one is the new version of the other, I think

nikomatsakis (Feb 19 2019 at 23:48, on Zulip):

I'm not sure I 100% see what you are trying to do

Aaron Turon (Feb 19 2019 at 23:48, on Zulip):

do you know, if one was ever to get a value of type &'empty T, whether the region checker would instantly complain about it escaping?

nikomatsakis (Feb 19 2019 at 23:48, on Zulip):

well, it's not quite like that, it's more like

Aaron Turon (Feb 19 2019 at 23:48, on Zulip):

well so the only choice of 'b for foo is 'empty

nikomatsakis (Feb 19 2019 at 23:48, on Zulip):

the region checker ought to be adding constraints

nikomatsakis (Feb 19 2019 at 23:48, on Zulip):

such that every value that gets used and/or is stored into a local variable etc

Aaron Turon (Feb 19 2019 at 23:49, on Zulip):

so i'm trying to make a function which will give you back a &'b String, where 'b is known to the compiler to be 'empty

nikomatsakis (Feb 19 2019 at 23:49, on Zulip):

is required to outlive the expressions/scopes/whatever where it appears

Aaron Turon (Feb 19 2019 at 23:49, on Zulip):

right ok

nikomatsakis (Feb 19 2019 at 23:49, on Zulip):

so if you somehow got 'empty it should grow to the scope

Aaron Turon (Feb 19 2019 at 23:49, on Zulip):

so in some sense there's a safeguard there already

nikomatsakis (Feb 19 2019 at 23:49, on Zulip):

which in turn would (presumably) cause an error

Aaron Turon (Feb 19 2019 at 23:49, on Zulip):

right

nikomatsakis (Feb 19 2019 at 23:50, on Zulip):

so in some sense there's a safeguard there already

yeah -- and there have definitely been bugs with missing constraints in the past, so there could be more, though those usually show up as bugs in other ways too

Aaron Turon (Feb 19 2019 at 23:50, on Zulip):

right

nikomatsakis (Feb 19 2019 at 23:50, on Zulip):

the more interesting question I think is whether you can prove something in a generic context

Aaron Turon (Feb 19 2019 at 23:50, on Zulip):

TLDR there's not a fundamental reason to fear 'empty being a thing

nikomatsakis (Feb 19 2019 at 23:50, on Zulip):

that winds up being unsound post substitution

Aaron Turon (Feb 19 2019 at 23:50, on Zulip):

and in fact we could expose it in the surface syntax without issue, given what you're saying

Aaron Turon (Feb 19 2019 at 23:51, on Zulip):

yeah ok

nikomatsakis (Feb 19 2019 at 23:51, on Zulip):

I .. think that is correct, it would just be an error just about everywhere it appeared

Aaron Turon (Feb 19 2019 at 23:51, on Zulip):

gotta run for now, will keep mulling though

nikomatsakis (Feb 19 2019 at 23:51, on Zulip):

at least I would say, that had better be true :)

nikomatsakis (Feb 20 2019 at 12:16, on Zulip):

Somewhat amusing: As part of the universe transition, I added these tests:

in which I deduced all of the above (e.g., the fn(&u8) = fn(&'empty u8) and so on) but I apparently totally forgot about that.

Last update: Nov 12 2019 at 17:10UTC