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

Syntax to refer to a specific overload of a function #3556

Open
axic opened this issue Feb 20, 2018 · 29 comments
Open

Syntax to refer to a specific overload of a function #3556

axic opened this issue Feb 20, 2018 · 29 comments
Labels
language design :rage4: Any changes to the language, e.g. new features medium effort Default level of effort medium impact Default level of impact must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. needs design The proposal is too vague to be implemented right away

Comments

@axic
Copy link
Member

axic commented Feb 20, 2018

pragma experimental "v0.5.0";

contract Selector {
    function transfer(address, uint) external;
    function transfer(address) external;
    function calculateSelector() public pure returns (bytes4) {
        return this.transfer.selector;
    }
}

Not sure what could be a good notation and/or a generic way for this. As a workaround a function type can be defined.

Split off #3544.

EDIT by @cameel: Renamed the issue since we often defer to it as the main issue about resolving ambiguous overloads. The original title was Support retrieving selector of an overloaded function.

@k06a
Copy link

k06a commented Jan 31, 2019

Any progress?

@chriseth
Copy link
Contributor

chriseth commented Feb 4, 2019

Sorry, no progress. Do you have any suggestion for the syntax?

@k06a
Copy link

k06a commented Feb 4, 2019

@chriseth, sure:

this.transfer(address,uint256).selector
this.transfer(address,uint256,bytes).selector
this.balanceOf.selector
address.transfer(uint256).selector

@axic axic added the language design :rage4: Any changes to the language, e.g. new features label Feb 4, 2019
@Marenz
Copy link
Contributor

Marenz commented Mar 25, 2019

Isn't this issue very similar to #526? Maybe whatever solution will be found can be used for both

@chriseth
Copy link
Contributor

Yep, sounds similar!

@axic
Copy link
Member Author

axic commented Jan 25, 2021

The workaround:

contract Selector {
    function transfer(address, uint) external {}
    function transfer(address) external {}
    function calculateSelector() public pure returns (bytes4) {
        function (address) external t;
        return t.selector;
    }
}

But assigning the correct function to the t is not possible (due to overloading resolution lacking), hence type checking can't be enforced here. So not sure if this workaround is better than just doing keccak manually.

@cameel
Copy link
Member

cameel commented Jun 2, 2021

Should we remove the bug label from this one? To me it looks more like a missing feature. I.e. there's nothing broken, just the right syntax is missing.

@chriseth chriseth removed the bug 🐛 label Jun 3, 2021
@chriseth
Copy link
Contributor

chriseth commented Jun 3, 2021

Yes, not a bug.

@cleanunicorn
Copy link

The workaround:

contract Selector {
    function transfer(address, uint) external {}
    function transfer(address) external {}
    function calculateSelector() public pure returns (bytes4) {
        function (address) external t;
        return t.selector;
    }
}

But assigning the correct function to the t is not possible (due to overloading resolution lacking), hence type checking can't be enforced here. So not sure if this workaround is better than just doing keccak manually.

However, calling calculateSelector() returns 0x00000000.

image

Maybe I don't understand how this should be used.

@ekpyron
Copy link
Member

ekpyron commented Jan 18, 2022

IIUC it's not really a working workaround. The idea was that function (address) external t = this.transfer; return t.selector; could be a workaround, if it worked (but it doesn't)...
I'm not sure there's a really good workaround at this point. What you can always do, though, is to define an abstract auxiliary contract in which the function is not overloaded (or calculate the hash manually with keccak256), e.g.

abstract contract Selector2 {
    function transfer(address, uint) external virtual;
}
contract Selector {
    function transfer(address, uint) external {}
    function transfer(address) external {}
    function calculateSelector() public pure returns (bytes4) {
        return Selector2.transfer.selector;
    }
}

But we should probably try to solve this issue properly.

@k06a
Copy link

k06a commented Jan 18, 2022

@chriseth so what about the proposed synthax? #3556 (comment)

@0xmikko
Copy link

0xmikko commented Jan 18, 2022

Existing syntax create ambiguity, which could be a source of potential errors. Providing arguments looks as remedy for such errors, as it was mentioned above:

