Replace many impls of `(Add|Mul|AddAssign|MulAssign)<Self>` with derive macros #62

huitseeker · 2020-01-17T19:36:51Z

The derive macros defer to the versions implemented on Self-type references.
This eliminates a lot of the impl boilerplate (& might go away when Rust figures out how to to autoref/deref on operators).

Hopefully this also serves as inspiration for extension of the approach to Sub, Neg ...

Pratyush · 2020-01-17T19:55:41Z

It would actually be great to add macros for Sub, Neg and Div as well, so that we can have impls for those too =)

huitseeker · 2020-01-22T21:03:19Z

@Pratyush Done! You might want to read the end of the last commit message before review.

Pratyush · 2020-01-23T04:13:17Z

Hmm it would be nice to avoid adding the bound and removing const fn.

Is it possible to rework the macros with the following changes:

Use custom attributes to allow passing custom bounds to the derive macro
To avoid the blowup due to #[derive(SubFromRef, SubAssignFromRef, AddFromRef, ...)], maybe it would be nice to just have two macros: AdditiveOpsFromRef and MultiplicativeOpsFromRef. The former adds impls for Add, AddAssign, Sub, and SubAssign, while the latter handles the Mul and Div equivalents.

If you give me commit access to your branch I can also make these changes, since I'm interested in playing with proc macros anyway =)

Pratyush · 2020-01-24T22:43:25Z

ping @huitseeker if you give me access to your branch then I can make these changes

huitseeker · 2020-01-25T03:03:02Z

@Pratyush AFAICT this is already on:

Pratyush · 2020-01-25T03:04:17Z

Oh hmm I think I had tried that in the past and it hadn't worked; let me try again. Sorry for the noise!

huitseeker · 2020-01-25T03:09:05Z

Note that while the attribute modification is straightforward, the trigger on additivity may be a tad more complicated, as there are many instances where you have AddAssign implemented but not Add, or the reverse.

Pratyush · 2020-01-25T03:28:22Z

Hmm that's a bug; there should be nothing in algebra that is missing one of the impls

huitseeker · 2020-01-25T16:29:10Z

@Pratyush No issue in algebra - there are some missing structs in the curve models, but I assume those could be added -, but I had assumed you may have wanted to extend the use of the macro (which is quite generic) beyond that crate. Looking at the implementations out of it with for i in Add Sub Div Mul; do echo "## $i :"; rg -g '!algebra*' "impl.*?($i)(Assign)?<" -H --no-heading | awk -F" for " 'NR>1{split($2, subfield, "{"); arr[subfield[1]]++}END{for (a in arr) print a, arr[a]}'; done :

## Add :
&'b DensePolynomial<F>  1
Evaluations<F>  1
DensePolynomial<F>  2
LinearCombination<F>  7
ConstraintVar<F>  5
&ConstraintVar<F>  2
&'b Evaluations<F>  1
&LinearCombination<F>  4
## Sub :
&'b DensePolynomial<F>  1
Evaluations<F>  1
DensePolynomial<F>  1
LinearCombination<F>  6
ConstraintVar<F>  4
&ConstraintVar<F>  2
&'b Evaluations<F>  1
&LinearCombination<F>  4
## Div :
&'b DensePolynomial<F>  1
Evaluations<F>  1
## Mul :
&'b DensePolynomial<F>  1
Evaluations<F>  1
LinearCombination<F>  2
ConstraintVar<F>  1
&'b Evaluations<F>  1

Of those:

Evaluations is compatible with a bundled (Foo+FooAssign) macro,
DensePolynomial has irregularities, using for i in Add Sub Mul Div; do echo "## $i:"; rg "impl.*($i)(Assign)?.*?DensePolynomial<F>"; done:

