Clean up visibility via contract name and unimplemented base functions. #8137
Conversation
ekpyron
force-pushed
the
declarationsByContractName
branch
from
4e6e47c to
4a3a67a
Compare
ekpyron
changed the title
ekpyron
force-pushed
the
declarationsByContractName
branch
from
4a3a67a to
1f0c34d
Compare
ekpyron
changed the title
ekpyron
force-pushed
the
declarationsByContractName
branch
from
6c8106e to
cf0a9d3
Compare
| registerDeclaration(*m_scopes[m_currentScope], _declaration, nullptr, nullptr, warnAboutShadowing, inactive, m_errorReporter); | ||
|
|
||
| _declaration.annotation().scope = m_currentScope; | ||
| _declaration.annotation().contract = m_currentContract; |
There was a problem hiding this comment.
To be honest, I think we should add a function to the annotation (or a utility function) to determine the contract from the scope instead of storing it.
There was a problem hiding this comment.
Maybe... it's not like I added it - I just moved it from the annotations of function definitions to declarations - which wouldn't even have been necessary for this PR, but if we have the annotation, then this is the better place for it...
| } | ||
| bool isVisibleViaContractName() const override | ||
| { | ||
| return visibility() >= Visibility::Public; |
There was a problem hiding this comment.
We could actually allow internal ones... but they would be empty objects with no use... they couldn't be called and don't have a signature and they can't be assigned to anything...
There was a problem hiding this comment.
Isn't it possible to call them internally?
There was a problem hiding this comment.
contract A {
function f() internal virtual pure returns (uint) {return 2; }
}
contract B is A {
function f() internal virtual override pure returns (uint) {return 3; }
}
contract C is B {
function g() public pure returns (uint) {
return A.f();
}
}
There was a problem hiding this comment.
That's isVisibleInDerivedContract not this one... but yeah, I'm not sure how to be clearer in the distinction - I renamed it to isVisibleViaContractTypeAccess, maybe that helps, but arguably that's still also what happens in the inheritance case... it's only meant to return true, in case something can be accessed via the contract type name in any context, not merely when inheriting.
| functionDefinition && | ||
| _scope && | ||
| functionDefinition->annotation().contract && | ||
| _scope != functionDefinition->annotation().contract && |
There was a problem hiding this comment.
Maybe this could warrant a different function kind to be honest...
There was a problem hiding this comment.
Like Public? Maybe... not sure if I should do that now, though... I already spent way too much time on this - and I'm not particularly happy with it myself yet, but I'm not sure how much more time we should spend on refactoring this right now...
The problem is that I'd probably need two more kinds PublicWithInternalCallingContext and PublicWithExternalCallingContext... in general it would probably be good to decouple the function type or at least the function type kind from the calling context a bit... but that would require major code changes...
| members.emplace_back(stru->name(), stru->type(), stru); | ||
| for (auto const& enu: contract.definedEnums()) | ||
| members.emplace_back(enu->name(), enu->type(), enu); | ||
| if (dynamic_cast<ModifierDefinition const*>(declaration)) |
There was a problem hiding this comment.
Modifiers are not accessible via contract name...
| } | ||
| // ---- | ||
| // TypeError: (100-106): Member "f" not found or not visible after argument-dependent lookup in type(contract base). | ||
| // TypeError: (100-108): Cannot call function via contract name. |
There was a problem hiding this comment.
Is this an error message we had previously?
There was a problem hiding this comment.
It's the one I added for #8105 .
And I'd say it fits much better here than the old one.
test/libsolidity/syntaxTests/types/contractTypeType/members/base_contract_invalid.sol
Outdated
Show resolved
Hide resolved
|
Ok, I think I get the idea behind this better now that I read all of it :) |
|
Please squash! |
…ented base function.
ekpyron
force-pushed
the
declarationsByContractName
branch
from
e5e9bdd to
ee5ff4d
Compare
|
Squashed. I hope you're confident that this really does the same as before in all cases in which it should :-). I think we're still lacking test cases in this area... |
Fixes #7314 and closes #7987 .