Skip to content

move resolve_lifetimes into a proper query - #46657

Merged
bors merged 3 commits into
rust-lang:masterfrom
nikomatsakis:resolve-lifetimes-query
Dec 12, 2017
Merged

move resolve_lifetimes into a proper query#46657
bors merged 3 commits into
rust-lang:masterfrom
nikomatsakis:resolve-lifetimes-query

Conversation

@nikomatsakis

@nikomatsakisnikomatsakis commented Dec 11, 2017

Copy link
Copy Markdown
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
nikomatsakisforce-pushed the resolve-lifetimes-query branch 2 times, most recently from 2d9214f to 15fda6aCompareDecember 11, 2017 14:13
@nikomatsakis

nikomatsakis commented Dec 11, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@nikomatsakisnikomatsakis 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

gaurikholkar-zz commented Dec 11, 2017•
edited
Loading

Copy link
Copy Markdown

Thanks @nikomatsakis

@kennytmkennytm 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
Comment threadsrc/librustc/middle/resolve_lifetime.rs Outdated
) -> Rc<ResolveLifetimes> {
assert_eq!(for_krate, LOCAL_CRATE);

let named_region_map = krate(tcx).unwrap_or_default();

@arielb1arielb1Dec 11, 2017•
edited
Loading

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.

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
Comment threadsrc/librustc/middle/resolve_lifetime.rs Outdated
})
}

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

@arielb1arielb1Dec 11, 2017

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.

Can this function actually return ErrorReported now?

@nikomatsakisnikomatsakisDec 11, 2017

Copy link
Copy Markdown
ContributorAuthor

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

arielb1 commented Dec 11, 2017

Copy link
Copy Markdown
Contributor

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

arielb1 commented Dec 11, 2017

Copy link
Copy Markdown
Contributor

r=me with that cleaned up.

@kennytmkennytm 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

nikomatsakis commented Dec 11, 2017

Copy link
Copy Markdown
ContributorAuthor

@bors r=arielb1

@bors

bors commented Dec 11, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit fdbd9b0 has been approved by arielb1

@bors

bors commented Dec 12, 2017

Copy link
Copy Markdown
Collaborator

⌛ 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

bors commented Dec 12, 2017

Copy link
Copy Markdown
Collaborator

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

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

Reviewers

1 more reviewer
@arielb1arielb1arielb1 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@michaelwoeristermichaelwoerister

Labels

S-waiting-on-authorStatus: 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