Skip to content

fix: patch solang-parser #10509

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

Merged
merged 11 commits into from
May 18, 2025
Merged

fix: patch solang-parser #10509

merged 11 commits into from
May 18, 2025

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented May 13, 2025

Hotpatch to foundry-solang-parser maintained at https://github.com/danipopes/solang (branch new-features) (diff) for the following fixes:

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)

@DaniPopes DaniPopes marked this pull request as ready for review May 13, 2025 15:25
@grandizzy
Copy link
Collaborator

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

@DaniPopes
Copy link
Member Author

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

@zerosnacks
Copy link
Member

zerosnacks commented May 15, 2025

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

@DaniPopes
Copy link
Member Author

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

Copy link
Member

@zerosnacks zerosnacks left a comment

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

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

👍

@DaniPopes DaniPopes merged commit dc6a216 into master May 18, 2025
42 of 44 checks passed
@DaniPopes DaniPopes deleted the dani/solang-patch branch May 18, 2025 10:15
@github-project-automation github-project-automation bot moved this to Done in Foundry May 18, 2025
@wtdcode
Copy link
Contributor

wtdcode commented May 19, 2025

Why not use foundry_compilers::artifacts::Ast?

@DaniPopes
Copy link
Member Author

DaniPopes commented May 19, 2025

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

@wtdcode
Copy link
Contributor

wtdcode commented May 19, 2025

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.

@DaniPopes DaniPopes added this to the v1.2.0 milestone May 19, 2025
@DaniPopes
Copy link
Member Author

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
* fix: patch solang-parser

* layout at test

* bump

* chore: update

* update

* format layout

* fix pragma

* chore: update

* chore: update

* fix: pragma 2

* feat: re-implement pragma
@grandizzy grandizzy mentioned this pull request May 19, 2025
3 tasks
grandizzy added a commit that referenced this pull request May 19, 2025
* fix: patch solang-parser (#10509)

* fix: patch solang-parser

* layout at test

* bump

* chore: update

* update

* format layout

* fix pragma

* chore: update

* chore: update

* fix: pragma 2

* feat: re-implement pragma

* fix(fmt): 'at' is not a keyword (#10556)

---------

Co-authored-by: DaniPopes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Forge fmt does not work with custom storage layouts 0.8.29 feat(fmt): support transient keyword by moving to Solar
4 participants