## Add:
ff-fft/src/polynomial/dense.rs
156:impl<'a, 'b, F: Field> Add<&'a DensePolynomial<F>> for &'b DensePolynomial<F> {
182:impl<'a, 'b, F: Field> AddAssign<&'a DensePolynomial<F>> for DensePolynomial<F> {
203:impl<'a, 'b, F: Field> AddAssign<(F, &'a DensePolynomial<F>)> for DensePolynomial<F> {
## Sub:
ff-fft/src/polynomial/dense.rs
252:impl<'a, 'b, F: Field> Sub<&'a DensePolynomial<F>> for &'b DensePolynomial<F> {
284:impl<'a, 'b, F: Field> SubAssign<&'a DensePolynomial<F>> for DensePolynomial<F> {
## Mul:
ff-fft/src/polynomial/dense.rs
321:impl<'a, 'b, F: PrimeField> Mul<&'a DensePolynomial<F>> for &'b DensePolynomial<F> {
## Div:
ff-fft/src/polynomial/dense.rs
309:impl<'a, 'b, F: Field> Div<&'a DensePolynomial<F>> for &'b DensePolynomial<F> {

ContraintVar also has issues (same one-liner as above):

## Add:
r1cs-core/src/impl_constraint_var.rs
69:impl<F: Field> Add<LinearCombination<F>> for ConstraintVar<F> {
94:impl<F: Field> Add<LinearCombination<F>> for &ConstraintVar<F> {
119:impl<F: Field> Add<(F, Variable)> for ConstraintVar<F> {
132:impl<F: Field> AddAssign<(F, Variable)> for ConstraintVar<F> {
183:impl<F: Field> Add<Variable> for ConstraintVar<F> {
200:impl<'a, F: Field> Add<&'a Self> for ConstraintVar<F> {
234:impl<F: Field> Add<&ConstraintVar<F>> for &ConstraintVar<F> {
252:impl<'a, F: Field> Add<(F, &'a Self)> for ConstraintVar<F> {
## Sub:
r1cs-core/src/impl_constraint_var.rs
81:impl<F: Field> Sub<LinearCombination<F>> for ConstraintVar<F> {
106:impl<F: Field> Sub<LinearCombination<F>> for &ConstraintVar<F> {
174:impl<F: Field> Sub<(F, Variable)> for ConstraintVar<F> {
191:impl<F: Field> Sub<Variable> for ConstraintVar<F> {
217:impl<'a, F: Field> Sub<&'a Self> for ConstraintVar<F> {
243:impl<F: Field> Sub<&ConstraintVar<F>> for &ConstraintVar<F> {
270:impl<'a, F: Field> Sub<(F, &'a Self)> for ConstraintVar<F> {
## Mul:
r1cs-core/src/impl_constraint_var.rs
152:impl<F: Field> Mul<F> for ConstraintVar<F> {
164:impl<F: Field> MulAssign<F> for ConstraintVar<F> {
## Div:

LinearCombinations is incompatible as well:

## Add:
r1cs-core/src/impl_lc.rs
77:impl<F: Field> Add<(F, Variable)> for LinearCombination<F> {
87:impl<F: Field> AddAssign<(F, Variable)> for LinearCombination<F> {
133:impl<F: Field> Add<Variable> for LinearCombination<F> {
187:impl<F: Field> Add<&LinearCombination<F>> for &LinearCombination<F> {
205:impl<F: Field> Add<LinearCombination<F>> for &LinearCombination<F> {
223:impl<'a, F: Field> Add<&'a LinearCombination<F>> for LinearCombination<F> {
241:impl<F: Field> Add<LinearCombination<F>> for LinearCombination<F> {
340:impl<F: Field> Add<(F, &LinearCombination<F>)> for &LinearCombination<F> {
360:impl<'a, F: Field> Add<(F, &'a LinearCombination<F>)> for LinearCombination<F> {
380:impl<F: Field> Add<(F, LinearCombination<F>)> for &LinearCombination<F> {
399:impl<F: Field> Add<(F, Self)> for LinearCombination<F> {
## Sub:
r1cs-core/src/impl_lc.rs
97:impl<F: Field> Sub<(F, Variable)> for LinearCombination<F> {
142:impl<F: Field> Sub<Variable> for LinearCombination<F> {
259:impl<F: Field> Sub<&LinearCombination<F>> for &LinearCombination<F> {
281:impl<'a, F: Field> Sub<&'a LinearCombination<F>> for LinearCombination<F> {
301:impl<F: Field> Sub<LinearCombination<F>> for &LinearCombination<F> {
321:impl<F: Field> Sub<LinearCombination<F>> for LinearCombination<F> {
419:impl<F: Field> Sub<(F, &LinearCombination<F>)> for &LinearCombination<F> {
427:impl<'a, F: Field> Sub<(F, &'a LinearCombination<F>)> for LinearCombination<F> {
435:impl<F: Field> Sub<(F, LinearCombination<F>)> for &LinearCombination<F> {
443:impl<'a, F: Field> Sub<(F, LinearCombination<F>)> for LinearCombination<F> {
## Mul:
r1cs-core/src/impl_lc.rs
116:impl<F: Field> Mul<F> for LinearCombination<F> {
126:impl<F: Field> MulAssign<F> for LinearCombination<F> {
## Div:

In sum, the unbundled macro seems more versatile unless we want to rewrite it for use outside the algebra crate. I also like the idea that with an unbundled macro scheme, the developer can deduce what traits are implemented from the derive annotation, without too much implicit knowledge.

huitseeker · 2020-01-25T21:55:35Z

@Pratyush Done with the custom attribute (at the cost of some complexity in the macros, please have them reviewed by your local/favorite proc macro expert).

huitseeker · 2020-01-27T16:18:46Z

@Pratyush rebased on the sccache master state.

…ve macros The derive macros defer to the versions implemented on `Self`-type references.

…FromRef This required removing the const annotation on the constructor for fp_256->fp_832. This can be re-added with `#![feature(const_fn)]`. This was necessary because otherwise the derivation of generic parameters would have to rely on name mangling.

… a trait bound on the struct

Pratyush · 2020-01-27T21:16:12Z

Hmm, I did actually intend to provide these impls only for algebra. I think it might be simpler to handle these just via macro_rules macros instead of the proc_macro machinery which might be overkill.

However, I think the machinery in this PR can be repurposed for two uses:

A derive macro like ff-derive that generates the parameters for fields (but implemented on FpParameters, not on a Fp type)
Derive macros for common traits in r1cs-std, like ConditionalEqGadget, ToBytesGadget, ConditionalSelectGadget, etc. because these are currently just a lot of boilerplate.

huitseeker · 2020-02-02T03:02:19Z

@Pratyush At the end of the day, this PR is pretty much exactly auto_impl specialized to the std::ops::(Add|...) traits, triggered on the struct rather than on the trait. It's a very generic approach that I'm happy taking in the direction that's the most useful.

Would you mind opening (two) issues fleshing out your thinking?

huitseeker · 2020-02-17T22:59:18Z

Superseded by #98

huitseeker force-pushed the derive_macros branch from 456d5ed to adae5e6 Compare January 22, 2020 21:02

huitseeker force-pushed the derive_macros branch from adae5e6 to e2cf170 Compare January 25, 2020 21:54

huitseeker force-pushed the derive_macros branch from e2cf170 to 1934cc9 Compare January 27, 2020 16:16

huitseeker force-pushed the derive_macros branch from 1934cc9 to a413cfa Compare January 27, 2020 18:05

huitseeker added 4 commits January 27, 2020 11:50

Replace many impls of (Add|Mul|AddAssign|MulAssign)<Self> with deri…

1240d74

…ve macros The derive macros defer to the versions implemented on `Self`-type references.

Add derivability through custom macro attribute rather than requiring…

697944d

… a trait bound on the struct

reinstate const constructors and bound-less structs

6343100

huitseeker force-pushed the derive_macros branch from a413cfa to 6343100 Compare January 27, 2020 20:02

Pratyush mentioned this pull request Feb 14, 2020

Add X, XAssign ops for X in {Add, Sub, Mul, Div} when taking arguments by value. #98

Merged

huitseeker closed this Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace many impls of `(Add|Mul|AddAssign|MulAssign)<Self>` with derive macros #62

Replace many impls of `(Add|Mul|AddAssign|MulAssign)<Self>` with derive macros #62

huitseeker commented Jan 17, 2020 •

edited

Pratyush commented Jan 17, 2020

huitseeker commented Jan 22, 2020

Pratyush commented Jan 23, 2020

Pratyush commented Jan 24, 2020

huitseeker commented Jan 25, 2020

Pratyush commented Jan 25, 2020

huitseeker commented Jan 25, 2020

Pratyush commented Jan 25, 2020

huitseeker commented Jan 25, 2020 •

edited

huitseeker commented Jan 25, 2020 •

edited

huitseeker commented Jan 27, 2020

Pratyush commented Jan 27, 2020

huitseeker commented Feb 2, 2020

huitseeker commented Feb 17, 2020

Replace many impls of (Add|Mul|AddAssign|MulAssign)<Self> with derive macros #62

Replace many impls of (Add|Mul|AddAssign|MulAssign)<Self> with derive macros #62

Conversation

huitseeker commented Jan 17, 2020 • edited

Pratyush commented Jan 17, 2020

huitseeker commented Jan 22, 2020

Pratyush commented Jan 23, 2020

Pratyush commented Jan 24, 2020

huitseeker commented Jan 25, 2020

Pratyush commented Jan 25, 2020

huitseeker commented Jan 25, 2020

Pratyush commented Jan 25, 2020

huitseeker commented Jan 25, 2020 • edited

huitseeker commented Jan 25, 2020 • edited

huitseeker commented Jan 27, 2020

Pratyush commented Jan 27, 2020

huitseeker commented Feb 2, 2020

huitseeker commented Feb 17, 2020

Replace many impls of `(Add|Mul|AddAssign|MulAssign)<Self>` with derive macros #62

Replace many impls of `(Add|Mul|AddAssign|MulAssign)<Self>` with derive macros #62

huitseeker commented Jan 17, 2020 •

edited

huitseeker commented Jan 25, 2020 •

edited

huitseeker commented Jan 25, 2020 •

edited