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

Proposal: Implement assignment operator overloads #5992

Closed
brendanzab opened this issue Apr 21, 2013 · 15 comments
Closed

Proposal: Implement assignment operator overloads #5992

brendanzab opened this issue Apr 21, 2013 · 15 comments
Labels
A-traits Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. P-low Low priority

Comments

@brendanzab
Copy link
Member

This would be very useful for core::num (see #4819), and also mathematics libraries.

// overloads `+=`
#[lang="add_assign"]
trait AddAssign<RHS> {
    fn add_assign(&mut self, &other: RHS);
}

// overloads `-=`
#[lang="sub_assign"]
trait SubAssign<RHS> {
    fn sub_assign(&mut self, &other: RHS);
}

// overloads `*=`
#[lang="mul_assign"]
trait MulAssign<RHS> {
    fn mul_assign(&mut self, &other: RHS);
}

// overloads `/=`
#[lang="quot_assign"]
trait QuotAssign<RHS> {
    fn quot_assign(&mut self, &other: RHS);
}

// overloads `%=`
#[lang="rem_assign"]
trait RemAssign<RHS> {
    fn rem_assign(&mut self, &other: RHS);
}

It would also be useful to be able to assign to values accessed via the index operator. This would return a mutable reference to the element. =, +=, -=, *=, /=, and %= would then be based off the overloads defined for that element type.

#[lang="index_assign"]
trait IndexAssign<Index,Element> {
    fn mut_index<'a>(&'a mut self, index: Index) -> &'a mut Element;
}

Edit: Removed Assign trait for = operator.

@Kimundi
Copy link
Member

Kimundi commented Apr 22, 2013

Maybe related: #4329

@graydon
Copy link
Contributor

graydon commented May 2, 2013

nominating for backwards compat

@graydon
Copy link
Contributor

graydon commented May 2, 2013

(I'm pretty sure I don't want to permit overloading = though; users will have nothing left to stand on)

@brendanzab
Copy link
Member Author

@graydon sure.

@graydon
Copy link
Contributor

graydon commented May 2, 2013

accepted for backward compatibility

@chris-morgan
Copy link
Member

I'm working on this at present, though I'm not tackling Index yet.

@chris-morgan
Copy link
Member

The work that I have done thus far on this is on my augmented-assignment branch. Most of it is done (the traits, implementing the assigning traits approximately everywhere where the associated non-assigning trait is implemented, putting the lang items in and handling the expr type in the compiler), but it doesn't yet successfully build code which uses the augmented assignment operators correctly (e.g. if you try to use += with an LHS type that doesn't implement AddAssign<RHS> it will fail with the appropriate error, but in cases where it should work, the compiler crashes).

If someone who actually knows what they are doing could look at it and figure out what I've still got wrong, I would be very grateful. My code should certainly not be accepted without careful review; this is my first work on the compiler itself. (@nikomatsakis, I believe you have owned a lot of this code?)

Note that at present AddAssign<RHS> requires Add<RHS, Self> to be implemented; I don't like this, but at present it's the only reasonable way to get a default method in. Haven't yet decided what should be done about it. See #7059 for more about that.

Sample code:

#[deriving(Eq)]
struct Vector1 {
    x: int,
}

impl Add<Vector1, Vector1> for Vector1 {
    fn add(&self, rhs: &Vector1) -> Vector1 {
        Vector1 {
            x: self.x + rhs.x,
        }
    }
}

impl AddAssign<Vector1> for Vector1 {
    fn add_assign(&mut self, rhs: &Vector1) {
        self.x = self.x + rhs.x;
    }
}

fn main() {
    let v1 = Vector1 { x: 1 };
    let v2 = Vector1 { x: 4 };
    let _v3 = v1.add(&v2);
    let v3 = v1 + v2;
    if v3.x != 5 {
        println("+ :-(");
    }

    let mut v1 = Vector1 { x: 1 };
    let v2 = Vector1 { x: 4 };
    v1.add_assign(&v2);
    v1 += v2;
    if v1.x != 9 {
        println("+= :-(");
    }
}

Compiling this leads to an abort with

