Skip to content

[NLL] Returns are interesting for free regions - #53175

Merged
bors merged 7 commits into
rust-lang:masterfrom
matthewjasper:more-return-stuff
Aug 18, 2018
Merged

[NLL] Returns are interesting for free regions#53175
bors merged 7 commits into
rust-lang:masterfrom
matthewjasper:more-return-stuff

Conversation

@matthewjasper

@matthewjasper matthewjasper commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

Based on #53088 - creating now to get feedback.

Closes #51175

  • Make assigning to the return type interesting.
  • Use "returning this value" instead of "return" in error messages.
  • Prefer one of the explanations that we have a name for to a generic interesting cause in some cases.
  • Treat causes that involve the destination of a call like assignments.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2018
@matthewjasper

Copy link
Copy Markdown
Contributor Author

r? @nikomatsakis

@nikomatsakis nikomatsakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this, apart from wanting a comment. The results seem great.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread src/test/ui/issue-15034.nll.stderr Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem less good but still "tolerable" — I think with a bit more thought we should be able to narrow back down to the lexer expression. The problem is that we still have trouble distinguishing "intermediate, passthrough" regions (like the one on Parser) from the "source regions" (the ones on lexer). That's ok for now though I think.

Comment thread src/test/ui/issue-40510-1.nll.stderr Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear win. Nice!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this reads, though it's less "concise", which I think some people value. Still, seems ok to me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a comment here... something lke:


Classify calls differently depending on whether or not the sub region appears in the return type. If the return type contains the sub-region, then this is either an assignment or a return, depending on whether we are writing to the RETURN_PLACE or not. The idea here is that the region is being propagated from an input into the output place, so it's a kind of assignment. Otherwise, if the sub-region only appears in the argument types, then use the CallArgument classification.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: wild indentation

@matthewjasper

matthewjasper commented Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

Rebased, some tests were changed in the mean time. Added another commit where we had lifetimes in some messages the wrong way around.

@rust-highfive

This comment has been minimized.

@nikomatsakis

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Aug 9, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 49cf4e1e49e8ef5527148fa663d47ace4a30f89c has been approved by nikomatsakis

@bors 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 Aug 9, 2018
@matthewjasper

Copy link
Copy Markdown
Contributor Author

Rebased and tests updated

@nikomatsakis

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Aug 16, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit a19db49 has been approved by nikomatsakis

@bors

bors commented Aug 17, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a19db49 with merge 5cbed2c8cc23b11aee0543296770e77137070580...

@bors

bors commented Aug 17, 2018

Copy link
Copy Markdown
Collaborator

💔 Test failed - status-travis

@bors

bors commented Aug 17, 2018

Copy link
Copy Markdown
Collaborator

💔 Test failed - status-travis

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

Copy link
Copy Markdown
Contributor

The job dist-arm-linux 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.
travis_time:end:0552a9fe:start=1534490371244013800,finish=1534490371253802880,duration=9789080
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:04409332
$ 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 -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:1287bcaa
travis_time:start:1287bcaa
$ 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:03b37b1e
$ 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)

@kennytm

kennytm commented Aug 17, 2018

Copy link
Copy Markdown
Member

@bors retry travis-ci/travis-ci#9696

@bors 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 Aug 17, 2018
@bors

bors commented Aug 17, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a19db49 with merge 3087520eac999cbb0b528b0985381a0b5b33e80c...

@bors

bors commented Aug 17, 2018

Copy link
Copy Markdown
Collaborator

💔 Test failed - status-travis

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

Copy link
Copy Markdown
Contributor

The job x86_64-gnu-aux 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:52:37] [RUSTC-TIMING] tempfile test:false 1.138
[00:52:38] [RUSTC-TIMING] pulldown_cmark test:false 8.429
[00:52:38]    Compiling rustdoc v0.0.0 (file:///checkout/src/librustdoc)

Broadcast message from root@travis-job-8b0b114a-6c82-4d24-91e8-2e146428c76d
 (unknown) at 10:38 ...
The system is going down for power off NOW!
[00:53:55] 
[00:53:55] Session terminated, terminating shell... ...terminated.
[00:53:55] Makefile:28: recipe for target 'all' failed
[00:53:55] make: *** [all] Terminated

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 143.
travis_time:start:0469b5e3
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@kennytm

kennytm commented Aug 17, 2018

Copy link
Copy Markdown
Member

@bors retry travis-ci/travis-ci#4924

@bors 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 Aug 17, 2018
@bors

bors commented Aug 17, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a19db49 with merge 5c194343d069ce4839b85c1208b434e48afe4146...

@bors

bors commented Aug 17, 2018

Copy link
Copy Markdown
Collaborator

💔 Test failed - status-travis

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

Copy link
Copy Markdown
Contributor

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:37:57] [TIMING] Rustdoc { host: "x86_64-unknown-linux-gnu" } -- 86.604
[00:37:57] Build completed successfully in 0:32:38
[00:37:57]     Finished dev [unoptimized] target(s) in 0.25s
[00:37:58] thread 'main' panicked at 'fs::read_dir(builder.src.join("src/doc/book/redirects")) failed with No such file or directory (os error 2)', bootstrap/doc.rs:294:21
[00:37:58] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap doc
[00:37:58] Build completed unsuccessfully in 0:00:01
[00:37:58] Makefile:28: recipe for target 'all' failed
[00:37:58] Makefile:28: recipe for target 'all' failed
[00:37:58] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0982019a
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:217c58a0:start=1534515331016287719,finish=1534515331023178740,duration=6891021
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:03596451
$ 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 -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:063a9c78
travis_time:start:063a9c78
$ 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:05bd3d6c
$ 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)

@kennytm

kennytm commented Aug 18, 2018

Copy link
Copy Markdown
Member

@bors retry

[00:01:23] Submodule path 'src/tools/rustfmt': checked out '5c9a2b6c13d3b6f8d3f9c02b130bb4b54fd489fb'
[00:02:00] 
[00:02:00] gzip: stdin: not in gzip format
[00:02:00] tar: Child returned status 1
[00:02:00] tar: Error is not recoverable: exiting now

@bors 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 Aug 18, 2018
@bors

bors commented Aug 18, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit a19db49 with merge 7de3dea...

bors added a commit that referenced this pull request Aug 18, 2018
[NLL] Returns are interesting for free regions

Based on #53088 - creating now to get feedback.

Closes #51175

* Make assigning to the return type interesting.
* Use "returning this value" instead of "return" in error messages.
* Prefer one of the explanations that we have a name for to a generic interesting cause in some cases.
* Treat causes that involve the destination of a call like assignments.
@bors

bors commented Aug 18, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 7de3dea to master...

@bors
bors merged commit a19db49 into rust-lang:master Aug 18, 2018
@matthewjasper
matthewjasper deleted the more-return-stuff branch August 18, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants