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

future-proof type-checking of binary operators #23319

Closed
nikomatsakis opened this issue Mar 12, 2015 · 5 comments · Fixed by #23673
Closed

future-proof type-checking of binary operators #23319

nikomatsakis opened this issue Mar 12, 2015 · 5 comments · Fixed by #23673
Assignees
Milestone

Comments

@nikomatsakis
Copy link
Contributor

We need to invest a bit of energy to be sure that the way we handle binary operators is future-proof with possible plans:

  1. We may want to add autoref
  2. We don't want to make too many assumptions

@japaric has been doing some work on this in this PR #22993 and there is discussion there, including my preferred solution ;)

@nikomatsakis
Copy link
Contributor Author

triage: P-backcompat-lang (1.0 beta)

@rust-highfive rust-highfive added this to the 1.0 beta milestone Mar 12, 2015
@nikomatsakis nikomatsakis self-assigned this Mar 12, 2015
@nikomatsakis
Copy link
Contributor Author

I've been working more on this. It turns out that my preferred solution (treat everything like a trait match) did not work because a==b, when applied to SIMD types, does not have a bool result, and hence is not compatible with the PartialEq traits. I've now adopted what I consider to be a superior path. In particular, this path is definitely forwards compatible with optional autoref should we decide to go down that route.

The idea is simple: for each binary operator a+b, we require that the LHS and RHS have known types. In particular, we need to be able to distinguish whether this is a builtin operator application -- e.g. f32+f32 or i64+i64 -- from one that should be dispatched by traits -- e.g. i64 + BigInt. This is somewhat different from what we did before because, before, we only required the type of the LHS to be known.

This does cause some breakage, mostly when the RHS is an application of some generic trait method, e.g. self == Float::infinity(). The fix here is to use UFCS notation (thanks @eddyb) and write self == f32::infinity(), so that the type of both sides are known. As a bonus, this is more readable. In some cases the type may be generic (e.g., some type T where T:Int), in which case one can just write T::zero() or <T>::zero(). (In particular, <> were needed in macros <$ty>::zero() due to what I think coudl be considered a bug.)

Here is an example diff. I can tell you that the resulting code is definitely more readable to someone who has to come and read it for the first time (since that person was me, when making the change):

     #[inline]
     fn is_infinite(self) -> bool {
-        self == Float::infinity() || self == Float::neg_infinity()
+        self == f32::infinity() || self == f32::neg_infinity()
     }

There is one unresolved question. In my current branch, we don't do coercions for either side. This caused one broken change, where a String was being added to a &InternedString. The RHS was being coerced to &str by a deref-coercion. This no longer works for me because I am being stricter about the trait that is matched (L: Add<R> where L and R are the types of the left- and right-hand sides, respectively).

I think I can restore the old coercion behavior, at the cost that trait dispatch viability is based only on the LHS. This will potentially impact future auto-ref fallback, because the decision of whether to autoref or not will be based solely on the LHS.

cc @aturon @japaric, this should interest you both.

@nikomatsakis
Copy link
Contributor Author

There is one unresolved question. In my current branch, we don't do coercions for either side. This caused one broken change, where a String was being added to a &InternedString. The RHS was being coerced to &str by a deref-coercion. This no longer works for me because I am being stricter about the trait that is matched (L: Add where L and R are the types of the left- and right-hand sides, respectively).

Note: Presumably this will also break String + &String, which works today. This is why I am concerned.

@nikomatsakis
Copy link
Contributor Author

Example of code that fails to compile today but I think should compile (and compiles on my branch): http://is.gd/38Z70g

@nikomatsakis
Copy link
Contributor Author

After some discussion with @aturon, we decided that the better thing to do is revert to the previous approach (all traits, all the time) with some hacky code for SIMD. Basically using == for this seems wrong, because == should map to the PartialEq trait, so we'd rather change SIMD than have stronger rules about when types must be known. This makes sense but I'm not eager to re-implement this all for the 3rd time. ;)

bors added a commit that referenced this issue Mar 25, 2015
The current binary operator code assumed that if the LHS was a scalar (`i32` etc), then the RHS had to match. This is not true with multidispatch. This PR generalizes the existing code to (primarily) use the traits -- this also allows us to defer the precise type-checking when the types aren't fully known. The one caveat is unstable SIMD types, which don't fit in with the current traits -- in that case, the types must be known ahead of time.

There is one semi-hacky bit in that during writeback, for builtin operators, if the types resolve to scalars (i32 etc) then we clear the method override. This is because we know what the semantics are and it is more efficient to generate the code directly. It also ensures that we can use these overloaded operators in constants and so forth.

cc @japaric
cc @aturon 

Fixes #23319 (and others).
bors added a commit that referenced this issue Mar 30, 2015
The current binary operator code assumed that if the LHS was a scalar (`i32` etc), then the RHS had to match. This is not true with multidispatch. This PR generalizes the existing code to (primarily) use the traits -- this also allows us to defer the precise type-checking when the types aren't fully known. The one caveat is the unstable SIMD types, which don't fit in with the current traits -- in that case, the LHS type must be known to be SIMD ahead of time.

There is one semi-hacky bit in that during writeback, for builtin operators, if the types resolve to scalars (i32 etc) then we clear the method override. This is because we know what the semantics are and it is more efficient to generate the code directly. It also ensures that we can use these overloaded operators in constants and so forth.

cc @japaric
cc @aturon 

Fixes #23319 (and others).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants