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

rustc: simpler ParameterEnvironment and free regions. #41914

Merged
merged 10 commits into from
May 13, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented May 11, 2017

The commits describe the slow transformation but the highlights are:

  • ReEarlyBound is considered free, with a scope based on the item that defined the lifetime parameter, and the root body of the RegionMaps in use, removing the need for free_substs
  • liberate_late_bound_regions and implicit_region_bound moved to typeck
  • CodeExtent not interned at all now - ideally it would be 2 u32 but it's small anyway

Future work building up on this could include:

  • ParameterEnvironment becoming just the result of predicates_of
    • interning makes my "parent chain" scheme unnecessary
  • implicit_region_bound could be retrieved from RegionMaps
  • renaming CodeExtent to Scope
    • generalizing "call site" to "use site" or something better to include constants
  • renaming RegionMaps to ScopeTree and its API to talk about "parents" explicitly

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

OK, this looks pretty nice. I left two small nits.

body
})
.unwrap_or_else(|| {
let root = tcx.hir.as_local_node_id(fr.scope).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't really follow what's going on here; maybe add a comment or two for what kinds of rust code falls into which case here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is explained on root_parent but do note this is not the final version of that function.
Let me know on the final functions where it seems unclear.

@@ -213,6 +215,7 @@ pub struct RegionVarBindings<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
// record the fact that `'a <= 'b` is implied by the fn signature,
// and then ignore the constraint when solving equations. This is
// a bit of a hack but seems to work.
early_bound_givens: RefCell<FxHashSet<(ty::EarlyBoundRegion, ty::RegionVid)>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe combine these by changing the type to RefCell<FxHashSet<(ty::Region<'tcx>, ty::RegionVid)>>, where the region is either an early-bound or free?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering if that would be more expensive but I doubt it now. Will do.

@nikomatsakis
Copy link
Contributor

I see that the librustc_driver unit tests are failing. I do hate those. I've been wanting to refactor them in such a way that we can encode the same scenarios in .rs files, but it's kind of difficult. I'm tempted to just tell you to delete them -- I'm not sure to what extent we have complete coverage of those scenarios elsewhere though (specifically some of the LUB tests).

@arielb1
Copy link
Contributor

arielb1 commented May 11, 2017

CodeExtent was interned as a u32 once upon a time. Not sure why that changed.

}
_ => fr.scope
};
assert_eq!(param_owner, fr.scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't you use fr.scope directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to have no regressions from lifetime parameters not defined on the scope leaking into e.g. the signature of a closure.

@nikomatsakis
Copy link
Contributor

@arielb1

CodeExtent was interned as a u32 once upon a time. Not sure why that changed.

I changed that to a pointer because there were various contexts that needed to be able to access the data without having enough context to get a region-maps (note that there is now more than one region-maps). But passing by value is probably even better. (Also, in the meantime, we've started to intern Regions, so I don't think that the size matters so much anymore anyway.)

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2017

let param_owner_id = tcx.hir.as_local_node_id(param_owner).unwrap();
let body_id = tcx.hir.maybe_body_owned_by(param_owner_id).unwrap_or_else(|| {
// The lifetime was defined on node that doesn't own a body,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice comment

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 12, 2017

📌 Commit 788e70e has been approved by nikomatsakis

bors added a commit that referenced this pull request May 12, 2017
@bors
Copy link
Contributor

bors commented May 13, 2017

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

@eddyb
Copy link
Member Author

eddyb commented May 13, 2017

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 13, 2017

📌 Commit 6da4123 has been approved by nikomatsakis

@eddyb
Copy link
Member Author

eddyb commented May 13, 2017

@bors p=1000 (separate for perf. I wish we had a guaranteed to keep PRs out of rollups)

@bors
Copy link
Contributor

bors commented May 13, 2017

⌛ Testing commit 6da4123 with merge 826d8f3...

bors added a commit that referenced this pull request May 13, 2017
rustc: simpler ParameterEnvironment and free regions.

The commits describe the slow transformation but the highlights are:
* `ReEarlyBound` is considered free, with a scope based on the item that defined the lifetime parameter, and the root body of the `RegionMaps` in use, removing the need for `free_substs`
* `liberate_late_bound_regions` and `implicit_region_bound` moved to typeck
* `CodeExtent` not interned at all now - ideally it would be 2 `u32` but it's small anyway

Future work building up on this could include:
* `ParameterEnvironment` becoming just the result of `predicates_of`
  * interning makes my "parent chain" scheme unnecessary
* `implicit_region_bound` could be retrieved from `RegionMaps`
* renaming `CodeExtent` to `Scope`
  * generalizing "call site" to "use site" or something better to include constants
* renaming `RegionMaps` to `ScopeTree` and its API to talk about "parents" explicitly
@bors
Copy link
Contributor

bors commented May 13, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants