[Sol->Yul] Provide selector for some internal functions. #11015
Conversation
|
Closes #10891 |
cameel
force-pushed
the
fixConstantsCallGraph
branch
from
5dc0dce to
186d14d
Compare
chriseth
force-pushed
the
fixSelectorForInternal
branch
from
909a59a to
814fdca
Compare
test/libsolidity/semanticTests/constants/function_unreferenced.sol
Outdated
Show resolved
Hide resolved
| contract B { | ||
| function g() public {} | ||
| } | ||
| contract C is B { | ||
| bytes4 constant s2 = B.g.selector; | ||
| function f() external pure returns (bytes4) { return s2; } | ||
| } |
There was a problem hiding this comment.
I'd add more cases from #10891, maybe combined into a single test:
contract B {
function ext() external {}
function pub() public {}
}
contract C is B {
function test() public {
B.ext.selector;
B.pub.selector;
this.ext.selector;
pub.selector;
}
}
contract D {
function test() public {
B.ext.selector;
B.pub.selector;
}
}There was a problem hiding this comment.
The problem is that those cannot be done as a semantic test.
There was a problem hiding this comment.
Perhaps split into two tests
contract B {
function ext() external {}
function pub() public {}
}
contract C is B {
function test() public returns (bytes4, bytes4, bytes4, bytes4) {
return (B.ext.selector, B.pub.selector, this.ext.selector, pub.selector);
}
}
// ====
// compileViaYul: true
// ----
// test() ->
and
contract B {
function ext() external {}
function pub() public {}
}
contract D {
function test() public returns (bytes4, bytes4) {
return (B.ext.selector, B.pub.selector);
}
}
// ====
// compileViaYul: true
// ----
// test() -> 0xcf9f23b500000000000000000000000000000000000000000000000000000000, 0x7defb41000000000000000000000000000000000000000000000000000000000
There was a problem hiding this comment.
Does the tuple really trigger the same code path? I'll add a syntax test for the others just to be sure.
There was a problem hiding this comment.
Does the tuple really trigger the same code path? I'll add a syntax test for the others just to be sure.
Yes, just verified that it does.
chriseth
force-pushed
the
fixSelectorForInternal
branch
2 times, most recently
from
6ef8363 to
9e44b1c
Compare
| function g() public {} | ||
| } | ||
| contract C is B { | ||
| bytes4 constant s2 = B.g.selector; |
There was a problem hiding this comment.
Isn't this exactly the case that already worked before?
|
No changelog entry because it is sol->yul |
chriseth
force-pushed
the
fixSelectorForInternal
branch
from
9e44b1c to
5e94fce
Compare
|
Added tests. |
| @@ -0,0 +1,26 @@ | |||
| contract B { | |||
There was a problem hiding this comment.
This file does not trigger an error when run through solc --ir (and I think this is also tested for all syntax tests)
There was a problem hiding this comment.
Do we run IR generator on all syntax tests? That's good to know.
There was a problem hiding this comment.
Acutally we do not. We compile to bytecode, but not via yul. I just tried it and it is kind of stuck in the optimizer :(
There was a problem hiding this comment.
Do we run IR generator on all syntax tests? That's good to know.
Not yet.
There was a problem hiding this comment.
Just ran the IR on all syntax tests and found one unimplemented feature: #11024
There was a problem hiding this comment.
We did that a few weeks ago already (it used to be more like 9 cases or so), so that issue may be a duplicate...
There was a problem hiding this comment.
I couldn't find an equivalent issue. We probably missed that one. this was the CI job https://app.circleci.com/pipelines/github/ethereum/solidity/13394/workflows/42f7aa23-443b-4847-be5e-b37f8af856b3/jobs/621452/tests#failed-test-2
There was a problem hiding this comment.
Weird... I do remember that we were wondering if it actually works and that I noted that it's a bit weird even in old codegen... but maybe we still didn't do anything about it or even file an issue :-).
Depends on #11014
References #10905
Closes #10891