rustc: /home/chris/vc/rust/src/llvm/lib/IR/Instructions.cpp:276: void llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, const llvm::Twine&): Assertion `(Args.size() == FTy->getNumParams() || (FTy->isVarArg() && Args.size() > FTy->getNumParams())) && "Calling a function with bad signature!"' failed.

Selected parts of the output with GDB:

rustc: /home/chris/vc/rust/src/llvm/lib/IR/Instructions.cpp:276: void llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, const llvm::Twine&): Assertion `(Args.size() == FTy->getNumParams() || (FTy->isVarArg() && Args.size() > FTy->getNumParams())) && "Calling a function with bad signature!"' failed.

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff7fc2700 (LWP 13558)]
0x00007ffff58dcf77 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56  ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff58dcf77 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff58e05e8 in __GI_abort () at abort.c:90
#2  0x00007ffff58d5d43 in __assert_fail_base (fmt=0x7ffff5a2c3b8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x7ffff491a390 "(Args.size() == FTy->getNumParams() || (FTy->isVarArg() && Args.size() > FTy->getNumParams())) && \"Calling a function with bad signature!\"", 
    file=file@entry=0x7ffff4918768 "/home/chris/vc/rust/src/llvm/lib/IR/Instructions.cpp", line=line@entry=276, 
    function=function@entry=0x7ffff491b580 <llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, llvm::Twine const&)::__PRETTY_FUNCTION__> "void llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, const llvm::Twine&)") at assert.c:92
#3  0x00007ffff58d5df2 in __GI___assert_fail (
    assertion=0x7ffff491a390 "(Args.size() == FTy->getNumParams() || (FTy->isVarArg() && Args.size() > FTy->getNumParams())) && \"Calling a function with bad signature!\"", 
    file=0x7ffff4918768 "/home/chris/vc/rust/src/llvm/lib/IR/Instructions.cpp", line=276, 
    function=0x7ffff491b580 <llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, llvm::Twine const&)::__PRETTY_FUNCTION__> "void llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, const llvm::Twine&)") at assert.c:101
#4  0x00007ffff4264d9d in llvm::CallInst::init(llvm::Value*, llvm::ArrayRef<llvm::Value*>, llvm::Twine const&) ()
   from /home/chris/opt/rust/bin/../lib/./librustllvm.so
#5  0x00007ffff3e81909 in llvm::IRBuilder<true, llvm::ConstantFolder, llvm::IRBuilderDefaultInserter<true> >::CreateCall(llvm::Value*, llvm::ArrayRef<llvm::Value*>, llvm::Twine const&) ()
   from /home/chris/opt/rust/bin/../lib/./librustllvm.so
#6  0x00007ffff41e75c6 in LLVMBuildCall () from /home/chris/opt/rust/bin/../lib/./librustllvm.so
#7  0x00007ffff6129f27 in middle::trans::builder::__extensions__::meth_35620::call::_8fc96a176fc2e2::_0$x2e8$x2dpre () from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#8  0x00007ffff604fb4e in middle::trans::build::Call::_92a0f689105bc1f8::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#9  0x00007ffff60995d3 in middle::trans::base::invoke::_e1ab63192a1f6a6f::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#10 0x00007ffff609727d in middle::trans::callee::trans_call_inner::anon::expr_fn_29337 ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#11 0x00007ffff603f408 in middle::trans::base::with_scope_result::_14c7edc7eee2f21::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#12 0x00007ffff6080c15 in middle::trans::callee::trans_call_inner::_67b8034bab360ac::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#13 0x00007ffff60b7bcf in middle::trans::expr::trans_overloaded_op::_6519f25aaf593e4::_0$x2e8$x2dpre ()
---Type <return> to continue, or q <return> to quit---
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#14 0x00007ffff60b1077 in middle::trans::expr::trans_assign_op::_7ec41527fd36ec84::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#15 0x00007ffff60a9dce in middle::trans::expr::trans_rvalue_stmt_unadjusted::_104d12f0a1705e95::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#16 0x00007ffff6033048 in middle::trans::expr::trans_into::_96f726277f568b6::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#17 0x00007ffff60316c7 in middle::trans::base::trans_stmt::_1989ce2f7584bf94::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#18 0x00007ffff602f79e in middle::trans::controlflow::trans_block::_f2d871ae4ab77770::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#19 0x00007ffff6142e63 in middle::trans::base::trans_closure::_54779ddb14913af8::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#20 0x00007ffff5ffcc35 in middle::trans::base::trans_fn::_34f4f57a46788272::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#21 0x00007ffff5ff5809 in middle::trans::base::trans_item::_5516fd51ed144c0::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#22 0x00007ffff614944d in middle::trans::base::trans_mod::_19a1ff1b36e4f326::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#23 0x00007ffff6159213 in middle::trans::base::trans_crate::_ad6e822415ada776::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#24 0x00007ffff6790a93 in driver::driver::phase_4_translate_to_llvm::_98876058202a4b9a::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#25 0x00007ffff679189e in driver::driver::compile_input::_71707bfacfa5c237::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#26 0x00007ffff67b28b9 in run_compiler::_3c3c9a54ca89072::_0$x2e8$x2dpre ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#27 0x00007ffff67d472e in main::anon::expr_fn_101450 ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#28 0x00007ffff67d04e4 in monitor::anon::expr_fn_101208 ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#29 0x00007ffff67c76a2 in task::__extensions__::try_100078::anon::expr_fn_100496 ()
   from /home/chris/opt/rust/bin/../lib/librustc-d3cb8c2ccd84a7a7-0.8-pre.so
