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 arbitrary_self_types #45870

Merged
merged 26 commits into from
Nov 12, 2017
Merged

Conversation

mikeyhew
Copy link
Contributor

@mikeyhew mikeyhew commented Nov 8, 2017

r? @arielb1
cc @nikomatsakis

Partial implementation of #44874. Supports trait and struct methods with arbitrary self types, as long as the type derefs (transitively) to Self. Doesn't support raw-pointer self yet.

Methods with non-standard self types (i.e. anything other than &self, &mut self, and Box<Self>) are not object safe, because dynamic dispatch hasn't been implemented for them yet.

I believe this is also a (partial) fix for #27941.

If the feature is enabled, allow method `self` types to be any type
that auto-derefs to `self`.
- Currently, this supports inherent methods as well as trait methods.
The plan AFAIK is to only allow this for trait methods, so I guess it
won’t stay this way
- Dynamic dispatch isn’t implemented yet, so the compiler will ICE if
you define a trait method that takes `self: Rc<Self>` and try to call
it on an `Rc<Trait>`. I will probably just make those methods
non-object-safe initially.
…error

Rewrote ExplicitSelf, adding a new `Other` variant for arbitrary self
types. It’s a bit more sophisticated now, and checks for type equality,
so you have to pass the type context and param env as arguments.
There’s a borrow-checker error here that I have to fix

Rewrote check_method_receiver, so it acts as if arbitrary self types
are allowed, and then checks for ExplicitSelf::Other at the end and
disallows it unless the feature is present.
Had to take the infer context as a parameter instead of the type
context, so that the function can be called during inference
- removed the inherent impls compile-fail test, because we’ll be
supporting them
- remove E0308-2 because it’s gonna be supported now (behind a feature
gate)
- replaced the mismatched method receiver error message with something
better, so fixed the tests that that broke
- added some old code that used ExplicitSelf::determine to check for eqtype with the expected self type in the simple cases
- this fixes problems with error messages being worse in those cases, which caused some compile-fail tests to fail
I was only doing it for self_arg_ty, and ended up causing
run-pass/associated-types-projection-from-known-type-in-impl.rs to fail.
…origin

I doubt this changes anything, I was just trying to fix an issue with
error messages and ended up doing this along with other things.
Committing it separately so I can undo it easily.
… error messages that I don't understand why they changed, so the tests pass
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 8, 2017
})

let cause = fcx.cause(span, ObligationCauseCode::MethodReceiver);
let eq = |expected, actual| fcx.at(&cause, fcx.param_env).eq(expected, actual);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to wrap this inside a commit_if_ok, because IIRC just an eq isn't transactional.

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 don't know what you mean by transactional, but I'll try it; maybe it will help with the error messages problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arielb1 on second thought, I had another approach here. If I did that instead, would it address your concern? (i.e. is demand_eqtype_with_origin "transactional"?)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikeyhew

Sorry forgot to detail - the problem is that eq can have side-effects even if it fails (e.g. if you equate (_, u32) with (i32, i32), it will make the _ be i32 before erroring out).

(i.e. is demand_eqtype_with_origin "transactional"?)

Nope. You want fcx.commit_if_ok(|_| fcx.at(&cause, fcx.param_env).eq(expected, actual)), which creates a transaction and rolls it back if the equality fails.

@@ -1,3 +1,5 @@
//~ ERROR mismatched types
Copy link
Contributor

@arielb1 arielb1 Nov 8, 2017

Choose a reason for hiding this comment

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

This doesn't make sense how are you getting mismatched type errors on this line you passed DUMMY_SP to a type error function, please use a correct span so that the error will appear in the correect line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya so this is the thing I was having trouble with on Gitter. In e06cd31#diff-15d40489387405cc3e23e2f9f66a74c2L519 I removed some error checks that I (still think) are redundant, and it caused the error messages to go wonky. I can't figure out why

put the error message on one line so the test suite does not think it is two errors
use a substring of the error message so it fits in 100 chars for tidy
@mikeyhew mikeyhew force-pushed the arbitrary_self_types branch from 03b3294 to 7f8b003 Compare November 8, 2017 21:46
@arielb1
Copy link
Contributor

arielb1 commented Nov 8, 2017

The error wonkyness is caused by wonky constraint handling in region_inference - that function allows the cause of constraints from a snapshot to overwrite a cause from out of it:

fn add_constraint(&self, constraint: Constraint<'tcx>, origin: SubregionOrigin<'tcx>) {
// cannot add constraints once regions are resolved
assert!(self.values_are_none());
debug!("RegionVarBindings: add_constraint({:?})", constraint);
if self.constraints.borrow_mut().insert(constraint, origin).is_none() {
if self.in_snapshot() {
self.undo_log.borrow_mut().push(AddConstraint(constraint));
}
}
}

I think that converting this HashMap to keep the first cause instead of the last one should avoid problems like this.

now that we've fixed the bug where constraint origins were getting overwritten, the good error messages are back (with some tweaks)
@mikeyhew
Copy link
Contributor Author

mikeyhew commented Nov 9, 2017

@arielb1 I fixed the bug, and the good error messages are back. There's a slight difference because instead of comparing &Self with &Self, we're comparing Self with Self.

There was also one ui test whose error message changed, presumably because of the bugfix (constraint origins are no longer being overwritten, even when we're outside of a snapshot).


error: aborting due to previous error
error: arbitrary `self` types are unstable (see issue #44874)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you not have this error if you already emitted an "invalid self type" error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it emits both errors. I can change it though, wasn't sure whether to emit both or just the "invalid self type" error

Copy link
Contributor

@arielb1 arielb1 Nov 9, 2017

Choose a reason for hiding this comment

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

You should change it to just emit the "invalid self type" error - you don't want to emit unnecessary feature gate errors because they are confusing.

@arielb1
Copy link
Contributor

arielb1 commented Nov 9, 2017

This is LGTM, just:

  1. change the code to not emit a feature gate error if the self type is truly invalid - spurious feature gate errors are confusing to end users.
  2. add a run-pass test that tests that both trait and inherent methods with generalized self work.

r=me with these fixed

@mikeyhew
Copy link
Contributor Author

mikeyhew commented Nov 9, 2017

@arielb1 should be good to go once the CI finishes running

@arielb1
Copy link
Contributor

arielb1 commented Nov 9, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2017

📌 Commit 77cd993 has been approved by arielb1

@kennytm kennytm 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 Nov 10, 2017
@bors
Copy link
Contributor

bors commented Nov 12, 2017

⌛ Testing commit 77cd993 with merge b087ded...

bors added a commit that referenced this pull request Nov 12, 2017
Implement arbitrary_self_types

r? @arielb1
cc @nikomatsakis

Partial implementation of #44874.  Supports trait and struct methods with arbitrary self types, as long as the type derefs (transitively) to `Self`. Doesn't support raw-pointer `self` yet.

Methods with non-standard self types (i.e. anything other than `&self, &mut self, and Box<Self>`) are not object safe, because dynamic dispatch hasn't been implemented for them yet.

I believe this is also a (partial) fix for #27941.
@bors
Copy link
Contributor

bors commented Nov 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing b087ded 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-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants