Skip to content

Conversation

ggiraldez
Copy link
Contributor

@ggiraldez ggiraldez commented Mar 25, 2025

Fix resolving identifiers in arguments to inheritance specifiers, eg. contract Test is Base(SOME_CONSTANT) {}. Arguments are resolved in the parent scope of the contract, ie. the source unit lexical scope.

Also fix that storage layout expressions should be resolved in the contract's parent scope, not the contract's lexical scope itself.

Added some snapshot test cases to verify that cycles in contract hierarchies and recursive structs are bound correctly, even though they are invalid in Solidity.

Closes #1289

Copy link

changeset-bot bot commented Mar 25, 2025

🦋 Changeset detected

Latest commit: bcb00c0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/slang Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

LGTM and to CI, why is it a draft?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but the definiens section looks scrambled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leading trivia makes the output look scrambled, but it seems to be ok.

@ggiraldez
Copy link
Contributor Author

LGTM and to CI, why is it a draft?

Waiting for #1288 to be merged to add a similar rule for the expressions in storage layout specifiers.

@OmarTawfik
Copy link
Contributor

@ggiraldez #1288 has been merged. Thank you!

@ggiraldez ggiraldez changed the title Add binding rule to resolve arguments in inheritance specifiers Resolve arguments in inheritance specifiers and fix scope for expressions in storage layout specifiers Mar 26, 2025
@ggiraldez ggiraldez marked this pull request as ready for review March 26, 2025 19:04
@ggiraldez ggiraldez requested review from a team as code owners March 26, 2025 19:04
@@ -0,0 +1,11 @@
uint constant BASE = 42;

contract Base {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: splitting such tests into two (positive that should bind, and negative that shouldn't bind).
This will make it easier to review/detect bugs in the future.

@@ -2,3 +2,7 @@ uint256 constant C1 = 0;

contract X1 layout at C1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: splitting such tests into two (positive that should bind, and negative that shouldn't bind).
This will make it easier to review/detect bugs in the future.

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

Left a suggestion on splitting tests. Otherwise LGTM. Thanks!

@ggiraldez ggiraldez enabled auto-merge March 27, 2025 21:25
@ggiraldez ggiraldez added this pull request to the merge queue Mar 27, 2025
Merged via the queue into NomicFoundation:main with commit da1f863 Mar 27, 2025
2 checks passed
@ggiraldez ggiraldez deleted the fix/1289 branch March 27, 2025 22:08
@github-actions github-actions bot mentioned this pull request Mar 27, 2025
github-merge-queue bot pushed a commit that referenced this pull request Apr 10, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and publish to npm
yourself or [setup this action to publish
automatically](https://github.com/changesets/action#with-publishing). If
you're not ready to do a release yet, that's fine, whenever you add more
changesets to main, this PR will be updated.


# Releases
## @nomicfoundation/slang@1.1.0

### Minor Changes

- [#1288](#1288)
[`2090ab8`](2090ab8)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support Solidity
`0.8.29` and [Custom Storage
Layouts](https://docs.soliditylang.org/en/v0.8.29/contracts.html#custom-storage-layout):

- `ContractDefinition` nodes will no longer have an optional
`InheritanceSpecifier` child directly, but will hold a list of
`ContractSpecifier` children
- `ContractSpecifier` nodes have either `InheritanceSpecifier` or
`StorageLayoutSpecifier` children

- [#1265](#1265)
[`2312260`](2312260)
Thanks [@mjoerussell](https://github.com/mjoerussell)! - Add
`LanguageUtils::infer_language_versions(source_code) -> Version[]` API,
which will analyze version pragmas inside a source file, and return a
list of supported language versions that they allow. This can be used to
select a valid language version to use with the rest of Slang APIs.
Please see the [Choosing a Solidity
Version](https://nomicfoundation.github.io/slang/1.1.0/user-guide/04-getting-started/02-choosing-a-solidity-version/)
guide for more information.

### Patch Changes

- [#1291](#1291)
[`da1f863`](da1f863)
Thanks [@ggiraldez](https://github.com/ggiraldez)! - Resolve arguments
to inheritance specifiers and expressions in storage layout specifiers
using the contract's parent scope.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

contract/interface specifiers lexical scope should be parent instead
3 participants