nll type annotations in multisegment path #55093

nikomatsakis · 2018-10-15T13:05:13Z

This turned out to be sort of tricky. The problem is that if you have a path like

<Foo<&'static u32>>::bar

and it comes from an impl like impl<T> Foo<T> then the self-type the user gave doesn't directly map to the substitutions that the impl wants. To handle this, then, we have to preserve not just the "user-given substs" we used to do, but also a "user-given self-ty", which we have to apply later. This PR makes those changes.

It also removes the code from NLL relate-ops that handled canonical variables and moves to use normal inference variables instead. This simplifies a few things and gives us a bit more flexibility (for example, I predict we are going to have to start normalizing at some point, and it would be easy now).

r? @matthewjasper -- you were just touching this code, do you feel comfortable reviewing this?

Fixes #54574

rust-highfive · 2018-10-15T13:20:17Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:12] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:13] tidy error: /checkout/src/librustc_mir/hair/cx/expr.rs:725: line longer than 100 chars
[00:04:13] tidy error: /checkout/src/librustc/mir/visit.rs:217: line longer than 100 chars
[00:04:13] tidy error: /checkout/src/librustc/mir/visit.rs:786: line longer than 100 chars
[00:04:14] some tidy checks failed
[00:04:14] 
[00:04:14] 
[00:04:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:14] 
[00:04:14] 
[00:04:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:14] Build completed unsuccessfully in 0:00:46
[00:04:14] Build completed unsuccessfully in 0:00:46
[00:04:14] Makefile:79: recipe for target 'tidy' failed
[00:04:14] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1c2510b8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0c8fd2b9:start=1539609067031300881,finish=1539609067035337065,duration=4036184
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:200a90a8
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:02ceb97a
travis_time:start:02ceb97a
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0beccaec
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

src/librustc/ty/subst.rs

src/librustc/infer/nll_relate/mod.rs

bors · 2018-10-15T19:59:49Z

