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.

Sign up for GitHub

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

Jump to bottom

move resolve_lifetimes into a proper query #46657

Merged
merged 3 commits into from
Dec 12, 2017
Merged

move resolve_lifetimes into a proper query #46657

merged 3 commits into from
Dec 12, 2017

Conversation

nikomatsakis
Copy link
Contributor

Now that we made resolve_lifetimes into a query, elision errors no
longer abort compilation, which affects some tests.

Also, remove dep_graph_crosscontaminate_tables -- there is no a path in
the dep-graph, though red-green handles it. The same scenario
is (correctly) tested by issue-42602.rs in any case.

r? @michaelwoerister

@nikomatsakis nikomatsakis force-pushed the resolve-lifetimes-query branch 2 times, most recently from 2d9214f to 15fda6a Compare December 11, 2017 14:13
@nikomatsakis
Copy link
Contributor Author

cc @gaurikholkar -- this is the PR I was talking about

@nikomatsakis nikomatsakis mentioned this pull request Dec 11, 2017
Merged
2 tasks
@nikomatsakis
move resolve_lifetimes into a proper query
b7794c0 
Now that we made `resolve_lifetimes` into a query, elision errors no
longer abort compilation, which affects some tests.

Also, remove `dep_graph_crosscontaminate_tables` -- there is no a path in
the dep-graph, though red-green handles it. The same scenario
is (correctly) tested by issue-42602.rs in any case.
@gaurikholkar-zz
Copy link

gaurikholkar-zz commented Dec 11, 2017 •
edited
Loading

Thanks @nikomatsakis

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2017
arielb1
arielb1 reviewed Dec 11, 2017
View reviewed changes
src/librustc/middle/resolve_lifetime.rs Outdated
) -> Rc<ResolveLifetimes> {
assert_eq!(for_krate, LOCAL_CRATE);

let named_region_map = krate(tcx).unwrap_or_default();
Copy link
Contributor

@arielb1 arielb1 Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This looks a bit violent and likely to cause cascade errors - if resolving lifetimes for one item fails, it will destroy all lifetime information without aborting compilation.

On a second reading, krate can't actually return an error and trigger this misbehavior.

arielb1
arielb1 reviewed Dec 11, 2017
View reviewed changes
src/librustc/middle/resolve_lifetime.rs Outdated
})
}

fn krate<'tcx>(tcx: TyCtxt<'_, 'tcx, 'tcx>) -> Result<NamedRegionMap, ErrorReported> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function actually return ErrorReported now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I should fix that. A holdover from older code.

@arielb1
Copy link
Contributor

arielb1 commented Dec 11, 2017

If fn krate could return an error, I suspect this will cause an error cascade if you add a broken lifetime to a big crate, because you'll have an empty lifetime map for every item, even non-erroneous items.

OTOH, fn krate doesn't appear to be returning errors, which means that can't actually happen. I would make it not return a Result.

With that done, and because I don't think fn krate is a good name, I would either find a better name for it or merge it into resolve_lifetimes - it's just calling a visitor.

@arielb1
Copy link
Contributor

arielb1 commented Dec 11, 2017

r=me with that cleaned up.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2017
@nikomatsakis
Copy link
Contributor Author

@bors r=arielb1

@bors
Copy link
Contributor

bors commented Dec 11, 2017

📌 Commit fdbd9b0 has been approved by arielb1

@bors
Copy link
Contributor

bors commented Dec 12, 2017

⌛ Testing commit fdbd9b0 with merge 5951f8d...

bors added a commit that referenced this pull request Dec 12, 2017
@bors
Auto merge of #46657 - nikomatsakis:resolve-lifetimes-query, r=arielb1
5951f8d 
move `resolve_lifetimes` into a proper query

Now that we made `resolve_lifetimes` into a query, elision errors no
longer abort compilation, which affects some tests.

Also, remove `dep_graph_crosscontaminate_tables` -- there is no a path in
the dep-graph, though red-green handles it. The same scenario
is (correctly) tested by issue-42602.rs in any case.

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Dec 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 5951f8d to master...

@bors bors merged commit fdbd9b0 into rust-lang:master Dec 12, 2017
This was referenced Dec 12, 2017
Merged
Closed
Closed
Merged
Merged
@aidanhs aidanhs mentioned this pull request Dec 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arielb1 arielb1 arielb1 left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nikomatsakis @gaurikholkar-zz @arielb1 @bors @kennytm @michaelwoerister