refactor!: introducing #[external(...)] macro#17827
Conversation
benesjan
force-pushed
the
10-20-refactor_introducing_external_._
branch
2 times, most recently
from
5d0728d to
bf0af9e
Compare
benesjan
changed the title
benesjan
force-pushed
the
10-20-refactor_introducing_external_._
branch
from
ab78fd8 to
51c1f55
Compare
| // Marker attribute - see the comment above | ||
|
|
||
| if !is_fn_private(f) & !is_fn_public(f) { | ||
| if !is_fn_external(f) { |
There was a problem hiding this comment.
This is the main sad thing about this PR. Now I am unable to evaluate here if the function is not utility because I don't have the guarantee if the external macro function has already been run. So I only perform this check here and then in utility macro I check if these attributes are not applied.
There was a problem hiding this comment.
Yea this is quite sad. What might be a direction long term? Would we keep doing
#[initializer]
#[external("private")
fn fooor might we move to e.g.
#[external("private", "initializer")]
fn bar
#[external("private", "no_init_check", "view")]
fn quxIn that scenario we'd not need to worry about macro order or not having the full picture.
There was a problem hiding this comment.
I would go with the second approach as it will make the macros much clearer.
The only bad thing about it is that invalid attribute args currently don't play well with LSP:
But I assume that Noir team could improve that and give us a nice error message at which point all seems fine.
benesjan
force-pushed
the
10-20-refactor_introducing_external_._
branch
from
e0c28ff to
c55306a
Compare
benesjan
changed the title
#[external(...)] macro
benesjan
force-pushed
the
10-20-refactor_introducing_external_._
branch
from
c977176 to
1c35ebe
Compare
nventuro
left a comment
nventuro
left a comment
There was a problem hiding this comment.
Very nice, thanks a lot! Leaving the review as a comment as I want to a) confirm with product we'll not accidentally release this until we're ready, and b) discuss a bit how we'll deal with other fn attributes like view, initializer, etc.
noir-projects/noir-contracts-comp-failures/contracts/invalid_external_function_type/src/main.nr
Show resolved
Hide resolved
| // Marker attribute - see the comment above | ||
|
|
||
| if !is_fn_private(f) & !is_fn_public(f) { | ||
| if !is_fn_external(f) { |
There was a problem hiding this comment.
Yea this is quite sad. What might be a direction long term? Would we keep doing
#[initializer]
#[external("private")
fn fooor might we move to e.g.
#[external("private", "initializer")]
fn bar
#[external("private", "no_init_check", "view")]
fn quxIn that scenario we'd not need to worry about macro order or not having the full picture.
benesjan
force-pushed
the
10-20-refactor_introducing_external_._
branch
from
b2aacae to
f77b39a
Compare
benesjan
force-pushed
the
10-20-refactor_introducing_external_._
branch
from
7ecca22 to
24854f2
Compare
|
Seems to have been accidentally closed by Jake so reopening |
Closes https://linear.app/aztec-labs/issue/F-19/rename-privateorpublicorutility-to-externalprivateorpublicorutility Having `#[private]` and `#[public]` macros was misleading because in Solidity these define function visibility while in Aztec's case they define the environment in which the functions are executed. For this reason I introduce `#[external(...)]` macro while the argument of the attribute defines the execution environment: `private`, `public` or `utility`. ## Notes for reviewer - I did a refactor in a [PR up the stack](#17840) which allowed me to drop a lot of the complexity introduced in this PR. For this reason I encourage you to check that one first to have context on what in this PR is a problem and what is not. It might make sense to just merge these 2 PRs into 1 but I think that would make it all harder to review. - This PR touches a lot of files but all the juicy bits are in the `noir-projects/aztec-nr/aztec/src/macros/` directory so I encourage you to just check that first. The `noir-contracts` dir can be completely ignored as there I just update the imports and the macros. - The new naming is now in conflict with the `#[internal]` macro as having both external and internal applied to a function is valid: <img width="874" height="182" alt="image" src="https://github.com/user-attachments/assets/4582a51a-225c-4a4c-9b9a-d2c2d413d927" /> This issue will be tackled in the near future. Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
AztecBot
force-pushed
the
10-20-refactor_introducing_external_._
branch
from
24854f2 to
711bbee
Compare
3 participants
Closes https://linear.app/aztec-labs/issue/F-19/rename-privateorpublicorutility-to-externalprivateorpublicorutility
Having
#[private]and#[public]macros was misleading because in Solidity these define function visibility while in Aztec's case they define the environment in which the functions are executed. For this reason I introduce#[external(...)]macro while the argument of the attribute defines the execution environment:private,publicorutility.Notes for reviewer
noir-projects/aztec-nr/aztec/src/macros/directory so I encourage you to just check that first. Thenoir-contractsdir can be completely ignored as there I just update the imports and the macros.#[internal]macro as having both external and internal applied to a function is valid:This issue will be tackled in the near future.