Tracking issue: Allow autoderef and autoref in operators (experiment)

nikomatsakis commented 6 years ago

This is a specific issue to track the changes proposed by @arielb1 in RFC 2174. We decided to roll this into a larger experiment around coercions, generics, and Copy type ergonomics and are therefore ready to implement (but not yet stabilize).

nikomatsakis commented 6 years ago

Note: @arielb1 was going to write-up some mentoring instructions here when they get the time. =)

arielb1 commented 6 years ago

Mentor Notes

This basically requires 2 main steps:

changing method-style lookup to allow for lookup based on multiple types
changing operator dispatch to use method-style lookup

1. Changing method-style lookup to allow for lookup based on multiple types

Method lookup needs to have flags added to it, so it can support operator lookup as specified in RFC 2174.

The semantics are actually pretty similar to how it works today, but there are a few needed changes:

Operator lookup looks for a specific method DefId - the operator's trait method - instead of looking for all methods with the right name.
Operator lookup can't perform mutable autoref.
Operator lookup works with 2 receivers, while method lookup works with only 1

The code for looking up methods is in rustc_typeck::method::probe, and it needs to be adapted for all 3 components.

You basically want to add a method on FnCtxt that does an operator-style probe, which should have a similar interface to probe_for_name at https://github.com/rust-lang/rust/blob/d8d5b6180f56e4aaddc9492b891f85f9ca4a3c64/src/librustc_typeck/check/method/probe.rs#L201-L283

However, I feel that operator dispatch avoids most of the code in probe_op, so it's probably a good idea to copy-paste and modify probe_op instead of adding a new bunch of flags to it. You probably also want to call pick_core directly instead of doing the extra error reporting pick does - the extra error reporting does not look relevant for method calls.

Only probing for a specific DefId should be easy - instead of calling assemble_inherent_candidates etc., just add the operator method candidate (as a TraitCandidate) to extension_candidates.

Similarly, not probing for mutable autoref just requires having a flag and an if around the call to pick_autorefd_method with a mutable autoref, found in https://github.com/rust-lang/rust/blob/d8d5b6180f56e4aaddc9492b891f85f9ca4a3c64/src/librustc_typeck/check/method/probe.rs#L837

Allowing dispatch with 2 receivers is a bit more annoying. I think the cleanest way would be to have an optional "secondary" xform_self_ty field on Candidate and to have pick_method and friends take an optional second step parameter, but I could be wrong about this. You'll also need to change ProbeContext so that it has an optional second set of steps, but it's a Simple matter of Coding™.

You will also need to have a variant of method confirmation that handles operators - see https://github.com/rust-lang/rust/blob/d8d5b6180f56e4aaddc9492b891f85f9ca4a3c64/src/librustc_typeck/check/method/confirm.rs#L80-L143

For that method, you should make the segment parameter optional - it is here to support giving extra type parameters with a turbofish (e.g. my_iter.collect::<Vec<_>>), which isn't possible with operators (that would be something like x +::<IntegerAdd> y, which isn't valid Rust).a

2. Changing operator dispatch to use method-style lookup

For dispatch, there are 2 kinds of operators, "rvalue" operators and "lvalue" operators (the relevant lvalue operator here is overloaded indexing - implementing autoderef for the * operator is a bit pointless). You'll need to change both of them to use the new method-style lookup you implemented in step 1 instead of the hacky lookup they do now.

For an example of method lookup, see lookup_method (the function also does a bunch of error message guessing and other stuff you don't care about, you probably just want the probe and confirm components) at https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/method/mod.rs#L144-L180

Rvalue operators are implemented in rustc_typeck::check::op - check_overloaded_binop - https://github.com/rust-lang/rust/blob/d8d5b6180f56e4aaddc9492b891f85f9ca4a3c64/src/librustc_typeck/check/op.rs#L156-L306

Array indexing is implemented in lookup_indexing and try_index_step - https://github.com/rust-lang/rust/blob/d8d5b6180f56e4aaddc9492b891f85f9ca4a3c64/src/librustc_typeck/check/mod.rs#L2188-L2290

lookup_indexing does something weird with builtin indexing, I think because arrays do not implement the Index/IndexMut traits. You might need to add a BuiltinIndexCandidate or some variant of it something to method lookup to get around this. You should avoid the call to write_method_call or undo it (by removing the entries write_method_call inserted) because otherwise you'll have weird borrow errors.

Note: feature gating

This is a big improvement to operator lookup, which we might not want to be active by default. Initially, we should probably keep the old operator lookup code, and switch between the old and the new code using feature gating.

rustyseris commented 6 years ago

I would like to give it a shot!

arielb1 commented 6 years ago

@rustyseris

That's cool. This is a nice quality-of-life improvement I'm looking forward to seeing. If you need any help, I'm @arielb1 on gitter.

rustyseris commented 6 years ago

I've studied a long time the code and the documentation to have a glimse on the working of the code i was going to work with.

I'm coming back to you to present a work-in-progress to be sure i'm on the good track : https://github.com/rustyseris/rust/commit/858a0d89f7dcf271a163e05935b9985417a429d0

I think I should use the Op enum defined here directly instead of having a is_assign flag. https://github.com/rustyseris/rust/blob/858a0d89f7dcf271a163e05935b9985417a429d0/src/librustc_typeck/check/op.rs#L517 The main problem here is that I'm not sure if I can put it in public. It will not leak outside the check module, that's for sure.

I've taken some shortcut for now using unwrap(), i should add some error handling code instead of making the whole compiler panic x) but i'm not sure what exactly i should do.