Supress warning on 0.4.x branch for MontConfig derivation (I need a different base-branch)

[richardpringle]

BRANCH IS WRONG... do you have a branch for a backport?

This is the actual commit I'd want released as 0.4.3
3f06040

Description

Closes #980
0.4.2 causes some compiler warnings with the MontConfig derivation. It would be great to get a patch release out with these changes from 0.5.x

---

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)
  Seems like there's no 0.4.x branch... think you could make one?
• Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
• Wrote unit tests
  Just a scoping change on derived trait code
• Updated relevant documentation in the code
  same as above
• Added a relevant changelog entry to the Pending section in CHANGELOG.md
  I can do this if you'd like.
• Re-reviewed Files changed in the GitHub PR explorer

[richardpringle]

@z-tech @Pratyush @mmagician

Is there any chance we can get something released? If not, I'll close this and look into another solution

[z-tech]

Why is this deleted?

[richardpringle]

Only the commit on top of 0.4.2 is relevant.
3f06040

Any code that looks like it's being deleted is newer than 0.4.2. I would ignore the diff here though.

Edit: (until I can change the base branch)

[z-tech]

So, you want a v0.4.3 that fixes the warnings? Should that be added here with a description of changes?

[z-tech]

Hi @richardpringle, thanks for the PR.

Fixing warnings is generally helpful and appreciated. However, it's not clear how to reproduce the warning you mentioned or what the minimal fix is. For clarity, there's a difference between fixing the root cause of a warning and just telling the compiler to ignore it (suppression).

If you're interested, please consider opening a PR that shows how to reproduce the warning and includes the minimal change to fix it (without the version changes etc). Then maybe we could go from there?

[z-tech]

BTW, I'm pretty sure you can open a branch from any commit, and then you can submit a pull against that branch with your fixes. From there, when / if we decide to move forward we'd do something like tag that branch with the version number and let it live somewhere in perpetuity but please don't worry about these details for the moment.

[richardpringle]

BTW, I'm pretty sure you can open a branch from any commit, and then you can submit a pull against that branch with your fixes. From there, when / if we decide to move forward we'd do something like tag that branch with the version number and let it live somewhere in perpetuity but please don't worry about these details for the moment.

Hmm, I've never opened a branch on someone else's repo in an org that I wasn't a part of, but I can try.

[richardpringle]

@z-tech, yeah I didn't think that this was possible. Someone with push perms to the repo has to create the branch.

BTW, I'm pretty sure you can open a branch from any commit, and then you can submit a pull against that branch with your fixes. From there, when / if we decide to move forward we'd do something like tag that branch with the version number and let it live somewhere in perpetuity but please don't worry about these details for the moment.

Hmm, I've never opened a branch on someone else's repo in an org that I wasn't a part of, but I can try.

[richardpringle]

@z-tech

To reproduce the issue, just run cargo +1.86 check -p ark-test-curves on the v0.4.2 and you'll see

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
 --> test-curves/src/fp128.rs:4:10
  |
4 | #[derive(ark_ff::MontConfig)]
  |          ^-----------------
  |          |
  |          `MontConfig` is not local
  |          move the `impl` block outside of this function `fqconfig___`
...
7 | pub struct FqConfig;
  |            -------- `FqConfig` is not local
  |
  = note: the derive macro `ark_ff::MontConfig` defines the non-local `impl`, and may need to be changed
  = note: the derive macro `ark_ff::MontConfig` may come from an old version of the `ark_ff_macros` crate, try updating your dependency with `cargo update -p ark_ff_macros`
  = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
  = note: `#[warn(non_local_definitions)]` on by default
  = note: this warning originates in the derive macro `ark_ff::MontConfig` (in Nightly builds, run with -Z macro-backtrace for more info)

If you create a branch, I can update this PR and add a blurb to the changelog.

Just to be clear, you guys already fixed this, I pulled in your code from the latest version (there were no logical changes, only the scope change)

[z-tech]

@z-tech, yeah I didn't think that this was possible. Someone with push perms to the repo has to create the branch.

Would fork work? Otherwise I can open you the branch tomorrow.

Also for transparency, I have no timeline for when/ if the fix will be published. I wouldn't put this before your other tasks but is nice small improvement.

[richardpringle]

@z-tech, yeah I didn't think that this was possible. Someone with push perms to the repo has to create the branch.

Would fork work? Otherwise I can open you the branch tomorrow.

Also for transparency, I have no timeline for when/ if the fix will be published. I wouldn't put this before your other tasks but is nice small improvement.

Hey, thanks for the quick reply @z-tech .

I could PR my own fork, but a new branch has to be created in this repo anyway if the patch is ever going to be released. If you could create a branch tomorrow that would be much appreciated.

And no worries on timeline. I'm hoping you'll accept my patch pretty quickly since it's really just applying changes that have already been merged to the latest version on to an older version. I didn't write a single line of code for this, I just made sure that there were no logical changes. An if/else was changed (instead of !=, == was used and the blocks were moved... so exact same logically) and const _: () = { ... }; was used for proper scoping.

[z-tech]

Sounds good. Thanks for the clear communication. Here's the branch: https://github.com/arkworks-rs/algebra/tree/version4.2.0 (please base off this)

[richardpringle]

Sounds good. Thanks for the clear communication. Here's the branch: https://github.com/arkworks-rs/algebra/tree/version4.2.0 (please base off this)

@z-tech

I think you misnamed that branch, I think you probably meant to name it version0.4.2 instead of version4.2.0...
It might help to use a naming convention like releases/0.4.x, that way you can use branch protection rules for releases/*. That means you can easily add new patches to that branch and release them. With that said, obviously, you might not want to continue to release patches...

In any case, what commit did you base that branch off of? If you run git log version4.2.0..v0.4.2 you get the following:

57be20e (HEAD, tag: v0.4.2) chore: Release (#620)
4b3d565 Cleanup the changelog
55dda97 chore: Release (#614)
693d133 Merge branch 'master' into releases
da3df1f (tag: v0.4.1) chore: Release (#608)
59a48d6 Merge branch 'master' into releases
c2f23fb Revert "temp remove cyclic dev-dependencies"
0944c9b (tag: v0.4.0) temp remove cyclic dev-dependencies
74d4ff4 chore: Release (#581)
c29a9aa Merge branch 'master' into releases
667ec3e (tag: 0.4.0-alpha.7) chore: Release (#568)

which means you're missing a bunch of commits that are part of the v0.4.2

Furthermore, you've got a bunch of extra commits that aren't in the v0.4.2 history, git log v0.4.2..version4.2.0 --oneline

b6dc180 poly: small refactor and unit tests for `SparsePolynomial` (#868)
6af88cb test(SparseTerm): add small unit tests to cover impl (#866)
45660a2 refactor: use simplified expression (#865)
e107556 Update hashbrown requirement from 0.14 to 0.15 (#859)
# ...
# plus 124 others...

I think what you want to do is git checkout -b version-0.4 v0.4.2 && git push origin version-0.4.

I appreciate your efforts! If this is all a little too low and the priority list, just me me know and I'll close the PR... I can probably just use a fork until I find the time to upgrade to 0.5.

Thank you!

[z-tech]

Hey, when I deleted the branch this autoclosed the PR. If you give me the sha I will make you the branch. I suggest opening your changes in a new branch and we'll keep this one closed as it's a bit full now.

[richardpringle]

@z-tech the sha for v0.4.2 is 57be20e56a142b059bca05653961f8a9ca4f54ae

[z-tech]

Here it is: https://github.com/arkworks-rs/algebra/tree/branch_of_0.4.2