fix: patch solang-parser by DaniPopes · Pull Request #10509 · foundry-rs/foundry (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation10 Commits11 Checks22 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

DaniPopes marked this pull request as ready for review

May 13, 2025 15:25

makes sense to keep us going until fully move to solar, @DaniPopes is the solang parser change something we could contribute upstream?

it's not exactly the right implementation plus unlikely to be merged/released in any reasonable time

note that this does not support more than just number literals in "layout at" due to a bug in solang-parser / lalrpop (build script runs forever if we allow any expression in that position)

Could we inform the user of this limitation when they try to format, given it is valid in the language?

Ref: https://docs.soliditylang.org/en/v0.8.29/contracts.html#custom-storage-layout

Error: Failed to parse Solidity code for src/Counter.sol. Leaving source unchanged.

Context:
- failed to parse file:
Error: ParserError
   ╭─[ :4:30 ]
   │
 4 │ contract Counter layout at 1 + 1 {
   │                              ┬  
   │                              ╰── unrecognised token '+', expected "{", "is", "layout"

[⠊] Compiling...
[⠆] Compiling 3 files with Solc 0.8.30
[⠔] Solc 0.8.30 finished in 251.13ms
Compiler run successful

Can only do that in docs since error message is specific to the parser generator implementation

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, lgtm 👍

In favor of shipping this patch to unblock, we can document the limitation in the release notes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Why not use foundry_compilers::artifacts::Ast?

The formatter was written 3 years ago with solang, and we've been slowly ripping it out in favor of solar: #9317

The formatter was written 3 years ago with solang, and we've been slowly ripping it out in favor of solar: #9317

Cool. Thanks for the instant response. solar is more promising indeed but it still lacks many side features, right? I mean why not just use existing solc ast bindings? This seems more feasible.

I haven't read a lot of code of these parts so feel free to correct me.

solc AST is just bindings and not very ergonomic to use, solar is designed specifically for use as a library and is feature complete

grandizzy pushed a commit to grandizzy/foundry that referenced this pull request

May 19, 2025