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

Operator dispatch #18486

Merged
merged 6 commits into from
Nov 6, 2014
Merged

Operator dispatch #18486

merged 6 commits into from
Nov 6, 2014

Conversation

nikomatsakis
Copy link
Contributor

This branch cleans up overloaded operator resolution so that it is strictly based on the traits in ops, rather than going through the normal method lookup mechanism. It also adds full support for autoderef to overloaded index (whereas before autoderef only worked for non-overloaded index) as well as for the slicing operators.

This is a [breaking-change]: in the past, we were accepting combinations of operands that were not intended to be accepted. For example, it was possible to compare a fixed-length array and a slice, or apply the ! operator to a &int. See the first two commits in this pull-request for examples.

One downside of this change is that comparing fixed-length arrays doesn't always work as smoothly as it did before. Before this, comparisons sometimes worked due to various coercions to slices. I've added impls for Eq, Ord, etc for fixed-lengths arrays up to and including length 32, but if the array is longer than that you'll need to either newtype the array or convert to slices. Note that this plays better with deriving in any case than the previous scheme.

Fixes #4920.
Fixes #16821.
Fixes #15757.

cc @alexcrichton
cc @aturon

@nikomatsakis nikomatsakis force-pushed the operator-dispatch branch 2 times, most recently from 727f5ed to 4babfe8 Compare October 31, 2014 11:09
@nikomatsakis
Copy link
Contributor Author

It occurs to me that we could have a special rule for operators that eagerly coerces fixed-length arrays into slices, much as we eagerly reborrow pointers. But I'm not super enthusiastic about it: special rules are annoying, it won't work super reliably (e.g., not for Rc<[int, ..n]> and Rc<[int]>) and it doesn't help with the deriving problem etc. Still, might be worth doing.

@nikomatsakis
Copy link
Contributor Author

(Oh, one other thing. If we make Eq and friends take multiple type parameters (as might be nice...) then we could easily define impls that permit "cross-comparison" between fixed-length arrays and slices.)

@japaric
Copy link
Member

japaric commented Oct 31, 2014

It occurs to me that we could have a special rule for operators that eagerly coerces fixed-length arrays into slices

If we add this special rule, and change #[deriving(PartialEq)] to use operators instead of method calls (as I have done in #18457) then we would be able to use #[deriving(PartialEq, PartialOrd)] on structs that contain fixed size arrays of any size. (It still wouldn't work with fields like Box<[T, ..N]>, Rc<[T, ..N]>, but those types seem not useful to me, do they even come up in practice?)

(Oh, one other thing. If we make Eq and friends take multiple type parameters (as might be nice...) then we could easily define impls that permit "cross-comparison" between fixed-length arrays and slices.)

Wouldn't the last part require being able to use integers as generic parameters? As in fn eq(&self, rhs: &[T, ..N]) -> bool where Self = &[T]. Or you mean manually implementing all the cross comparisons up to N = 32?

@alexcrichton
Copy link
Member

Can't wait to give this a go, thanks @nikomatsakis!

@nikomatsakis
Copy link
Contributor Author

@japaric yes, it would suffer from the same limitations about length.

Yes so perhaps we should "eagerly-unsize" fixed-length arrays used with operators. It handles arbitrary sizes and is more ergonomic. Deriving could be fixed either by using operators or by just modifying deriving to understand that (for things like Eq etc), if something is a fixed-length array, it ought to first convert to a slice. Hmm. As a bonus, I think it's not very hard to do.

@nikomatsakis
Copy link
Contributor Author

One possible downside I can see is that a == b where a and b are both fixed-length arrays may not be compiled as efficiently, because it will convert to a loop that llvm has to unroll. OTOH, LLVM can do that, at least in principle, and anyhow currently I implemented the traits just by doing a[] == b[], so clearly that's no different.

@nikomatsakis
Copy link
Contributor Author

(Note though that this efficiency argument could be side-stepped by not using operator form)

@nikomatsakis
Copy link
Contributor Author

@japaric ok, I prototyped the [T, ..n] => [T] coercion but without Eq implementations for [T] it winds up making less code compile. So I will land as is I think and then bring it in as a second PR once you land your new Eq implementations. It's a pretty small patch (about 5 lines or so).

@SiegeLord
Copy link
Contributor

