Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for NLL: unexpected "free region `` does not outlive" error #52793

Merged
merged 1 commit into from Aug 1, 2018

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link
Member

davidtwco commented Jul 27, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jul 31, 2018

@davidtwco Since the test uses #[rustc_regions], which is definitely very specific to the internals of NLL, I think the test should live underneath src/test/ui/nll/closure-requirements/.

(that is, I don't want someone who's idly skimming src/test/ui to run into something like this.)

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jul 31, 2018

Actually ... why are you using #[rustc_regions] ?

My interpretation of our request for a test is that we want one that reflects the end user's experience.

We don't need to be exploring the internals of the region requirements here.

@davidtwco davidtwco force-pushed the davidtwco:issue-49824 branch from 1aaa14f to 4488d55 Jul 31, 2018

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jul 31, 2018

@pnkfelix I've updated the test to remove #![rustc_regions].

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 1, 2018

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit 4488d55 has been approved by pnkfelix

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 1, 2018

@bors r-

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 1, 2018

@davidtwco so you can tell that I changed my mind about something here.

This test represents a case where NLL could break old code that was accepted by the AST-borrowck.

Its exactly the kind of case where I think the right approach is not to use #![feature(nll)], but instead to make the test encode both of the behaviors in the distinct borrow-checkers, so that we can immediately see (when we compare the diffs) that this is a scenario where the move to NLL is going to break people's code.

At the very least, I would remove the #![feature(nll)] from the test, and add #[rustc_error] to the fn main. This will force the compiler to error out even when the test is accepted under the AST-borrowck.

@davidtwco davidtwco force-pushed the davidtwco:issue-49824 branch from 4488d55 to c5553e1 Aug 1, 2018

// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(rustc_attrs)]

This comment has been minimized.

@pnkfelix

pnkfelix Aug 1, 2018

Member

please add a comment noting that this test is checking a problem that only arises in NLL mode, and maybe a pointer to the issue-49824.nll.stderr file, just so someone who looks over these tests in the futures doesn't think that this test is a no-op.

This comment has been minimized.

@davidtwco

davidtwco Aug 1, 2018

Author Member

Fixed.

@davidtwco davidtwco force-pushed the davidtwco:issue-49824 branch from c5553e1 to 8bbf042 Aug 1, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Aug 1, 2018

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit 8bbf042 has been approved by pnkfelix

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 1, 2018

Rollup merge of rust-lang#52793 - davidtwco:issue-49824, r=pnkfelix
Add test for NLL: unexpected "free region `` does not outlive" error

Fixes rust-lang#49824.

r? @pnkfelix @nikomatsakis

bors added a commit that referenced this pull request Aug 1, 2018

Auto merge of #52958 - pietroalbini:rollup, r=pietroalbini
Rollup of 15 pull requests

Successful merges:

 - #52793 (Add test for NLL: unexpected "free region `` does not outlive" error )
 - #52799 (Use BitVector for global sets of AttrId)
 - #52809 (Add test for unexpected region for local data ReStatic)
 - #52834 ([NLL] Allow conflicting borrows of promoted length zero arrays)
 - #52835 (Fix Alias intra doc ICE)
 - #52854 (fix memrchr in miri)
 - #52899 (tests/ui: Add missing mips{64} ignores)
 - #52908 (Use SetLenOnDrop in Vec::truncate())
 - #52915 (Don't count MIR locals as borrowed after StorageDead when finding locals live across a yield terminator)
 - #52926 (rustc: Trim down the `rust_2018_idioms` lint group)
 - #52930 (rustc_resolve: record single-segment extern crate import resolutions.)
 - #52939 (Make io::Read::read_to_end consider io::Take::limit)
 - #52942 (Another SmallVec.extend optimization)
 - #52947 (1.27 actually added the `armv5te-unknown-linux-musleabi` target)
 - #52954 (async can begin expressions)

Failed merges:

r? @ghost

@bors bors merged commit 8bbf042 into rust-lang:master Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidtwco davidtwco deleted the davidtwco:issue-49824 branch Aug 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.