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

[WIP] binops: don't eagerly unify the types of LHS and RHS if one of them is a primitive #22993

Closed
wants to merge 2 commits into from

Conversation

japaric
Copy link
Member

@japaric japaric commented Mar 3, 2015

Multidispatch now works when the LHS is a primitive, e.g. 1 + 1, 1 + BigFloat and 1 + BigInt all compile (if the proper trait impls are in scope).

Because RHS is no longer constrained to have the same type as the LHS when the LHS is a primitive, type annotations are required in some situations where multiple resolutions are possible:

2.0 * rng.gen();  //~ error: can't infer enough type information about `rng.gen()`
// instead write
2.0 * rng.gen::<f64>();
// or
2.0 * rng.gen::<BigFloat>();

This PR also fixes some questionable behavior where the RHS was driving type inference, this no longer compiles:

"5".parse().unwrap() + 0  // used to return 5i32
// instead pick the type of the LHS
"5".parse::<BigInt>().unwrap() + 1

[breaking-change]


closes #19035
closes #22743

r? @nikomatsakis

Another pattern that broke is 1 + (loop {}), because the RHS fallbacks to () and i32 doesn't implement Add<()>. This used to work because loop {} was coerced to i32, and that expression resolved to built-in operation. I think I can add extra logic to coerce the bottom type before it fallbacks to avoid breaking this pattern.

TODO

@@ -2683,8 +2683,14 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
unifier: F) where
F: FnOnce(),
{
debug!(">> typechecking: expr={} expected={}",
expr.repr(fcx.tcx()), expected.repr(fcx.tcx()));
if fcx.inh.node_types.borrow().get(&expr.id).is_some() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, the RHS ended being type checked (check_expr_*) twice. And that causes problems when RHS is a generic enum, eg Some(0) == None didn't type check (RUST_LOG reported unfulfilled _: Sized obligations on None), and the error message was that None needed type annotations: Some(0) == None::<i32>().

This was the least invasive solution that I could think of to avoid type cheking the RHS twice. If you have any other idea, let me know.

@bors
Copy link
Contributor

bors commented Mar 3, 2015

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

@nikomatsakis
Copy link
Contributor

I'm not sure how I feel about the specific approach that this patch takes. Maybe we can chat a bit on IRC. In particular, I am sort of keen to the approach of:

  1. Treat ALL binary operators as overloaded.
  2. In trans and in constant evaluation, if we see a (overloaded) binary operator whose types are two scalars, we don't actually dispatch to the trait but rather we use the builtin semantics (that is, it is not valid to impl Add<i32> for i32 unless you plan on having the expected semantics).

The advantages I see here:

  1. Most front-end code becomes simpler, because all binary operators are always overloaded, so we can remove the code for handling the "non-overloaded" case most of the time.
  2. The type checker in particular does not have to do anything special about trying to know the types of the operators, it can just generate a Add<$1> for $0 obligation.
    • modulo if we want to add optional symmetric autoref
  3. Still retains the privileged status of the builtin adds for constant evaluation and trans (should generate the same code).

Note that you can impl Add<i32> for i32 just by doing a + b -- that will resolve to infinite recursion, but trans won't actually recurse. We could also add some intrinsics if that seems distasteful, but it's not needed I don't think.

I'd be curious to hear others' opinion on this -- naturally @japaric but perhaps also @eddyb or @Aatch? I think this change is purely local to the how the compiler is structured.

@eddyb
Copy link
Member

eddyb commented Mar 4, 2015

@nikomatsakis we already handle intrinsic calls in-line, we could do the same thing for calls to built-in operators.
However... if we use the method side-table, then const_eval and check_const can't easily tell the overload is usable in a constant (and my const fn plan for 1.0 doesn't initially cover trait impls).

@japaric
Copy link
Member Author

japaric commented Mar 5, 2015

@nikomatsakis

Treat ALL binary operators as overloaded.

I like the idea in principle, but when I tried to implement it, type inference went downhill:

// bits: u32
bits >> 31 == 0  //~ error: `PartialEq<i32>` is not implemented by `u32`

This may be solved by making default type params (PartialEq<Rhs=Self>) drive inference.

// exponent: i16
exponent -= 127 + 23;
//          ~~~~~~~~~ expected `i16` found `i32`

To do this correctly, we need to find $0 and $1 given $0: Add<$1> and <$0 as Add<$1>>::Output == u16, but AFAIK the compiler doesn't use output types to resolve obligations.

@nikomatsakis
Copy link
Contributor

On Wed, Mar 04, 2015 at 02:25:15PM -0800, Eduard Burtescu wrote:

@nikomatsakis we already handle intrinsic calls in-line, we could do the same thing for calls to built-in operators.
However... if we use the method side-table, then const_eval and check_const can't easily tell the overload is usable in a constant (and my const fn plan for 1.0 doesn't initially cover trait impls).

That's not quite what I had in mind. Rather, I figured that trans +
constant eval would check the types of the LHS/RHS operands to tell
the difference, rather than considering the built-in operators to be
an intrinsic. This is precisely because I wanted it to work fine for
constant eval.

@nikomatsakis
Copy link
Contributor

On Wed, Mar 04, 2015 at 05:06:37PM -0800, Jorge Aparicio wrote:

@nikomatsakis

Treat ALL binary operators as overloaded.

I like the idea in principle, but when I tried to implement it, type inference went downhill:

Ah, interesting. Yes, it does seem like we'd need either:

  1. your suggestion of using the defaults, but that is a bit tricky,
    because there are multiple defaults at play; or,
  2. a special case around PartialEq (at least) for known integer types
    and integer variables.

@mdinger
Copy link
Contributor

mdinger commented Mar 9, 2015

Does this fix #22099?

@nikomatsakis
Copy link
Contributor

@japaric we can fix the type inference issues (at least in libcore) like so:

nikomatsakis@6473c6a

This basically encodes some knowledge about the binary op traits and the way they are implemented for integral types etc into the typeck code. Eventually, I could imagine the select code being smart enough to figure this stuff out on its own, but it's not there yet.

@japaric
Copy link
Member Author

japaric commented Mar 10, 2015

@mdinger It should.
@nikomatsakis Interesting, I'll take a look at it tonight.

@mdinger
Copy link
Contributor

mdinger commented Mar 10, 2015

Cool. You probably should add it to the header post (closes ...).

@japaric
Copy link
Member Author

japaric commented Mar 11, 2015

@mdinger I've added a TODO to the header post. (Btw, let me know if you spot any other issue that seems related/duplicated)

@mdinger
Copy link
Contributor

mdinger commented Mar 11, 2015

@japaric Will do. Thanks.

@japaric
Copy link
Member Author

japaric commented Mar 12, 2015

Update: After trying @nikomatsakis patch I got further but I'm getting a bunch of "error: user-defined operators are not allowed in constants" in all binary operations that are in const context. I know the cause of the error, and I'm thinking about how to best fix it.

@nikomatsakis
Copy link
Contributor

@japaric let's discuss on IRC :) I have thoughts...

@nikomatsakis
Copy link
Contributor

Closing this due to #23319

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 this pull request may close these issues.

Simple operator overloading does not work Operator dispatch: T op 1.0 works, but 1.0 op T doesn't
5 participants