Does this change in semantics completely preclude future implementation of the ad-hoc operator overloading (rust-lang/rfcs#399)?

@nikomatsakis
Copy link
Contributor Author

@SiegeLord this is not a change in semantics, it's just tightening up the semantics we have. It doesn't preclude going to ad-hoc operator overloading in particular, though obviously that would require further changes.

@nikomatsakis nikomatsakis force-pushed the operator-dispatch branch 3 times, most recently from 060398a to d1bf6c5 Compare November 1, 2014 09:55
@japaric
Copy link
Member

japaric commented Nov 2, 2014

@nikomatsakis How does this PR affects issues #8280 and #10337?

@nikomatsakis
Copy link
Contributor Author

@japaric I did not fix #8280 but #10337 is really a dup of #4920 I think. Certainly #10337 describes what I did in this PR for the most part; it also describes how we could possibly fix #8280 if we wanted to.

* to all lengths.
*/

#![doc(primitive = "tuple")]
Copy link
Member

Choose a reason for hiding this comment

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

I’m guessing “tuple” is a mistake here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. although as this is a private module it hardly matters...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I have no idea what that annotation does :) anyway, correction in commit ec67675

Copy link
Member

Choose a reason for hiding this comment

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

@nikomatsakis This marks the documentation for the primitive types as they are handled in links and things like that, e.g. for tuple and slice.

@sinistersnare
Copy link
Contributor

so does this fix #16821 / #15757?

*/

#![stable]
#![experimental] // not yet reviewed
Copy link
Member

Choose a reason for hiding this comment

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

So now it’s stable and experimental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Um... too late to be making edits, I guess.

@nikomatsakis
Copy link
Contributor Author

@sinistersnare yes it will fix those issues.

@@ -1980,7 +1980,7 @@ mod tests {
let (left, right) = values.split_at_mut(2);
{
let left: &[_] = left;
assert!(left[0..left.len()] == [1, 2]);
assert!(left[0..left.len()] == [1, 2][]);
Copy link
Member

Choose a reason for hiding this comment

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

Can't [1, 2][] be written just as &[1, 2]?

…ths 0...32 and repair various cases where slices and fixed-length arrays were being compared.
Key points are:
1. `a + b` maps directly to `Add<A,B>`, where `A` and `B` are the types of `a` and `b`.
2. Indexing and slicing autoderefs consistently.
@nikomatsakis
Copy link
Contributor Author

@alexcrichton is that a known failure?

@alexcrichton
Copy link
Member

Sadly not yet, that's the first time I've ever seen that :(

@nikomatsakis
Copy link
Contributor Author

@alexcrichton it could be the fault of this PR, plausibly, though make check passes locally (linux)

@alexcrichton
Copy link
Member

The error code listed corresponds to 0xc0000005 and googling for that points to it being some form of access violation, so we may have just uncovered a bug in gcc or ld (who knows!).

@nikomatsakis
Copy link
Contributor Author

@alexcrichton let's see if it repeats. not obvious to me how what I did would be at fault, but then I did change operator resolution, so in principle it COULD affect things.

bors added a commit that referenced this pull request Nov 5, 2014
This branch cleans up overloaded operator resolution so that it is strictly based on the traits in `ops`, rather than going through the normal method lookup mechanism. It also adds full support for autoderef to overloaded index (whereas before autoderef only worked for non-overloaded index) as well as for the slicing operators.

This is a [breaking-change]: in the past, we were accepting combinations of operands that were not intended to be accepted. For example, it was possible to compare a fixed-length array and a slice, or apply the `!` operator to a `&int`. See the first two commits in this pull-request for examples.

One downside of this change is that comparing fixed-length arrays doesn't always work as smoothly as it did before. Before this, comparisons sometimes worked due to various coercions to slices. I've added impls for `Eq`, `Ord`, etc for fixed-lengths arrays up to and including length 32, but if the array is longer than that you'll need to either newtype the array or convert to slices. Note that this plays better with deriving in any case than the previous scheme.

Fixes #4920.
Fixes #16821.
Fixes #15757.

cc @alexcrichton 
cc @aturon
@bors bors closed this Nov 6, 2014
@bors bors merged commit 81c00e6 into rust-lang:master Nov 6, 2014
@nikomatsakis nikomatsakis deleted the operator-dispatch branch March 30, 2016 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants