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

Calldata doc #15533

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Calldata doc #15533

merged 1 commit into from
Nov 11, 2024

Conversation

haoyang9804
Copy link
Contributor

Reference: #15483 (comment). I found an awkward scenario in using calldata struct/array in function bodies. Daniel pointed out (#15483 (comment)) that this is a workaround to allow complex initialization patterns but may not be well documented. This pr is for enriching the doc to mitigate users' confusion.

Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

@haoyang9804
Copy link
Contributor Author

@cameel Hi Kamil, could you help with the review? thx

@haoyang9804
Copy link
Contributor Author

@ekpyron Hi Daniel, would you mind helping review this pr? thx

@ekpyron
Copy link
Member

ekpyron commented Oct 29, 2024

We discussed this a bit more internally in response to your issue (and also #14021 and the fact that this also came up in the Solidity underhanded contest) and concluded that we will start emitting a warning for this pattern (and with the next breaking release disallow it entirely).

Originally, when this pattern was allowed for allowing complex initialization patterns, we did not yet have assembly access to storage or calldata pointers. While discussing this again, we realized that today, complex initialization patterns can also be allowed by an even more explicit and conscious

    S calldata s;
    assembly { s.offset := 0 }
    ... complex initialization pattern ...

(or similarly in the storage case)

    S storage s;
    assembly { s.slot := 0 }
    ... complex initialization pattern ...

which makes it absolutely clear what happens at the user's peril.

We can't emit an error for the s = s; pattern right away, since people may still use it as a workaround, but we'll warn about it and stage it for becoming an error in the future.

I'd still be fine with merging this as docs improvement for now, but wanted to make it clear that this will become obsolete again soon.

@haoyang9804
Copy link
Contributor Author

We discussed this a bit more internally in response to your issue (and also #14021 and the fact that this also came up in the Solidity underhanded contest) and concluded that we will start emitting a warning for this pattern (and with the next breaking release disallow it entirely).

Originally, when this pattern was allowed for allowing complex initialization patterns, we did not yet have assembly access to storage or calldata pointers. While discussing this again, we realized that today, complex initialization patterns can also be allowed by an even more explicit and conscious

    S calldata s;
    assembly { s.offset := 0 }
    ... complex initialization pattern ...

(or similarly in the storage case)

    S storage s;
    assembly { s.slot := 0 }
    ... complex initialization pattern ...

which makes it absolutely clear what happens at the user's peril.

We can't emit an error for the s = s; pattern right away, since people may still use it as a workaround, but we'll warn about it and stage it for becoming an error in the future.

I'd still be fine with merging this as docs improvement for now, but wanted to make it clear that this will become obsolete again soon.

No problem. When the error handling is done, you can let me know and I will remove this doc. This update is only useful for a while.

Copy link
Collaborator

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

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

I think this is good now. But we should rebase and squash the commits. Can you do that please, @haoyang9804 ?

docs/contracts/visibility-and-getters.rst Outdated Show resolved Hide resolved
docs/types/reference-types.rst Outdated Show resolved Hide resolved
@haoyang9804
Copy link
Contributor Author

I think this is good now. But we should rebase and squash the commits. Can you do that please, @haoyang9804 ?

Done. Seems that all redundant commits have been rebased into one.

@haoyang9804
Copy link
Contributor Author

@matheusaaguiar , hi, can you help with the merge?

@matheusaaguiar
Copy link
Collaborator

@haoyang9804 , can you rebase on top of latest develop branch instead of merge, please?

@haoyang9804
Copy link
Contributor Author

@matheusaaguiar The rebasing is done and all checks have passed. plz take a look.

@matheusaaguiar matheusaaguiar merged commit b0d991b into ethereum:develop Nov 11, 2024
73 checks passed
high089123

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants