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] Don't rely on the type of lhs when type checking lhs OP rhs #19434

Closed
wants to merge 4 commits into from

Conversation

japaric
Copy link
Member

@japaric japaric commented Dec 1, 2014

Tweaks the binary operator dispatch algorithm to not rely on the full resolution of the type of the LHS.

Closes #8280
Closes #19035


This is an overview of the current type checking algorithm vs the new algorithm introduced in this PR:

Current

  • Check lhs
    • (i.e. check_expr(lhs))
  • Fully resolve the type of lhs (compiler error if can't fully resolve)
  • If lhs_ty is integer and OP is bit-shift
    • Enforce that rhs_ty is a subtype of uint
  • Categorize lhs_ty and OP, if lhs_ty OP lhs_ty is a built-in operation
  • Raise compile error if OP is && or ||
  • Otherwise operator is overloaded, proceed with trait dispatch

New (this PR)

  • Check both lhs and rhs
    • (i.e. check_expr(lhs|rhs))
  • Try to fully resolve lhs and rhs
  • If both got fully resolved
    • (i.e. iff both try_structurally_resolved_type(lhs|rhs) return Some)
    • If both lhs_ty and rhs_ty are integers and OP is bit-shift
      • Enforce that rhs_ty is a subtype of uint
    • Categorize lhs_ty, rhs_ty and OP, if lhs_ty and rhs_ty have the same category and lhs OP rhs is a built-in operation
      • (i.e. is_builtin_binop(cx, lhs_ty, op) returns true)
      • Watch out for SIMD comparison and write type of lhs OP rhs expression
  • Raise compile error if OP is && or ||
  • Otherwise operator is overloaded, proceed with trait dispatch

This PR passes make check as it is, but it comes with two new issues:

  • The "reborrow adjustment" trick we use to make &'a T == &'b T work even though only &'a T == &'a T (note same lifetime 'a) should work ... well, it no longer works (or it doesn't work in most cases). However, this is not a real problem, because with Overloaded comparison traits #19167 (overloaded == operator), we can now impl PartialEq<&'b T> for &'a T to take care references of with different lifetimes.
  • The most aggravating issue is an increase in the verbosity and decrease in the quality of the error messages related to binary operators. Consider the following code:
struct Foo;

#[deriving(PartialEq)]
struct Bar(Foo);

fn main() {}

Which now generates the following error message:

deriving.rs:4:12: 4:16 error: binary operation `==` cannot be applied to type `Foo`
deriving.rs:4 struct Bar(Foo);
                         ^~~~
note: in expansion of #[deriving]
deriving.rs:3:1: 3:23 note: expansion site
deriving.rs:4:12: 4:16 error: binary operation `&&` cannot be applied to type `bool`
deriving.rs:4 struct Bar(Foo);
                         ^~~~
note: in expansion of #[deriving]
deriving.rs:3:1: 3:23 note: expansion site
deriving.rs:4:12: 4:16 error: binary operation `==` cannot be applied to type `Foo`
deriving.rs:4 struct Bar(Foo);
                         ^~~~
note: in expansion of #[deriving]
deriving.rs:3:1: 3:23 note: expansion site
deriving.rs:4:12: 4:16 error: binary operation `!=` cannot be applied to type `Foo`
deriving.rs:4 struct Bar(Foo);
                         ^~~~
note: in expansion of #[deriving]
deriving.rs:3:1: 3:23 note: expansion site
deriving.rs:4:12: 4:16 "
deriving.rs:4 struct Bar(Foo);
                         ^~~~
note: in expansion of #[deriving]
deriving.rs:3:1: 3:23 note: expansion site
deriving.rs:4:12: 4:16 error: binary operation `!=` cannot be applied to type `Foo`
deriving.rs:4 struct Bar(Foo);
                         ^~~~
note: in expansion of #[deriving]
deriving.rs:3:1: 3:23 note: expansion site
error: aborting due to 6 previous errors
  • The error message "binary operation == cannot be applied to type Foo" (and its != brother) should appear only once.
  • The error message "binary operation == cannot be applied to type Foo", although it sounds absurd, it's actually a real error, just that the message it's bad. That message it's actually referring to the operation bool && [Type Error] where [Type Error] is the type of Foo == Foo (which can't be resolved, since Foo doesn't implement PartialEq).

@nikomatsakis Thoughts? I think I can fix/improve the error messages, but I'd like to know what you think about the approach and whether the "reborrow adjustment" can/should be fixed or not.

@japaric
Copy link
Member Author

japaric commented Dec 1, 2014

@nikomatsakis BTW, this is rebased on top of #19167, so you should ignore the first commit.

@sinistersnare
Copy link
Contributor

with this PR, will lhs == some_generic_function() not require a type annotation if there is only one overload for lhs? or is this not related to the PR?

@japaric
Copy link
Member Author

japaric commented Dec 1, 2014

@sinistersnare This PR is unrelated to that case.

lhs == some_generic_function() should never require type annotation iff there's only one impl PartialEq for Lhs. As soon as you overload the == (with, say, impl PartialEq<Foo> for Lhs), then you may need to annotate some_generic_function() in some cases where the compiler can't infer the rhs type for you.

@nikomatsakis
Copy link
Contributor

@japaric I'll take a look. I'm not sure if the revised algorithm feels... quite right. Let me read what you did and ponder it a bit.

@Gankra
Copy link
Contributor

Gankra commented Jan 2, 2015

@nikomatsakis status?

@japaric
Copy link
Member Author

japaric commented Jan 11, 2015

Because of the new coherence rules, we can't overload binary operations where the LHS is a built-in type (see #20749), which was the main motivation of this PR (i.e. to allow the 1.0 + big_float sugar). So I'm closing this PR until the coherence issue gets resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants