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

Overload resolution failure should explain for each candidate why it does not match. #9607

Open
Marenz opened this issue Aug 12, 2020 · 10 comments
Labels
annoys users 😢 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.
Projects

Comments

@Marenz
Copy link
Contributor

Marenz commented Aug 12, 2020

In the example below it is obvious we want to call the variant with the two extra parameters, but the error does not make it obvious why we can't call it (uint -> int conversion required). A better error message showing the different types would be great.

	function checkStealing(uint _x, uint _y) private
	{
		CrossState[] memory stealPattern = new CrossState[](4);
                [..]
		for (uint i = 0; i < directions.length; i++)
		{
			(int xS, int yS, int xE, int yE) =
				checkPattern(_x, _y, stealPattern, directions[i].x, directions[i].y); // error here
                       [..]
		}
	}

	function checkPattern(int _x, int _y, CrossState[] memory _pattern,
		int _xStep, int _yStep) public payable
		returns(int startX, int startY, int endX, int endY)
	{ [..] }

	function checkPattern(int _x, int _y, CrossState[] memory _pattern) public returns(bool)
	{
        [...]
        }


Error: No matching declaration found after argument-dependent lookup.
   --> src/Game.sol:137:5:
    |
137 |                                 checkPattern(_x, _y, stealPattern, directions[i].x, directions[i].y);
    |                                 ^^^^^^^^^^^^
Note: Candidate:
   --> src/Game.sol:112:2:
    |
112 |         function checkPattern(int _x, int _y, CrossState[] memory _pattern) public returns(bool)
    |  ^ (Relevant source part starts here and spans across multiple lines).
Note: Candidate:
   --> src/Game.sol:156:2:
    |
156 |         function checkPattern(int _x, int _y, CrossState[] memory _pattern,
    |  ^ (Relevant source part starts here and spans across multiple lines).
@Marenz Marenz added feature language design :rage4: Any changes to the language, e.g. new features labels Aug 12, 2020
@axic
Copy link
Member

axic commented Aug 12, 2020

Note: Candidate:
   --> src/Game.sol:156:2:
    |
156 |         function checkPattern(int _x, int _y, CrossState[] memory _pattern,
    |  ^ (Relevant source part starts here and spans across multiple lines).

I would also suggest to change the source location displayed here to be the entire function definition header.

@chriseth chriseth changed the title Show types that mismatch in error report Overload resolution failure should explain for each candidate why it does not match. Aug 18, 2020
@chriseth chriseth removed the language design :rage4: Any changes to the language, e.g. new features label Aug 18, 2020
@chriseth chriseth added this to New issues in Solidity via automation Aug 18, 2020
@chriseth chriseth moved this from New issues to Implementation Backlog in Solidity Aug 18, 2020
@bshastry
Copy link
Contributor

bshastry commented Nov 2, 2020

I stumbled upon this issue while writing test cases with function types. The following compiles just fine (Edit: And binds to the square of function).

function f(uint x) pure returns (uint) { return x ** 2; }
contract C {
        function (uint) pure returns (uint) immutable i = f;
}

However, the following does not. The type error seems to be incorrect at first glance. Unlike @Marenz example, there is no type conversion happening so one would expect the first free function to be bound to the immutable.

function f(uint x) pure returns (uint) { return x ** 2; }
function f() pure returns (uint) { return 42; }
contract C {
        function (uint) pure returns (uint) immutable i = f;
}
// ----
// TypeError 2144: (170-171): No matching declaration found after variable lookup.

@chriseth
Copy link
Contributor

chriseth commented Nov 5, 2020

@bshastry this is expected. What an expression resolves to should not depend on how the expression is used later on, at least in my opinion.

@bshastry
Copy link
Contributor

bshastry commented Nov 5, 2020

@bshastry this is expected. What an expression resolves to should not depend on how the expression is used later on, at least in my opinion.

Not sure I follow. Shouldn't i bind to f(uint) in the example? i is not used later on, so I don't understand what you mean.

@chriseth
Copy link
Contributor

chriseth commented Nov 5, 2020

Solidity follows the concept that in x = i, what i refers to does not depend on x, but only on the scope of i.

@bshastry
Copy link
Contributor

bshastry commented Nov 5, 2020

Solidity follows the concept that in x = i, what i refers to does not depend on x, but only on the scope of i.

Sorry, I still don't understand :) How does the scope of i explain the fact that

function f(uint x) pure returns (uint) { return x ** 2; }
contract C {
        function (uint) pure returns (uint) immutable i = f;
}

compiles

but the following throws an error

function f(uint x) pure returns (uint) { return x ** 2; }
function f() pure returns (uint) { return 42; }
contract C {
        function (uint) pure returns (uint) immutable i = f;
}
// ----
// TypeError 2144: (170-171): No matching declaration found after variable lookup.

In both cases i (in this case function (uint) pure returns (uint)) is in scope because the free function should be visible in the contract and hence in scope, right?

@cameel
Copy link
Member

cameel commented Nov 6, 2020

Another case reported in #10220 by @barakman.

contract Test {
    uint256[] private arr;

    function func(uint256 x, uint256 y) public {
        arr.push(x, y);
    }
}

Up to 0.5.17 the compiler used to report this:

Error: Wrong argument count for function call: 2 arguments given but expected 1.

Starting with 0.6.0 the error message is much less informative:

Error: Member "push" not found or not visible after argument-dependent lookup in uint256[] storage ref.

@chriseth
Copy link
Contributor

chriseth commented Nov 9, 2020

@bshastry the compiler tries to resolve f but there are two candidates. Overload resolution does not work by checking which option would result in fewer errors, it works by looking at the types of arguments in the function call. No function is called here, so it cannot be resolved without ambiguity.

@cameel cameel added medium effort Default level of effort medium impact Default level of impact should have We like the idea but it’s not important enough to be a part of the roadmap. labels Sep 26, 2022
@cameel
Copy link
Member

cameel commented Dec 17, 2022

This came up again recently: #13812. The issue has a nice test case where the error messages look confusing at first glance and only after removing the overload it becomes clear why the candidates do not match.

Also, two related issues:

@cameel cameel added annoys users 😢 must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. and removed should have We like the idea but it’s not important enough to be a part of the roadmap. labels Dec 17, 2022
@Derixtar54

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
annoys users 😢 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.
Projects
No open projects
Solidity
  
Implementation Backlog
Development

No branches or pull requests

7 participants