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

9892a90
Opened by Niko Matsakis at 2023-09-23 18:59:47

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).

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

    Niko Matsakis at 2017-11-01 18:52:04

  2. Mentor Notes

    This basically requires 2 main steps:

    1. changing method-style lookup to allow for lookup based on multiple types
    2. 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:

    1. Operator lookup looks for a specific method DefId - the operator's trait method - instead of looking for all methods with the right name.
    2. Operator lookup can't perform mutable autoref.
    3. 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.

    Ariel Ben-Yehuda at 2017-11-19 16:59:56

  3. I would like to give it a shot!

    Maerten at 2017-11-28 23:27:09

  4. @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.

    Ariel Ben-Yehuda at 2017-11-29 19:07:20

  5. 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.

    Maerten at 2017-12-09 23:25:07

  6. Hi, I have a mostly working MVP for this ie I can have autoref/autoderef on the self part of binary operators. A lot is still missing, but i'm wondering what I should do next

    peckpeck at 2022-01-29 22:47:08

  7. @peckpeck interesting! Where is it?

    Niko Matsakis at 2022-01-31 14:29:34

  8. Here : https://github.com/peckpeck/rust/tree/autoref_op But i got a little bit carried away, it doesn't work as is, i committed in a non working state and i can't make it work again. Sorry, this is my first attempt at reading the compiler source code and i do a lot of experiment with the code to understand how it works.

    It looks like confirm_method mess up with the caller's state, even with the exact same return value as with original code, the result is not accepted when i pass through it

    peckpeck at 2022-01-31 22:27:01

  9. As for the second argument part i looked into it and saw that most rust_type_check::check::method::* code assume that there is only one parameter which can be autoref/autoderef There will be a lot of

    Simple matter of Coding™

    peckpeck at 2022-01-31 22:41:16

  10. I could use some help in my exploration, I'm pretty sure there are some side effect to the calls of probe_for_name and confirm_method but i can't find which ones.

    peckpeck at 2022-02-02 15:54:09

  11. Progress information: it is now feature gated with #![feature(operator_autoref)]

    It works for unary ops and self in binary ops, but any attempt with something invalid may break the compilation or the resulting binary.

    next step is the hard part : implementing autoref for the rhs if any

    peckpeck at 2022-02-04 14:34:29

  12. Hi, i've now got a working implementation for some use cases, the probe loop is still not checking all use cases and some questions remain, but I think it's starting to be usable.

    peckpeck at 2022-02-11 19:06:33

  13. It works \o/ and passes existing tests (the current behaviour is unchanged) Now I need to write tests to exhibit the new behaviour and fix the search loop (it currently tries everything including mutability change for example)

    What would be the next step after this ?

    peckpeck at 2022-02-16 13:30:02

  14. @peckpeck you can open a PR and put "r? @nikomatsakis" in it

    Niko Matsakis at 2022-02-16 15:13:17

  15. Hi, i will be offline for a week, so the PR will wait a little bit

    peckpeck at 2022-02-18 08:58:06

  16. visiting for T-compiler backlog bonanza.

    Its awesome that @peckpeck picked up the ball and ran with it here, as evidenced at https://github.com/peckpeck/rust/tree/autoref_op

    But, since nothing has landed in tree yet, we still want the tracking issue's status label to reflect that. (@peckpeck , feel free to reach out if you need help turning your branch into a PR.)

    @rustbot label: S-tracking-unimplemented

    Felix S Klock II at 2022-05-06 14:53:51

  17. Can I pick this up if it is still relevant?

    Jessie Chatham Spencer at 2023-09-23 18:59:47