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

Implement RFC 2056 trivial constraints in where clauses #48557

Merged
merged 6 commits into from
May 16, 2018

Conversation

matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented Feb 26, 2018

This is an implementation of the new behaviour for #48214. Tests are mostly updated to show the effects of this. Feature gate hasn't been added yet.

Some things that are worth noting and are maybe not want we want

  • &mut T: Copy doesn't allow as much as someone might expect because there is often an implicit reborrow.
  • There isn't a check that a where clause is well-formed any more, so where Vec<str>: Debug is now allowed (without a str: Sized bound).

r? @nikomatsakis

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2018
@bors
Copy link
Contributor

bors commented Feb 27, 2018

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

@bors bors 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 Feb 27, 2018
@cramertj
Copy link
Member

This is so cool! :)

@shepmaster
Copy link
Member

trivial constaints

Typo. You may want to double check any other places for the same one.

@matthewjasper matthewjasper changed the title [WIP] Implement RFC 2056 trivial constaints in where clauses [WIP] Implement RFC 2056 trivial constraints in where clauses Mar 4, 2018
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.

D'oh! I had this review with a few initial questions for some time but never submitted it! Sorry about that. I've been wondering why you didn't respond ;)

@@ -1492,7 +1492,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
let ty = self.resolve_type_vars_if_possible(&ty);
// Even if the type may have no inference variables, during
// type-checking closure types are in local tables only.
if !self.in_progress_tables.is_some() || !ty.has_closure_types() {
if !self.in_progress_tables.is_some() || !ty.has_closure_types() || param_env.is_global() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why you added this param_env.is_global() check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, looks like I added this before I put in the changes to ParamEnv::and from #48411. It doesn't appear to be needed now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's remove then =)

@@ -518,17 +518,8 @@ pub fn normalize_param_env_or_error<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

let predicates: Vec<_> =
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds.to_vec())
.filter(|p| !p.is_global()) // (*)
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can you clarify how you avoid the soundness problems discussed here =)

Is that related to the global check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't create any soundness problems after removing this, so I can't really be sure that I have avoided it. See the tests for an idea of what I attempted.

It looks like the current caching is (at the time of making this PR) very conservative, which probably explains this though.

