Properly check data location in inheritance. #12850

chriseth · 2022-03-24T14:03:25Z

~~Depends on #13027~~. Merged.
Fixes #10900

Note that the bug is also relevant for the return types.

chriseth · 2022-03-24T16:30:52Z

Patch for openzeppelin (only one contract affected): OpenZeppelin/openzeppelin-contracts#3293

chriseth · 2022-03-24T17:17:28Z

Blog post: ethereum/solidity-blog#173

cameel · 2022-03-24T18:11:00Z

Patch for Gnosis Safe: safe-global/safe-smart-account#394 (also only one contract affected)

And Bleeps is only affected through the Governor contract from OpenZeppelin.

cameel · 2022-03-24T19:04:05Z

I just pushed a commit with a workaround for this issue in Gnosis, Zeppelin and Bleeps. It's a simple replacement of the affected lines. We can just drop it from the PR if they fix the problem quickly or merge it if they don't.

leonardoalt · 2022-04-04T12:44:24Z

Tests are failing. Is it just rebase or are there problems in the PR?

cameel · 2022-04-04T14:36:08Z

chk_errorcodes probably just needs a small tweak. I haven't reviewed the PR yet though so I don't know how complete it is.

As for external tests, they are failing because this change is technically not backwards-compatible (we're treating it as a bugfix). @chriseth took care of submitting fixes to OZ and ENS (ensdomains/ens-contracts#65), I did Gnosis. Bleeps breakage turned out to be coming from the OZ dependency.

The only reason why Gnosis, OZ and Bleeps passed here was that I added a quick and dirty workaround in case this does not get fixed upstream quickly. I did not do ENS because I did not investigate that one.

Anyway, looks like OZ and ENS merged our PRs. The only remaining one is Gnosis (zero reaction so far). So I guess I'll remove the workarounds for everything except Gnosis and ext tests should pass. Other than that, this still needs the fix for chk_errorcode and a review.

cameel · 2022-04-05T15:39:15Z

I just reverted workarounds for OZ and Bleeps. Looks like my PR will be merged in Gnosis soon and then we'll be able to remove that one too.

cameel · 2022-04-07T19:47:04Z

Looks like for ENS we'll actually have to switch to the master branch. We're currently at a tag but they don't have too many tags and the fix is only in master.

And for Bleeps we'll have to wait for an OZ release (or hard-code it to use a pre-release). A new OZ release is currently in an open review period, which will end around April 22nd.

aarlt · 2022-04-27T15:34:28Z

libsolidity/analysis/OverrideChecker.cpp

+{
+	return std::visit(GenericVisitor{
+		[&](FunctionDefinition const* _item) { return TypeProvider::function(*_item); },
+		[&](VariableDeclaration const*) -> FunctionType const* { solAssert(false, "Requested specifig function type of variable."); return nullptr; },


"specifig" -> "specific"

cameel · 2022-04-27T16:13:59Z

Current status regarding external tests:

ENS: there's a fix upstream but we're using an earlier, tagged version. Because of this there was an extra commit here to switch ENS from that tag to master branch. It won't pass otherwise. This commit has to be restored (I still have it locally).
Bleeps: Turns out it's actually hard-coded for OZ 4.3.2 so it does not pick up the latest 4.6.0 despite the fact that we're removing package.lock. So we need to force the newer one in bleeps.sh:
```
npm install @openzeppelin/contracts@>=4.6.0
```
- The problem is that on 4.6.0 there's an unrelated error:
```
DeclarationError: Identifier not found or not unique.
  --> src/bleepsdao/BleepsDAOGovernor.sol:31:9:
   |
31 |         ERC20Votes _token,
   |         ^^^^^^^^^^
```
  Maybe ERC20Votes was moved to a different location in one of the newer versions of OZ. Or it used to be imported by some transitive dependency and now no longer is. Unfortunately I think we'll have to patch that ourselves.
Brink: this is a new test and we have not reported the problem there. Probably best to patch it on our side now.
Gnosis: My upstream PR has not been merged yet so patching on our side this is still needed.

cameel · 2022-04-28T11:19:07Z

The ext tests should all be passing now. I restored the ENS and Gnosis workarounds and added a new one for Brink (also submitted a PR upstream: brinktrade/brink-core#52).

For Bleeps it turns out that updating to OZ 4.6.0 and patching the single compilation error that appears after that makes a ton of other compilation errors pop up. So I gave up, kept it at OZ 4.3.2 and just copied the patch command we had here for OZ formerly.

cameel · 2022-04-28T13:22:06Z

ENS is now failing. My commit updates it to the latest version and in that version apparently they have some low-level functions for doing reverts (LowLevelCallUtils.propagateRevert()) or just accessing the returndata (LowLevelCallUtils.readReturnData()).

UniversalResolver.callWithOffchainLookupPropagation() uses them like this:

        bool result = LowLevelCallUtils.functionStaticCall(target, data);
        uint256 size = LowLevelCallUtils.returnDataSize();

        if(result) {
            return LowLevelCallUtils.readReturnData(0, size);
        }

So apparently #12860 does break some things (though in their case fix is trivial, just make these functions view).

cameel · 2022-04-28T13:25:26Z

Just to be clear, this is not caused directly by this PR, merging #12860 broke it. It only shows up here due to the ENS update (which we do to get an upstream fix for the override issue and avoid having to do even more patching in external tests).

ekpyron · 2022-05-11T10:19:15Z

docs/bugs.json

+        "description": "TODO",
+        "link": "TODO",


Those still need to be filled in I guess - apart from that the fix generally looks good.

chriseth · 2022-05-12T08:32:01Z

Since we plan to revert #12860 that should fix the ens test breakage.

ekpyron · 2022-05-12T11:23:42Z

libsolidity/analysis/OverrideChecker.cpp

+			_overriding.isFunction() &&
+			!returnTypesDifferAlready &&
+			_super.visibility() != Visibility::External &&
+			_overriding.functionKind() != Token::Fallback


Is this really necessary? Doesn't fallback imply external visibility (both of _overriding and of _super)?
Not that it hurts in any case.

Yes, maybe, but we can also just leave it in...

cameel

I have a few more suggestions but overall the PR seems correct. I haven't found any serious problems so I'm approving already.

libsolidity/analysis/OverrideChecker.h

test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location_illegal.sol

test/libsolidity/syntaxTests/inheritance/dataLocation/memory_calldata_public.sol

libsolidity/analysis/OverrideChecker.cpp

test/libsolidity/syntaxTests/inheritance/dataLocation/memory_calldata_internal.sol

Changelog.md

cameel

Looks fine overall. There's just one last small inconsistency in #12850 (comment)

cameel · 2022-05-16T15:15:34Z

You can rebase this on #13027 if you want to be sure all external tests pass.

chriseth · 2022-05-16T15:34:47Z

Rebased, so please don't merge before #13027

…patches are accepted upstream

chriseth force-pushed the dataLocationInInheritance branch from 8b9e0b7 to 255cb42 Compare March 24, 2022 14:46

chriseth changed the title ~~Tests for changing data location in inheritance.~~ Properly check data location in inheritance. Mar 24, 2022

cameel force-pushed the dataLocationInInheritance branch from dd68761 to 52d2b04 Compare April 5, 2022 15:37

chriseth force-pushed the dataLocationInInheritance branch 6 times, most recently from cac39b5 to aeeede5 Compare April 27, 2022 14:29

aarlt reviewed Apr 27, 2022

View reviewed changes

cameel force-pushed the dataLocationInInheritance branch from 3ab96c4 to c45e4dc Compare April 28, 2022 11:15

chriseth force-pushed the dataLocationInInheritance branch from c45e4dc to 2d5e30c Compare April 28, 2022 12:02

chriseth force-pushed the dataLocationInInheritance branch from 2d5e30c to c7d88cb Compare May 2, 2022 10:27

ekpyron reviewed May 11, 2022

View reviewed changes

chriseth force-pushed the dataLocationInInheritance branch 2 times, most recently from fda987b to 41e1d2b Compare May 12, 2022 08:45

ekpyron reviewed May 12, 2022

View reviewed changes

cameel previously approved these changes May 12, 2022

View reviewed changes

chriseth dismissed cameel’s stale review via f50cf60 May 16, 2022 13:30

chriseth force-pushed the dataLocationInInheritance branch 3 times, most recently from c200c89 to bb13124 Compare May 16, 2022 14:20

cameel previously approved these changes May 16, 2022

View reviewed changes

chriseth dismissed cameel’s stale review via e91548e May 16, 2022 14:51

chriseth force-pushed the dataLocationInInheritance branch from bb13124 to e91548e Compare May 16, 2022 14:51

cameel previously approved these changes May 16, 2022

View reviewed changes

chriseth dismissed cameel’s stale review via f95015f May 16, 2022 15:34

chriseth force-pushed the dataLocationInInheritance branch from e91548e to f95015f Compare May 16, 2022 15:34

chriseth changed the base branch from develop to ethersproject-contracts-update-workaround May 16, 2022 15:34

cameel added the has dependencies The PR depends on other PRs that must be merged first label May 16, 2022

cameel force-pushed the ethersproject-contracts-update-workaround branch 2 times, most recently from 778a127 to adf3eaa Compare May 17, 2022 08:57

Base automatically changed from ethersproject-contracts-update-workaround to develop May 17, 2022 10:59

chriseth and others added 5 commits May 17, 2022 13:02

More strict override check for data locations.

dfa0bcf

Patch external tests for the override data alignment issue until our …

27e5afa

…patches are accepted upstream

Switch ENS external test to master branch

1164d1b

Changelog.

259edd4

Bug list entry.

f427247

chriseth force-pushed the dataLocationInInheritance branch from f95015f to f427247 Compare May 17, 2022 11:04

ekpyron approved these changes May 17, 2022

View reviewed changes

cameel removed the has dependencies The PR depends on other PRs that must be merged first label May 17, 2022

cameel approved these changes May 17, 2022

View reviewed changes

chriseth merged commit c5cc553 into develop May 17, 2022

chriseth deleted the dataLocationInInheritance branch May 17, 2022 11:48

r0qs mentioned this pull request Oct 18, 2023

Compiler bug virtual/override argument location specifcation #14603

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly check data location in inheritance. #12850

Properly check data location in inheritance. #12850

chriseth commented Mar 24, 2022 •

edited by cameel

Loading

chriseth commented Mar 24, 2022

chriseth commented Mar 24, 2022

cameel commented Mar 24, 2022

cameel commented Mar 24, 2022

leonardoalt commented Apr 4, 2022

cameel commented Apr 4, 2022

cameel commented Apr 5, 2022

cameel commented Apr 7, 2022 •

edited

Loading

aarlt Apr 27, 2022

cameel commented Apr 27, 2022 •

edited

Loading

cameel commented Apr 28, 2022

cameel commented Apr 28, 2022

cameel commented Apr 28, 2022

ekpyron May 11, 2022

chriseth commented May 12, 2022

ekpyron May 12, 2022 •

edited

Loading

chriseth May 12, 2022

cameel left a comment

cameel left a comment

cameel commented May 16, 2022

chriseth commented May 16, 2022

Properly check data location in inheritance. #12850

Properly check data location in inheritance. #12850

Conversation

chriseth commented Mar 24, 2022 • edited by cameel Loading

chriseth commented Mar 24, 2022

chriseth commented Mar 24, 2022

cameel commented Mar 24, 2022

cameel commented Mar 24, 2022

leonardoalt commented Apr 4, 2022

cameel commented Apr 4, 2022

cameel commented Apr 5, 2022

cameel commented Apr 7, 2022 • edited Loading

aarlt Apr 27, 2022

Choose a reason for hiding this comment

cameel commented Apr 27, 2022 • edited Loading

cameel commented Apr 28, 2022

cameel commented Apr 28, 2022

cameel commented Apr 28, 2022

ekpyron May 11, 2022

Choose a reason for hiding this comment

chriseth commented May 12, 2022

ekpyron May 12, 2022 • edited Loading

Choose a reason for hiding this comment

chriseth May 12, 2022

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

cameel commented May 16, 2022

chriseth commented May 16, 2022

chriseth commented Mar 24, 2022 •

edited by cameel

Loading

cameel commented Apr 7, 2022 •

edited

Loading

cameel commented Apr 27, 2022 •

edited

Loading

ekpyron May 12, 2022 •

edited

Loading