Skip to content

Conversation

@nikola-matic
Copy link
Collaborator

@nikola-matic nikola-matic commented Apr 28, 2023

Note: this is still a work in progress

  • Changelog entry (pragma, ast)
  • Decide feature name for the experimental pragma (current placeholder is next) (decision pragma experimental solidity)
  • Change pragma from next to solidity
  • Resolve imports in CompilerStack

Fixes: #10639 (comment)

@nikola-matic nikola-matic self-assigned this Apr 28, 2023
@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch 3 times, most recently from cf299e4 to 57c3e8b Compare May 5, 2023 09:39
@nikola-matic nikola-matic changed the title Introduce solidity-next pragma Introduce pragma experimental solidity May 5, 2023
@nikola-matic nikola-matic marked this pull request as ready for review May 5, 2023 12:54
Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

I'd change the changelog phrasing and would still change the flag name and not always emit it in the json AST, but only when it's true.
Apart from that mostly optional comments and almost good to go.
Testing is good enough (if you want, you can still add tests for it occurring twice in the beginning and once in the beginning plus once later on - but since that's all covered by the preexisting logic, it's also fine if not - the same goes for the experimental flag in CBOR metadata: could be tested specifically, but it's already tested for any experimental pragma, so also fine)

Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

Nice! Just some questions :)

@nikola-matic
Copy link
Collaborator Author

Testing is good enough (if you want, you can still add tests for it occurring twice in the beginning and once in the beginning plus once later on - but since that's all covered by the preexisting logic, it's also fine if not - the same goes for the experimental flag in CBOR metadata: could be tested specifically, but it's already tested for any experimental pragma, so also fine)

@ekpyron precisely why I didn't add such tests, as they would be completely redundant, i.e. adding them for experimental solidity covers the same path as experimental ABIEncoderV2, or SMTChecker.

@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch from 619fc81 to 97df8a8 Compare May 5, 2023 21:10
@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch from 97df8a8 to 8220f35 Compare May 8, 2023 12:11
Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Only minor adjustments - if you fix the comments above and squash, we can merge today.

Also needs a rebase, though, and the Changelog entries needs to move to 0.8.21.

@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch from 8220f35 to b1718e7 Compare May 15, 2023 11:42
@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch from b1718e7 to 6d35235 Compare May 15, 2023 12:47
ekpyron
ekpyron previously approved these changes May 15, 2023
@ekpyron ekpyron dismissed their stale review May 15, 2023 12:53

Premature approval.

@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch from 6d35235 to 363a76f Compare May 15, 2023 12:53
@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch from 363a76f to 3e88419 Compare May 15, 2023 14:06
r0qs
r0qs previously approved these changes May 15, 2023
ekpyron
ekpyron previously approved these changes May 15, 2023
Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Minor cosmetics left, if you please, but generally good to merge.

Exclude pragma experimental error from ANTLR tests

Test for first pragma after non-pragma declaration

Resolve import pragmas

Change pragma name from next to solidity

Add Changelog entries

Address review comments
@nikola-matic nikola-matic dismissed stale reviews from ekpyron and r0qs via 8a41f4a May 15, 2023 17:25
@nikola-matic nikola-matic force-pushed the pragma-solidity-next branch from 3e88419 to 8a41f4a Compare May 15, 2023 17:25
@ekpyron ekpyron enabled auto-merge May 15, 2023 17:27
@ekpyron ekpyron merged commit 1250ee7 into develop May 15, 2023
@ekpyron ekpyron deleted the pragma-solidity-next branch May 15, 2023 18:18
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.

5 participants