☔ The latest upstream changes (presumably #54858) made this pull request unmergeable. Please resolve the merge conflicts.

At some point, I had thought to use this code to handle equality comparisons for the `IfEq` verify bounds; at that point, we might not have had an infcx to talk about. But we wound up doing "SCC representatives" instead, so that's fine.

This used to be public, then I made it private in a previous PR, but there really wasn't a strong need for that.

We used to use a kind of "home-grown" variant where we tracked the value of each canonical variable.

The name is not great. Nor is the existence of this code great. It should be merged with the main "type relating code" at some point.

(long lines)

matthewjasper

LGTM. Not sure how confident I am reviewing a PR of this size though.

nikomatsakis · 2018-10-16T14:50:41Z

r? @pnkfelix

pnkfelix

If you end up rebasing this PR, you might as well move 24bee00 down next to e339e84 and then squash the former into the latter.

(But I haven't seen anything yet that would stop me from r+'ing this, I'm just noting it.)

pnkfelix

hooray for the new UserTypeAnnotation enum! I hope this will make the corresponding visit methods easier to grok

pnkfelix

Eeek regarding e7ed997; its a shame we couldn't continue keeping the contents of nll_relate hidden away under rustc_mir::borrow_check::nll ...

pnkfelix · 2018-10-16T20:44:26Z

src/librustc/ty/subst.rs

+    pub substs: &'tcx Substs<'tcx>,
+
+    /// The self-type, in the case of a `<T>::Item` path (when applied
+    /// to an inherent impl). See `UserSubsts` below.


I don't understand where the reader is meant to be led by the "See UserSubsts below."

(Probably a typo for UserSelfTy.)

yes, fixing

pnkfelix · 2018-10-16T20:53:00Z

src/test/ui/nll/user-annotations/method-ufcs-inherent-1.rs

+struct A<'a> { x: &'a u32 }
+
+impl<'a> A<'a> {
+    fn new<'b, T>(x: &'a u32, y: T) -> Self {


why is there a 'b here?

(I see the same thing occuring in the other tests. To be honest I had thought that we didn't allow unreferenced lifetimes in generic binders ... but I guess that is just a lint, when it comes to lifetimes...?)

copy and paste error, I think

pnkfelix · 2018-10-16T20:56:40Z

src/test/ui/nll/user-annotations/method-ufcs-inherent-1.rs

+
+fn foo<'a>() {
+    let v = 22;
+    let x = A::<'a>::new(&v, 22);


I think you and I mirrored each other on our respective PR's; there should be an //~ ERROR on this line, right?

Eep yes, will add.

pnkfelix · 2018-10-16T20:57:47Z

I had a slew of nits. But none of them should block landing this PR, and frankly, I want to rebase my own work pronto on top of this.

pnkfelix · 2018-10-16T20:58:43Z

@bors r+

bors · 2018-10-16T20:58:45Z

📌 Commit 7ce2e7a has been approved by pnkfelix

nikomatsakis · 2018-10-16T21:53:05Z

@bors p=1 -- lots of stuff wants to build on this

nikomatsakis · 2018-10-16T21:59:01Z

@pnkfelix

Eeek regarding e7ed997; its a shame we couldn't continue keeping the contents of nll_relate hidden away under rustc_mir::borrow_check::nll ...

Well, I was hoping they will get unified at some point, in which case it makes sense.

nikomatsakis · 2018-10-16T22:12:22Z

@bors r=pnkfelix p=1

bors · 2018-10-16T22:12:23Z

📌 Commit b70b4a6 has been approved by pnkfelix

bors · 2018-10-16T23:27:49Z

⌛ Testing commit b70b4a6 with merge 01ca85b...

@matthewjasper

…, r=pnkfelix nll type annotations in multisegment path This turned out to be sort of tricky. The problem is that if you have a path like ``` <Foo<&'static u32>>::bar ``` and it comes from an impl like `impl<T> Foo<T>` then the self-type the user gave doesn't *directly* map to the substitutions that the impl wants. To handle this, then, we have to preserve not just the "user-given substs" we used to do, but also a "user-given self-ty", which we have to apply later. This PR makes those changes. It also removes the code from NLL relate-ops that handled canonical variables and moves to use normal inference variables instead. This simplifies a few things and gives us a bit more flexibility (for example, I predict we are going to have to start normalizing at some point, and it would be easy now). r? @matthewjasper -- you were just touching this code, do you feel comfortable reviewing this? Fixes #54574

bors · 2018-10-17T01:59:21Z

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 01ca85b to master...

rust-highfive assigned matthewjasper Oct 15, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2018

nikomatsakis mentioned this pull request Oct 15, 2018

nll: respect user type annotations with constants in expressions #54571

Closed

tmandry reviewed Oct 15, 2018

View reviewed changes

src/librustc/ty/subst.rs Outdated Show resolved Hide resolved

src/librustc/infer/nll_relate/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

nikomatsakis force-pushed the nll-issue-54574-multisegment-path branch from d5eba13 to 63f5057 Compare October 15, 2018 16:14

nikomatsakis added 17 commits October 15, 2018 16:05

rename to user_substs_applied_to_def for consistency

4b69edc

add force_instantiate_unchecked method

24bee00

make TypeRelating take an infcx again

ca52427

At some point, I had thought to use this code to handle equality comparisons for the `IfEq` verify bounds; at that point, we might not have had an infcx to talk about. But we wound up doing "SCC representatives" instead, so that's fine.

make instantiate_canonical_with_fresh_inference_vars public again

a3409a1

This used to be public, then I made it private in a previous PR, but there really wasn't a strong need for that.

rework relate_type_and_user_type to use type inference variables

b6a080e

We used to use a kind of "home-grown" variant where we tracked the value of each canonical variable.

extract type_relate into the inference context as nll_relate

e7ed997

The name is not great. Nor is the existence of this code great. It should be merged with the main "type relating code" at some point.

move force_instantiate_unchecked to be local to nll_relate code

e339e84

introduce a UserTypeAnnotation enum

aed6e4a

introduce FnDef and AdtDef to UserTypeAnnotation

2a7fc22

introduce a more expressive UserSubsts

1443ac0

pull the common code across user-ty variants up top

0e1d362

handle user-self-type for def-ids

547182e

pass along user_self_ty

16a3824

avoid type variables in the self-type

ba20806

update tests

2e4e983

pacify the mercilous tidy

087d129

(long lines)

fix mir-opt test

7ce2e7a

nikomatsakis force-pushed the nll-issue-54574-multisegment-path branch from 049f1e6 to 7ce2e7a Compare October 15, 2018 20:25

matthewjasper approved these changes Oct 15, 2018

View reviewed changes

rust-highfive assigned pnkfelix and unassigned matthewjasper Oct 16, 2018

nikomatsakis mentioned this pull request Oct 16, 2018

user type annotations are captured post normalization #54940

Closed

pnkfelix reviewed Oct 16, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2018

fix comment

1008539

add ~ERROR annotations

b70b4a6

bors merged commit b70b4a6 into rust-lang:master Oct 17, 2018

This was referenced Oct 17, 2018

[WIP] Take 2: change how MIR represents Place #54426

Closed

[NLL] Propagate bounds from generators #55013

Merged

Replace ParamEnv with a new type in chalk context. #55040

Merged

tmandry added the WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 label May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nll type annotations in multisegment path #55093

nll type annotations in multisegment path #55093

nikomatsakis commented Oct 15, 2018 •

edited

Loading

rust-highfive commented Oct 15, 2018

This comment has been minimized.

bors commented Oct 15, 2018

matthewjasper left a comment

nikomatsakis commented Oct 16, 2018

pnkfelix left a comment

pnkfelix left a comment

pnkfelix left a comment

pnkfelix Oct 16, 2018

pnkfelix Oct 16, 2018

nikomatsakis Oct 16, 2018

pnkfelix Oct 16, 2018

pnkfelix Oct 16, 2018

nikomatsakis Oct 16, 2018

pnkfelix Oct 16, 2018

nikomatsakis Oct 16, 2018

pnkfelix commented Oct 16, 2018

pnkfelix commented Oct 16, 2018

bors commented Oct 16, 2018

nikomatsakis commented Oct 16, 2018

nikomatsakis commented Oct 16, 2018

nikomatsakis commented Oct 16, 2018

bors commented Oct 16, 2018

bors commented Oct 16, 2018

bors commented Oct 17, 2018

nll type annotations in multisegment path #55093

nll type annotations in multisegment path #55093

Conversation

nikomatsakis commented Oct 15, 2018 • edited Loading

rust-highfive commented Oct 15, 2018

This comment has been minimized.

bors commented Oct 15, 2018

matthewjasper left a comment

Choose a reason for hiding this comment

nikomatsakis commented Oct 16, 2018

pnkfelix left a comment

Choose a reason for hiding this comment

pnkfelix left a comment

Choose a reason for hiding this comment

pnkfelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Oct 16, 2018

pnkfelix commented Oct 16, 2018

bors commented Oct 16, 2018

nikomatsakis commented Oct 16, 2018

nikomatsakis commented Oct 16, 2018

nikomatsakis commented Oct 16, 2018

bors commented Oct 16, 2018

bors commented Oct 16, 2018

bors commented Oct 17, 2018

nikomatsakis commented Oct 15, 2018 •

edited

Loading