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

Improve constructor syntax check #3630

Open
simon-something opened this issue Dec 8, 2023 · 6 comments
Open

Improve constructor syntax check #3630

simon-something opened this issue Dec 8, 2023 · 6 comments
Labels
T-bug Type: Bug. Something is broken.

Comments

@simon-something
Copy link

simon-something commented Dec 8, 2023

TL;DR: wrong constructor syntax should be caught and have a proper error message, same as a missing constructor.

  • It would be nice to have a specific error message when using this (wrong) syntax:

     #[aztec(private)]
     constructor() {
         (...)
     }
    

    as this fails to compile either with a generic parsing error (Expected an end of input but found contract, which is easy to understand) or, if storage is used and there is some logic inside the constructor, a compute_note_hash_and_nullifier function not found (yes, this is a bit painful to debug the first time).

  • A missing constructor isn't currently caught before attempting to deploy (while it is a mandatory fn), maybe it would be nice to enforce this when compiling.

Rationale is, due to context switching with Solidity or JS (which I guess is to expect), these are difficult errors to catch, especially with the misleading compute_note_hash_and_nullifier error (Rustaceans should be fine tho).

@github-project-automation github-project-automation bot moved this to Todo in A3 Dec 8, 2023
@Shamim06
Copy link

Shamim06 commented Dec 8, 2023

Thanks for pointing this out! Having clearer error messages for incorrect constructor syntax, like #[aztec(private)], and enforcing constructor presence during compilation would greatly improve the Solidity development experience. These enhancements could streamline debugging and benefit developers, especially when switching contexts between languages. Improving error handling in Solidity aligns with creating a more user-friendly environment. Your insights are valuable for enhancing the language’s usability and reliability!

@simon-something
Copy link
Author

simon-something commented Dec 8, 2023

ike #[aztec(private)],

Should have added (but I guess it's a nice way to demonstrate it), here, the issue is a missing fn

e: and the "error handling" is in Noir, not Solidity... GitHub spam?

@rahul-kothari
Copy link
Contributor

Okay so there are really two fixes:

  1. Noir fix: for a syntax error, error message is unhelpful. E.g. if you forget adding the word "fn", you get Expected an end of input but found contract
  2. Aztec-nr fix: throw a compile time error if constructor is missing. It is only currently caught at deploy time today.

Thanks for flagging this @drgorillamd

@simon-something
Copy link
Author

@rahul-kothari For 1., the expected out of input at least point in the right direction of a syntax error, the really big pain is if it throws compute_note_hash_and_nullifier function not found (too aggressively hunting for such?), which mislead to an implementation issue (ergo pain, despair, tears, etc)

@rahul-kothari rahul-kothari added the T-bug Type: Bug. Something is broken. label Dec 11, 2023
rahul-kothari added a commit that referenced this issue Dec 12, 2023
Fix a part of the problem highlighted by #3630 

Also created 
* noir-lang/noir#3768
* [Discussion to see why we prioritise Aztec-nr issues over noir syntax
issues. E.g. if we have storage and a syntax error in a method, instead
of showinf the syntax error, we say "no
compute_note_hash_and_nullifier()
found"](https://aztecprotocol.slack.com/archives/C03P17YHVK8/p1702312023384759)
@rahul-kothari
Copy link
Contributor

Jyst added a compile time error if constructor is missing. Noir team will be looking into better function errors

@simon-something
Copy link
Author

simon-something commented Dec 12, 2023

Great! ty!
Commented the commit @rahul-kothari , maybe we can catch both issues there (I know, a tad hacky tho)

michaelelliot pushed a commit to Swoir/noir_rs that referenced this issue Feb 28, 2024
…col#3649)

Fix a part of the problem highlighted by AztecProtocol#3630 

Also created 
* noir-lang/noir#3768
* [Discussion to see why we prioritise Aztec-nr issues over noir syntax
issues. E.g. if we have storage and a syntax error in a method, instead
of showinf the syntax error, we say "no
compute_note_hash_and_nullifier()
found"](https://aztecprotocol.slack.com/archives/C03P17YHVK8/p1702312023384759)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: Bug. Something is broken.
Projects
Status: Todo
Development

No branches or pull requests

3 participants