this.transfer(address,uint256).selector
this.transfer(address,uint256,bytes).selector

@k06a
Copy link

k06a commented Jan 18, 2022

@MikaelLazarev moreover we already have similar syntax for abi.decode:

abi.decode(data, (address,uint256,bytes))

@cameel
Copy link
Member

cameel commented Jan 18, 2022

so what about the proposed synthax? #3556 (comment)

@chriseth is off now so we won't get an answer from him until February but for me this syntax looks ambiguous.

In the code below, would this.g(Enum) be a function call or a reference to a specific overload of the function?

contract C {
    enum Enum { A, B, C }

    function g(Enum e) public {}
    function g(uint i) public {}

    function f() public {
        uint Enum = 42;
        this.g(Enum).selector; // ambiguous
    }
}

I think we we need different syntax for this. Here are some ideas:

  1. If you want to follow abi.decode(), then it could be something like this, without introducing anything new:
    abi.resolve(this.transfer, (address, uint256, bytes))(0x123, 0, "abc");
    abi.resolve() suggests that it would work only for external functions so we may need a better name for such a builtin.
  2. Or we could just add something to distinguish it from a call. Best if it's not an existing operator so it cannot be interpreted as an expression:
    this.transfer@(address, uint256, bytes)(0x123, 0, "abc");
    or
    this.transfer::(address, uint256, bytes)(0x123, 0, "abc");
    or
    (this.transfer as (address, uint256, bytes))(0x123, 0, "abc");
  3. We could also try to make it look like a cast. Current cast syntax won't work but with Conversions revamped #11284 it could look like this:
    resolve<address, uint256, bytes>(this.transfer)(0x123, 0, "abc");
    or
    cast<function(address, uint256, bytes) external>(this.transfer)(0x123, 0, "abc");
  4. Another idea would be to reuse array brackets. It's not ambiguous in terms of syntax but might be confusing to people not used to it:
    this.transfer[address, uint256, bytes](0x123, 0, "abc");

@k06a
Copy link

k06a commented Jan 18, 2022

@cameel why it should look ambiguous if the param list contains types instead of expressions/values?

@cameel
Copy link
Member

cameel commented Jan 18, 2022

Type names are not necessarily reserved keywords. They can be just normal identifiers. Well, technically the ambiguity can be resolved but either way of doing it prevents you from doing something: in my example if you treat the type name as shadowed by a variable, it prevents you from being able to get a pointer to g(Enum). If you ignore shadowing and always treat it as a type, you cannot call g(Enum).

Also, it's IMO not great for humans either. With contract, struct, enum names, etc. it might not be clear at all that what you're looking at is a function pointer and not a function call. I'd rather have clearer syntax for that.

@cameel
Copy link
Member

cameel commented Jan 18, 2022

Unless you mean that appending .selector at the end makes it unambiguous that you're not referring to a call. But I think such context-dependent constructs unnecessarily complicate the language. It would require special-casing the parsing just for this and people would also need to know that it's a weird exception that breaks the general pattern. Adding new syntax for this seems much simpler to me.

@0xmikko
Copy link

0xmikko commented Jan 19, 2022

@cameel Does the same problem with type names and variable names exist with abi.decode method, when you will provide Enum as parameter in types list: abi.decode(this.transfer, (Enum))? It looks like a little bit artificial, and as developer you can simply eliminate it by naming things properly.

Adding abi.resolve function also makes things more complex, cause selector is bytes4 constant, when abi.encode / abi.decode are functions which will be compiled into bytecode.

@ekpyron
Copy link
Member

ekpyron commented Jan 19, 2022

Just to try to elaborate on the concern about the syntax in #3556 (comment):
Technically, in solidity types are regular expressions of type "type type" :-). That's e.g. how abi.decode(..., <tuple of types>) works - that's not custom magic syntax for abi.decode, but the tuple of types is a valid expression everywhere. You can also validly do e.g. (int, uint); as a statement in solidity :-).
That's mostly a technical detail at this point, since "type types" cannot be named in solidity. But having types as expressions may (or may not) be useful for generics/templates, depending how we'll end up doing them... so it's unclear if and how this will evolve in the future and it's better to keep our options open.

