Impl sub, mul and div for actual objects

[Cesar199999]

Description

Adds support for arithmetic operations on actual objects

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (master)
• Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
• Wrote unit tests
• Updated relevant documentation in the code
• Added a relevant changelog entry to the Pending section in CHANGELOG.md
• Re-reviewed Files changed in the GitHub PR explorer

[Antonio95]

@Pratyush @mmagician Decision needed

We realised the previous implementation of operators *, /, - for owned DensePolynomial (instead of references) broke some things. For instance, this line in poly-commit stopped compiling.

In the specific example given, the reason for the new failure is that, when operators (say, OP) were only implemented for &DensePolynomial OP &DensePolynomial, the compiler turned the call f_poly.div(&z_poly); into (&f_poly).div(&z_poly);. But after we had added DensePolynomial OP DensePolynomial, the compiler was not clever enough to add the reference and complained that DensePolynomial OP RHS should have RHS of type DensePolynomial (not &DensePolynomial).

This indicated us that the best way to ensure the new operator definitions were not a breaking change was to implement DensePolynomial OP DensePolynomial, DensePolynomial OP & DensePolynomial and &DensePolynomial OP DensePolynomial (all of them falling back to the honestly defined & DensePolynomial OP & DensePolynomial ). Since that had to be done for several operators, a macro seemed like the best approach, and we implemented it that way.

However, that introduced another breaking change. I'm gonna explain why I think it fails now, although it is quite subtle. It's easier to just try to compile e.g. poly-commit with the algebra dependency pointing to 4f14ebe. This line now breaks.

My guess as to the issue: In the case of DensePolynomial OP PrimeField, the only preexisting implementation was &DensePolynomial OP &PrimeField. It seems that, upon encountering A OP B, the compiler first checks whether all implementations of A OP ANY_TYPE are defined for &A only, and if so, it adds the reference; and otherwise, it doesn't. The outcome is that, now that we have implementations of Mul for both &DensePolynomial (e.g. and &DensePolynomial), the compiler never adds the reference to the LHS automatically. And this breaks code in which taking ownership is not acceptable and the compiler was implicitly adding a reference before we added Mul implementations for DensePolynomial, such as here.

Possible approaches: We either find a way to fix that problem (I can't see how), admit this breaking change into algebra and fix at least the libraries we have control over which break (e.g. poly-commit; I expect the number of breaking lines to be very small); or discard this PR altogether.

[burdges]

Rust does not coerce types to their references, except for &self and &mut self methods:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=763cda9c812bd55b10dc8470b371880c

Appears this page does not explain this aspect of reference promotion :
https://doc.rust-lang.org/reference/expressions/method-call-expr.html
https://doc.rust-lang.org/reference/type-coercions.html

This restriction is implicit in this sentence however:

If this results in multiple possible candidates, then it is an error, and the receiver must be converted to an appropriate receiver type to make the method call.

[Pratyush]

What happens if we change those types to use the operators? I.e l_poly * xyz or whatever?

[Cesar199999]

@burdges Thanks for the resources! I realized that, apparently, the method invocation a.add(b) can sometimes be coerced by the compiler as (&a).add(b). And you are absolutely correct, that doesn't happen for a + b

After further inspection, I agree with @Antonio95 about what happens in poly-comit. Under normal conditions, lang[j] moves the DensePolynomial out of the array. This couldn't be possible since DensePolynomial does not implement Copy, which tells me that for the compiler was definitely coercing the caller, which makes the line equivalent to:

(&lang[j]).mul(sca_inverse[j] * y_j)

Once we introduce Mul impl's for DensePolynomial (without reference) the compiler cannot optimize the line anymore, and fails to move out the value, it doesn't matter what type the rhs of Mul is.

@Pratyush Do you mean replacing the call to .mul() by * in poly-commit?

[burdges]

I realized that, apparently, the method invocation a.add(b) can sometimes be coerced by the compiler as (&a).add(b). And you are absolutely correct, that doesn't happen for a + b

Yeah, that's bizarre. I'd thought the restriction came from the implemented type, but no it really searches the whole space of methods. If you've so many impl Add flavors, like even core itself does, then rustc makes a choice, which the docs claim it never makes:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b465d11d2d28d2da0d85bdfd09c2932e

[Antonio95]

Wow, very interesting pointers (no pun intended) to the docs and examples, @burdges . Thanks a lot! The coercion decisions seem tricky to pin down. @Cesar199999 and I will test a couple of ideas, but I'm not super confident.

[Cesar199999]

@Burdge It can get even more bizarre... I just noticed that using the fully qualified syntax in the impl block (i.e. core::ops::Add<Foo> instead of Add<Foo>, as in your first example) the reference coercion does not happen anymore and the code won't compile -> https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=654f0a6b6e14fcab4bc0262315f83017

For a TL;DR of the whole issue, @Antonio95 came up with the following example that sums up how the compiler handles the coercion. In short, adding any impl Mul<..> for DensePolynomial block automatically disallows the compiler from coercing the type to a reference, there's no way to fix this by adding "reasonable" changes to this PR.

@Pratyush The only two remaining options are (1) forget about this PR (2) change the line in poly-commit that relies on the implicit coercion (only breaking change we've encountered so far).

[Pratyush]

I think better to rely on explicit annotation of references than relying on implicit coercions which can break from innocuous changes in dependencies. I think this is fine anyway because 0.5.0 has other breaking changes.