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

RFC: Change operator method lookup to be less magical #4920

Closed
nikomatsakis opened this issue Feb 13, 2013 · 15 comments · Fixed by #18486
Closed

RFC: Change operator method lookup to be less magical #4920

nikomatsakis opened this issue Feb 13, 2013 · 15 comments · Fixed by #18486
Assignees
Labels
A-trait-system Area: Trait system A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@nikomatsakis
Copy link
Contributor

Right now, operator "methods" proceed according to the same logic as any other method. But this leads to surprising results like this: https://gist.github.com/bstrie/4948410

Also, operators are already handled somewhat specially because we auto-ref their arguments. This means that you can write val1-of-my-linear-type == val2-of-my-linear-type and it doesn't move or do anything creepy (to call eq explicitly, in contrast, you'd write val1-of-my-linear-type.eq(&val2-of-my-linear-type)).

I think method lookup for operators should never apply any automatic transformations to the receiver or arguments, other than the current auto-ref behavior (which means that a OP b is kind of implicitly (&a).OP(&b))

@pcwalton I expect you in particular might have an opinion about this.

@nikomatsakis
Copy link
Contributor Author

I think this should apply also to unary operators and the [] operator, except that [] autoderefs its receiver (as it does normally).

@nikomatsakis
Copy link
Contributor Author

@nikomatsakis
Copy link
Contributor Author

It seems that we only partially discussed this issue in the meeting. In particular, we did not discuss:

  1. Removing argument coercions from uses of overloaded operators
  2. Extending auto-ref to the argument of the [] operator, which seems useful for hashtables

On point 2, currently a[b] is converted to a.index(b) but if we extended autoref (as we do with all other operators) it would be equivalent to a.index(&b).

@graydon
Copy link
Contributor

graydon commented Mar 26, 2013

seems reasonable to me

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Mar 27, 2013
@graydon
Copy link
Contributor

graydon commented May 22, 2013

Looks like this was merged in #5558 .. @nikomatsakis can this close?

@pnkfelix
Copy link
Member

pnkfelix commented Sep 4, 2013

Visiting for triage, email from 2013-09-02.

Nominating for milestone 1, well-defined.

(Of course, its possible that graydon's hypothesis that this might be closeable is true. @nikomatsakis, any thoughts?)

@nikomatsakis
Copy link
Contributor Author

Not closeable in that point 1 remains unresolved: do we want to coerce arguments? I remember @pcwalton had this on the agenda recently (under the guise of == operations).

@catamorphism
Copy link
Contributor

Accepted for milestone 1, well-defined

@flaper87
Copy link
Contributor

Triage bump, this still needs some work.

@sinistersnare
Copy link
Contributor

Should this go through the new RFC process? I hope that == does no coercion, and it might un-stagnate the conversation.

@rust-highfive
Copy link
Collaborator

This issue has been moved to the RFCs repo: rust-lang/rfcs#283

@japaric
Copy link
Member

japaric commented Oct 27, 2014

@nick29581 I think @rust-highfive shouldn't have closed this issue, since it's marked as P-backcompat-lang - unless that tag no longer applies, but I don't think that's the case here.

@nrc
Copy link
Member

nrc commented Oct 27, 2014

reopening and nominating for triage to decide what to do with this

@nrc nrc reopened this Oct 27, 2014
@nikomatsakis
Copy link
Contributor Author

Repeating a relevant comment from #16918:

what I'm most concerned about is that, currently, we do a kind of odd-ball search when looking up operators. e.g., if we have l + r, where the types of l and r are L and R, I just want to search for Add<L,R>, rather than doing a traditional method search (which involves auto-ref, coercion, and other considerations). This does mean that in some cases we might wind up writing "convenience" impls for things like String compared with &str that basically encode common coercions. But I think it's more predictable overall. Today I whippe up a prototype of this approach, though it doesn't quite build yet. One obstacle is the current Eq and Ord traits which do not permit comparison between two distinct types for some reason.

@nikomatsakis nikomatsakis self-assigned this Oct 30, 2014
@nikomatsakis
Copy link
Contributor Author

I am working on this. I consider this a bug fix and not RFC worthy, though this is perhaps debatable. Basically what I am implementing is what I think the semantics were supposed to be, and it requires very few changes to existing code anyhow since most code is using operators as intended.

Basically the changes I am doing are:

  1. The [] operator (both for indexing and slicing) autoderefs consistently (right now it only autoderefs for built-in but not overloaded usages, and slicing doesn't autoderef at all).
  2. An expression like a op b is basically a trait lookup OpTrait<A,B> where A and B are the types of a and b.
  3. Various minor fixes to mutability correction.

Point 2 is what occasionally causes breakage. We used to accept things like foo + bar where foo had type &int and bar had type int. This worked because we applied a kind of odd method search where we did not autoderef but we did autoref, but we did it in reverse order of precedence for a normal method (so we tried to autoref first). I don't consider this to be a particularly sensible semantics: it is neither normal method lookup nor a simple trait lookup, it's just a third thing. This means that occasionally the new code requires an extra *.

Now, I think there are changes we could (and should) make to the operator traits themselves (as I have discussed with @aturon), but what I'm working on now is strictly limited to how the operator traits are matched in the compiler.

bors added a commit that referenced this issue 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
calebcartwright added a commit to calebcartwright/rust that referenced this issue Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants