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

Private functions not overridden by other contracts #14357

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

davidBar-On
Copy link
Contributor

@davidBar-On davidBar-On commented Jun 25, 2023

Fixes #11889

Suggested fix for preventing error about overriding private functions by inheriting/other contracts. The fix is based on the previous suggested fix by PR #13328, with the enhancements required by @ekpyron in that PR to make sure during code generation that private functions are not used by other contracts. For that, a new origCallingContract item was added to FunctionCallAnnotation, that points to the contract that calls the function. This field is used to verify that a private function is called by the defining contract, instead of using mostDerivedContract, since in case of a contract calling internal library function that calls a library's private function, mostDerivedContract points to the contract and not the library.

The test cases are practically the same as suggested by in PR #13328.

@github-actions
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.

@davidBar-On davidBar-On force-pushed the issue-11889-private-functions-clash branch from fef2bfe to 85d17c6 Compare June 27, 2023 07:44
@ekpyron
Copy link
Member

ekpyron commented Jan 8, 2024

I apologize for taking our time with getting back to you on this - the test coverage now indeed looks more reassuring.
It also has codegen assertions now as a safety net.
This will still need very careful review, but I think we can work towards merging it. It will definitely need a Changelog entry to start with.

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.

Should private functions really clash ?
2 participants