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

chore: throw error with duplicated destination labels #308

Open
wants to merge 6 commits into
base: stage
Choose a base branch
from

Conversation

0xrusowsky
Copy link

Overview

This PR addresses issue #295 and aims to fulfill the feature requests made by @erhant and @Philogy.

Proposed Changes

  • Implement a contract-wide label uniqueness check.
  • Newly defined labels are validated against a hashset to ensure no duplication occurs across the entire contract.
  • Duplicated label definitions will throw ParserErrorKind::DuplicateLabel(String).

Caveats

  • test_erc721_compile currently fails due to non-unique labels (error and cont) in huff-examples/erc721/contracts/ERC721.huff.
  • To resolve this, the labels in ERC721.huff should be renamed for uniqueness.

Impact

  • This change ensures label uniqueness across the entire contract, preventing potential bugs related to label name collisions.
  • A decision is needed on renaming labels in ERC721.huff to ensure all tests pass with the new label uniqueness constraints.

@Maddiaa0
Copy link
Member

Maddiaa0 commented Jan 11, 2024

Hey, thank you for doing this!
This is good stuff and I will likely merge it and I can see it follows the approach of the duplicate_macro methods introduced recently!

@Maddiaa0
Copy link
Member

The example in ERC721 is common throughout a lot of contracts in huffmate, another larger bit of work would involve namespacing the labels further to the macro in which they are defined in, such that we address them file::macro::label or similar

@0xrusowsky
Copy link
Author

0xrusowsky commented Jan 11, 2024

I see, that makes total sense! That feature feels more advanced, but i'm happy to give it a go.

Besides throwing an error if there is a duplicated label within a namespace, if we were to go that route, how would you handle a scenario such as this one? #295 (comment)

I definitely think the compiler should raise an error here, duplicate label definitions should not be allowed. Huff also won't throw an error if you accidentally declare a duplicate label as so:

#define macro A() = takes(0) returns(0) {
    first:
        sub
}

#define macro B() = takes(0) returns(0) {
    first:
        add
}

#define macro MAIN() = takes(0) returns(0) {
    A()
    B()
    first jump
}

would u raise an out-of-scope error as the user would be trying to invoke a jumpdest for a label which had been defined in another macro (namespace)?

@0xrusowsky
Copy link
Author

As suggested by @iFrostizz on discord, I’ve removed the labels hashmap and moved towards caching labels directly within the Contract struct.

Will try to work on the label namespacing over the weekend.

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.

None yet

2 participants