fn can_use_global_caches(&self, param_env: ty::ParamEnv<'tcx>) -> bool {
// If there are any where-clauses in scope, then we always use
// a cache local to this particular scope. Otherwise, we
// switch to a global cache. We used to try and draw
// finer-grained distinctions, but that led to a serious of
// annoying and weird bugs like #22019 and #18290. This simple
// rule seems to be pretty clearly safe and also still retains
// a very high hit rate (~95% when compiling rustc).
if !param_env.caller_bounds.is_empty() {
return false;
}

@nikomatsakis nikomatsakis added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2018
&& !obligation.param_env.is_global() {
// If a param env has no global bounds, global obligations do not
// depend on its particular value in order to work, so we can clear
// out the param env and get better caching.
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I guess this is part of how we avoid the soundness problems too. I am trying to decide if this is_global check on the param environment suffices. I think it probably does given the current definitions of elaboration, but won't once we have a better definition of implied bounds (but that will require the newer solver, in which this PR is sort of easier to implement anyway).

I am imagining cases like this:

trait Foo where u32: Bar { }
trait Bar { }

Now, if we were to fully elaborate, T: Foo would imply u32: Bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I guess this is part of how we avoid the soundness problems too.

Yes, but this only seemed to cause bounds to not apply in my testing.

I am imagining cases like this:

trait Foo where u32: Bar { }
trait Bar { }

Now, if we were to fully elaborate, T: Foo would imply u32: Bar.

True, but this is surely just as much a problem with fully elaborating

trait Foo where Vec<T>: Copy { }
trait Bar { }

@@ -2074,7 +2071,7 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
Where(ty::Binder(Vec::new()))
}

ty::TyStr | ty::TySlice(_) | ty::TyDynamic(..) | ty::TyForeign(..) => Never,
ty::TyStr | ty::TySlice(_) | ty::TyDynamic(..) | ty::TyForeign(..) => None,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting. Should we remove Never from the enum? (Did you do so? If so, I missed it.)

ParamEnvAnd {
param_env: self,
value,
match self.reveal {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just landed a change much like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's where it's from. 😄

@nikomatsakis
Copy link
Contributor

@matthewjasper so -- much as this PR somehow makes me mildly nervous -- I can't think of a reason this won't work =) I suggest you rebase and remove the WIP comment from the header, so I can r+ ...

@nikomatsakis nikomatsakis 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 Mar 15, 2018
@matthewjasper matthewjasper force-pushed the allow-trvial-bounds branch 2 times, most recently from c03bacd to b080ee2 Compare March 15, 2018 21:55
@matthewjasper matthewjasper changed the title [WIP] Implement RFC 2056 trivial constraints in where clauses Implement RFC 2056 trivial constraints in where clauses Mar 15, 2018
@matthewjasper
Copy link
Contributor Author

rebased

A few further changes:

  • wfcheck changes reverted, it appears these aren't needed to allow inconsistent bonds and any further changes are probably be beyond the scope of the RFC.
  • The trivial_bounds lint from the RFC is now implemented. I don't really like the name, so suggestions are welcome.
  • No feature gate, I'm not sure really how best to add one with these changes.

@nikomatsakis Done, but this still needs a feature gate, right?

@nikomatsakis
Copy link
Contributor

@matthewjasper

Done, but this still needs a feature gate, right?

Um, yes, right. I was.. uh.. just testing you to see if you'd remember.

@nikomatsakis
Copy link
Contributor

@matthewjasper let me know when you add feature gate then =)

@matthewjasper matthewjasper force-pushed the allow-trvial-bounds branch 2 times, most recently from 760cec8 to fcfbfe0 Compare March 21, 2018 22:35
@matthewjasper
Copy link
Contributor Author

@nikomatsakis Feature gate now added.
The feature gate is added as another check in wfcheck so that the feature gate errors can have a custom note.

I haven't feature-gated built-in trait bound errors (e.g. where [T]: Sized), because they are a strange exception, and Clone only recently became a built-in trait.

The commits are now a bit of a mess, but I can clean them up later.

@bors
Copy link
Contributor

bors commented Mar 22, 2018

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

@nikomatsakis
Copy link
Contributor

@matthewjasper thanks -- sorry for being slow, will re-review

@@ -683,8 +683,8 @@ fn check_false_global_bounds<'a, 'gcx, 'tcx>(
let implied_obligations = traits::elaborate_predicates(fcx.tcx, predicates);

for pred in implied_obligations {
// HAS_LOCAL_NAMES is used to match the existing behvaiour.
if !pred.has_type_flags(ty::TypeFlags::HAS_LOCAL_NAMES) {
// Match the existing behvaior.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: behavoir

Copy link
Member

@shepmaster shepmaster May 15, 2018

Choose a reason for hiding this comment

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

Nit: behavior

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.

r=me upon rebase

@matthewjasper matthewjasper force-pushed the allow-trvial-bounds branch 2 times, most recently from 40eb071 to ad37f44 Compare May 15, 2018 20:48
@matthewjasper
Copy link
Contributor Author

rebased.
And another soundness issue raised for good measure.

@cramertj
Copy link
Member

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 15, 2018

📌 Commit be2900c 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 May 15, 2018
@bors
Copy link
Contributor

bors commented May 16, 2018

⌛ Testing commit be2900c with merge 448cc57...

bors added a commit that referenced this pull request May 16, 2018
Implement RFC 2056 trivial constraints in where clauses

This is an implementation of the new behaviour for #48214. Tests are mostly updated to show the effects of this. Feature gate hasn't been added yet.

Some things that are worth noting and are maybe not want we want

* `&mut T: Copy` doesn't allow as much as someone might expect because there is often an implicit reborrow.
* ~There isn't a check that a where clause is well-formed any more, so `where Vec<str>: Debug` is now allowed (without a `str: Sized` bound).~

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented May 16, 2018

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

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants