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

feat: visibility for impl functions #6179

Merged
merged 12 commits into from
Oct 3, 2024
Merged

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Sep 30, 2024

Description

Problem

Part of #4515

Summary

After searching for a while I found that to make this work we only needed to change one line

Additional Context

I didn't include tests for now, but I'll add them later because we first need to figure out which impl functions should be marked as pub or pub(crate) in the stdlib. I'm not familiar with the stdlib though I probably can't do it. That said, if you open the standard library with a nargo compiled from this PR you already get some warnings, so those functions should at least not be private.

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite requested a review from a team September 30, 2024 19:28
@TomAFrench
Copy link
Member

turbofish_numeric_generic_nested_call is failing so it seems like private impl functions cannot be called from the same module (i.e. the functions are private to within the impl rather than to the module in which the impl sits)

@asterite
Copy link
Collaborator Author

asterite commented Oct 1, 2024

Ah, right. I just bumped into this while trying to add some visibilities. It seems that also U128::method fails if you are inside U128, but it doesn't fail if you do Self::method 🤔 I'll take a look 😄

@asterite
Copy link
Collaborator Author

asterite commented Oct 1, 2024

Done! All tests pass now. I think there are still some fns that could be made pub.

@TomAFrench
Copy link
Member

This seems like a slightly different case to what I meant in my comment. I've pushed a new test case which I would expect to pass but doesn't.

@asterite
Copy link
Collaborator Author

asterite commented Oct 2, 2024

I think I got it now! 😄

@asterite
Copy link
Collaborator Author

asterite commented Oct 2, 2024

I noticed that visibility isn't checked for the new TypePath, which is something like Field::from_le_bytes where what's before :: isn't a Path but a Type. That requires different code to do it so I'd prefer to do it in a separate PR.

@TomAFrench
Copy link
Member

Yeah, there's a good amount of stuff broken with type aliases due to this, a lot of issues related to type aliases stem from this #4469

@asterite
Copy link
Collaborator Author

asterite commented Oct 3, 2024

I see.

Well, this is ready for review in case you want to take a look at it.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern about a function we're making public but otherwise looks fine.

noir_stdlib/src/embedded_curve_ops.nr Outdated Show resolved Hide resolved
@asterite asterite added this pull request to the merge queue Oct 3, 2024
Merged via the queue into master with commit 1b26440 Oct 3, 2024
47 checks passed
@asterite asterite deleted the ab/visibility-for-impl-functions branch October 3, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants