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

Getting Yul ASTs requires asking for ast at contract level instead of at file level #14452

Open
haltman-at opened this issue Jul 26, 2023 · 2 comments
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it.

Comments

@haltman-at
Copy link
Contributor

Description

When compiling in Yul mode, including "ast" in the file-level output selection doesn't result in getting ASTs like it does when compiling Solidity. Instead you have to set it at the contract level. This makes no sense, because ASTs are a file-level matter, and is also inconsistent with how the compiler acts when compiling Solidity.

Environment

  • Compiler version: 0.8.21
  • Target EVM version (as per compiler settings): default

Steps to Reproduce

Really any Yul code will do here! What's relevant here is the output selection. An outputSelection of

{
  "*": {
    "": ["ast"],
    "*": [
      "evm.bytecode.object"
    ]
  }
}

won't get the AST,

whereas one of


{
  "*": {
    "": [],
    "*": [
      "ast",
      "evm.bytecode.object"
    ]
  }
}

will.

@r0qs
Copy link
Member

r0qs commented Jul 28, 2023

Thanks @haltman-at, yeah, the Yul is generated per contract and, currently, only one Yul object can be compiled at the same time (so the ID would be always the same), whereas the solidity AST is cross contract. We may need to change that indeed and include ast in the file-level for Yul as well.

@r0qs r0qs added the must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. label Jul 28, 2023
@cameel
Copy link
Member

cameel commented Jul 31, 2023

Yeah, this is definitely a bug in #14177. In Yul there's always one contract per file, so you could technically associate the AST with a contract, but it's really the source unit that has an AST. The original PR confusingly made it a contract-level output so I pointed it out back then and it was changed, but looks like this was only done for the output JSON.

Note that this was really meant to be an experimental feature (even though it looks like we did not make that very clear and even included a changelog entry for it). As such it wasn't very thoroughly reviewed or tested. The intention was to merge it quickly. If anything else about it is off, please do report it :)

@cameel cameel added low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it.
Projects
None yet
Development

No branches or pull requests

3 participants