My preferred solution to the issue at hand at the moment would be to treat it as a plain regular cast - as @cameel noted that doesn't work right now, because functions cannot be casted using the current conversion syntax, but it's on our agenda to rework that anyways, so hopefully this will yield a nice solution killing two birds with one stone...

@cameel
Copy link
Member

cameel commented Jan 19, 2022

@MikaelLazarev You're right. abi.decode(this.transfer, (Enum)) has the same problem with shadowing and won't compile either if you define a variable called Enum so my argument was a bit weak.

Still, I really don't like how much of a special case that syntax would be. Like @ekpyron said, there's no magic with current abi.decode(). Parameter is just a tuple and cannot be interpreted differently. With this.transfer(address,uint256,bytes).selector the meaning of the parenthesis changes based on context.

Adding abi.resolve function also makes things more complex, cause selector is bytes4 constant

True, it would be a bit different than the other abi. functions. To be honest this is not my preferred syntax. I was just trying to give you a few varied possibilities to choose from. Personally, I'd rather have it as a cast. Alternatively, this.transfer@(address, uint256, bytes) would be my second choice.

@cameel
Copy link
Member

cameel commented Jan 19, 2022

Since #11284 would give us an easy way out of this, posting feedback there is likely this speed up getting to a solution here too. We need opinions on the proposed syntax (or new proposals).

@k06a
Copy link

k06a commented Jan 19, 2022

Right now function pointers have argument types defined in compile-time and selector defined in run-time:

function(address, uint256, bytes) external func;

Let's add the ability to have selector fixed in the compile too:

function transfer(address, uint256, bytes) external func;

Then this.transfer will be a function pointer with a fixed selector and for overloaded methods will create ambiguity during compilation and ask the developer to resolve this method with some of the syntax construction discussed earlier for example:

this.transfer[address,uint256]

@chriseth
Copy link
Contributor

While I also like the conversion syntax, it is a bit weird at type level, since you apply a function (the conversion function) to an object that does not have a fixed value. I think we have to go with some special syntax like name::<types>, although over the years I have come to see overloading more and more messy and would like to avoid it.

@chriseth
Copy link
Contributor

I'm not sure whether I understood @k06a 's proposal correctly, but a function pointer with a fixed selector is essentially an interface with a single defined function. Is this a possible way out here?

@k06a
Copy link

k06a commented Feb 17, 2022

@chriseth yes, but the interface is static at compile-time and a function pointer is dynamic.

@Marenz
Copy link
Contributor

Marenz commented Feb 17, 2022

What about this variation:

this.transfer.selector(address, uint256, bytes);

@chriseth
Copy link
Contributor

We need a syntax to resolve overloads in a generic way, not a syntax specific to the selector function.

@cameel cameel added medium effort Default level of effort medium impact Default level of impact must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. needs design The proposal is too vague to be implemented right away labels Sep 26, 2022
@cameel cameel changed the title Support retrieving selector of an overloaded function Syntax to refer to a specific overload of a function Jan 27, 2023
@CJ42
Copy link
Contributor

CJ42 commented Feb 18, 2023

How about the same way as web3js or ethersjs do?

this["transfer(address,uint256,bytes)"].selector;

@daoio
Copy link

daoio commented Aug 30, 2023

imo, one of the variations of the 2nd approach (@ is my favourite), would be the most appropriate, given the current syntax.

contract C {
    function g() public {}
    function g(uint256 i) public {}

    function f() public {
        // assign func to a variable
        function(uint256) fn0 = g@(uint256);
        function() fn1 = g@();
        
        // fetch selector
        bytes4 s0 = this.g@(uint256).selector;
        bytes4 s1 = this.g@().selector;
        
        // pass func as an argument
        _callFn(fn0);
        // etc....
    }
    
    function _callFn(function(uint256) fn) internal {
        fn(1337);
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language design :rage4: Any changes to the language, e.g. new features medium effort Default level of effort medium impact Default level of impact must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. needs design The proposal is too vague to be implemented right away
Projects
None yet
Development

No branches or pull requests