huff-rs icon indicating copy to clipboard operation
huff-rs copied to clipboard

chore: throw error with duplicated destination labels

Open 0xrusowsky opened this issue 6 months ago • 4 comments

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.

0xrusowsky avatar Dec 26 '23 22:12 0xrusowsky

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 avatar Jan 11 '24 00:01 Maddiaa0

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

Maddiaa0 avatar Jan 11 '24 00:01 Maddiaa0

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? https://github.com/huff-language/huff-rs/issues/295#issuecomment-1745064630

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 avatar Jan 11 '24 07:01 0xrusowsky

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.

0xrusowsky avatar Jan 16 '24 20:01 0xrusowsky