#30 0x00007ffff781f3ad in task::spawn::spawn_raw_oldsched::make_child_wrapper::anon::expr_fn_22342 ()
   from /home/chris/opt/rust/bin/../lib/libstd-6c65cf4b443341b1-0.8-pre.so
---Type <return> to continue, or q <return> to quit---
#31 0x00007ffff5c86932 in task_start_wrapper (a=0x7ffff24d2820) at /home/chris/vc/rust/src/rt/rust_task.cpp:162
#32 0x0000000000000000 in ?? ()

To get decent log information, rustc needs to be built with the configure flag --enable-debug. Then, run rustc with RUST_LOG=::rt::backtrace,rustc=4,rustc::middle::trans::base=3,rustc::metadata=3; you probably want to redirect stderr to a file. (rustc::middle::trans::base=3 because otherwise it'll get a different failure—doesn't seem to be an issue about that, I need to file one—and rustc::metadata=3 because otherwise it'll take an enormous amount of time during crate lookup—ditto.)

Going through the log there, the v1.add_assign(&v2) line leads to a log line of

rust: ~"Call(llfn=*fn(*{i64, *tydesc, *i8, *i8, i8}, *{i64}) -> Void, args=~[~\"*{i64, *tydesc, *i8, *i8, i8}\", ~\"*{i64}\"])"

while the v1 += v2 one leads to this similar line, which is the last line before the LLVM assertion failure:

rust: ~"Call(llfn=*fn(*{i64, *tydesc, *i8, *i8, i8}, *{i64}) -> Void, args=~[~\"*{i64}\", ~\"*{i64, *tydesc, *i8, *i8, i8}\", ~\"*{i64}\"])"

Observe the extra ~"*{i64}" argument there at the start. I presume that to explain the LLVM assertion failure and segfault, but I can't see quite where it comes from.

@thestinger
Copy link
Contributor

I don't think this is a backwards compatibility issue anymore since @pcwalton removed the old system of rewriting x += y to x = x + y. Renominating for the feature complete milestone.

@catamorphism
Copy link
Contributor

accepted for feature-complete

bors added a commit that referenced this issue Jan 10, 2014
…ichton

So far the following code
```
struct Foo;

fn main() {
  let mut t = Foo;
  let ref b = Foo;
  a += *b;
}
```
errors with 
```
test.rs:15:3: 13:11 error: binary operation + cannot be applied to type `Foo`
test.rs:15   *a += *b;
```
Since assignment-operators are no longer expanded to ```left = left OP right``` but are independents operators it should be 
```
test.rs:15:3: 13:11 error: binary operation += cannot be applied to type `Foo`
test.rs:15   *a += *b;
```
to make it clear that implementing Add for Foo is not gonna work. (cf issues #11143, #11344)

Besides that, we also need to typecheck the rhs expression even if the operator has no implementation, or we end up with unknown types for the nodes of the rhs and an ICE later on while resolving types. (once again cf #11143 and #11344).

This probably would get fixed with #5992, but in the meantime it's a confusing error to stumble upon.
@pcwalton, you wrote the original code, what do you think?
(closes #11143 and #11344)
@chris-morgan
Copy link
Member

I've salvaged the traits and implementations part of my augmented-assignment branch; that is now published at augmented-assignment-sans-wiring branch. The wiring is all that's left to be done, and it should be quite straightforward to someone that actually knows what they're doing (unlike me at the time of my first endeavour, and it hasn't really changed).

flaper87 pushed a commit to flaper87/rust that referenced this issue Feb 11, 2014
This is the first part of rust-lang#5992, covering the traits and their
implementations and documentation of it all, but not including the
wiring to make the actual operators (such as `+=`) desugar to the
appropriate method call.

This comes from my old augmented-assignment branch which had the wiring
also but wasn't quite right. Things have changed enough that that wiring
is utterly defunct and unworthy of any attempt at resurrection. The
traits, however, were not, so I have restored that part of the work.

All places in the present code base where any of the arithmetic traits
(`Add`, `Sub`, `Mul`, `Div`, `Rem`, `BitAnd`, `BitOr`, `BitXor`, `Shl`
and `Shr`) were implemented has an `*Assign` trait implementation added,
with the exception (as before and as noted in the code) of `&str` and
`&[T]` where the assignment operators cannot be implemented.

Note that there is necessarily no default implementation of the
`*Assign` traits, as that would preclude more efficient implementations
of the augmented assignment operators and render the whole thing utterly
pointless (c.f. rust-lang#7059 on function specialisation).
@pnkfelix
Copy link
Member

Assigning P-low, since it would be something we can add backwards compatibly post 1.0.

@flaper87 flaper87 self-assigned this Mar 13, 2014
chris-morgan added a commit to chris-morgan/rust that referenced this issue Jun 1, 2014
This is the first part of rust-lang#5992, covering the traits and their
implementations and documentation of it all, but not including the
wiring to make the actual operators (such as `+=`) desugar to the
appropriate method call.

This comes from my old augmented-assignment branch which had the wiring
also but wasn't quite right. Things have changed enough that that wiring
is utterly defunct and unworthy of any attempt at resurrection. The
traits, however, were not, so I have restored that part of the work.

All places in the present code base where any of the arithmetic traits
(`Add`, `Sub`, `Mul`, `Div`, `Rem`, `BitAnd`, `BitOr`, `BitXor`, `Shl`
and `Shr`) were implemented has an `*Assign` trait implementation added,
with the exception (as before and as noted in the code) of `&str` and
`&[T]` where the assignment operators cannot be implemented.

Note that there is necessarily no default implementation of the
`*Assign` traits, as that would preclude more efficient implementations
of the augmented assignment operators and render the whole thing utterly
pointless (c.f. rust-lang#7059 on function specialisation).
@chris-morgan
Copy link
Member

I have updated my augmented-assignment-sans-wiring branch in the hopes that someone would like to take this to completion and land it.

There is one new situation which I have not taken care of: #[simd] types. I presume they will need *Assign implementations added in the compiler wherever they are produced.

@japaric
Copy link
Member

japaric commented Oct 13, 2014

This issue should be moved to the rust-lang/rfcs repo, because it requires adding lang items, which (I think) needs an RFC.

cc @nick29581

@rust-highfive
Copy link
Collaborator

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

@japaric
Copy link
Member

japaric commented Mar 8, 2015

Implementation

RFC

flip1995 pushed a commit to flip1995/rust that referenced this issue Sep 10, 2020
…apsible_if, r=yaahc

Fix the wrong suggestion when using macro in `collapsible_if`

Fix rust-lang#5962

changelog: Fix the wrong suggestion when using macro in `collapsible_if`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. P-low Low priority
Projects
None yet
Development

No branches or pull requests

10 participants