Improve overloaded function call diagnostics

[seanyoung]

From Solidity 0.6 onwards, when an overloaded function/event resolves to multiple candidates, this is an error. In earlier versions, the first result is used. So, use the pragma solidity version to decide whether to error or not.

Fixes: https://github.com/hyperledger/solang/issues/1534 Fixes: https://github.com/hyperledger/solang/issues/707

[codecov[bot]]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (a960551) 87.67% compared to head (90bc7d6) 87.94%.

Files	Patch %	Lines
src/sema/pragma.rs	98.59%	3 Missing :warning:
src/sema/builtin.rs	98.59%	1 Missing :warning:
src/sema/expression/constructor.rs	99.42%	1 Missing :warning:
src/sema/expression/function_call.rs	99.74%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1592 +/- ## ========================================== + Coverage 87.67% 87.94% +0.27% ========================================== Files 136 137 +1 Lines 65457 65902 +445 ========================================== + Hits 57390 57959 +569 + Misses 8067 7943 -124 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[seanyoung]

@LucasSte Thanks for your "there is no test for this" comments. I am aware of this. Not exactly a useful review.

[seanyoung]

How / where are we going to document this?

I had not thought about this. Do you have any suggestions?

[xermicus]

How / where are we going to document this?

I had not thought about this. Do you have any suggestions?

I guess it depends on the general direction we want to go with this. Currently, we just say in the docs that we ignore the version pragma, which is kind of no longer true after this PR.

[xermicus]

I am still a bit confused on how we handle version pragma now.

It seems very dangerous to me if we just start honoring some pragmas randomly. At very least, I think Solang should refuse compiling incomatible contracts like the one in above example.

[xermicus]

Maybe it's a sensible strategy to have each Solang release being compatible with a specific solc release. I think that would clarify a lot of things.

[seanyoung]

I guess it depends on the general direction we want to go with this. Currently, we just say in the docs that we ignore the version pragma, which is kind of no longer true after this PR.

There are some differences between versions, but nothing huge. The idea here is that we switch behaviour based on the solidity pragma, so that - ultimately - we can compile any Solidity. A single solidity compiler for all versions.

[seanyoung]

If I got the following source:

// a.sol
pragma solidity ^0.7;

import "b.sol";

contract C {
        function c() public payable {
                require(msg.value > SERVICE_FEE); 
        }
}

// b.sol
pragma solidity ^0.8;

uint128 constant SERVICE_FEE = 123;

Shouldn't Solang then refuse to compile that?

No, why? Just because solc only support one version does not mean we have to be limited to one version too.

[xermicus]

No, why? Just because solc only support one version does not mean we have to be limited to one version too.

Are you sure this will work for all cases?

[seanyoung]

Maybe it's a sensible strategy to have each Solang release being compatible with a specific solc release. I think that would clarify a lot of things.

That would mean supporting lots of different versions of solang, one for each version of Solidity. That's too much work.

[xermicus]

I agree that'd be a lot of work.

What about a warning if there are conflicting version pragma found in the source. This way, the user at least gets warned that he is trying to compile "incompatible" sources.

[seanyoung]

I agree that'd be a lot of work.

What about a warning if there are conflicting version pragma found in the source. This way, the user at least gets warned that he is trying to compile "incompatible" sources.

The problem is, how do you determine that two pragmas are conflicting? We might have in one file: pragma solidity 0.5 - 0.5.16; and another: pragma solidity <=0.6;

In the solidity pragma parsing, all we have right now in solang is what the highest supported Solidity version. In this case we have 0.5.16 and 0.6. Is that a conflict? If you were to compile with solc 0.5.15 then it works fine, but if you compile with solc 0.6.1 then it is not fine.

I think this is something for another PR.

[xermicus]

By conflicting I meant there is no suitable compiler, (e.g. pragma solidity ^0.5.16; and pragma solidity ^0.6.16)

[seanyoung]

By conflicting I meant there is no suitable compiler, (e.g. pragma solidity ^0.5.16; and pragma solidity ^0.6.16)

I don't understand why that is relevant to this PR. Is there some sort of regression?

[seanyoung]

Encounters version pragmas that are mutually exclusive (meaning there would be no solc version that can compile this)

Sure, that is something that we should add at some point. Like I said, a bunch of code needs to written for this.

Is compiling the same code differently due to different version pragmas

Why? There are bunch of tiny differences between solidity versions. In early Solidity versions, constructors require a visibility attribute. In later versions, they are ignored. So you want a warning like:

pragma solidity 0.5;

contract C {
     constructor() public {}
}

warning: in later versions of solidity constructor visibility is ignored but if you remove it your contract will no longer build due to the pragma solidity 0.5;

[xermicus]

Sure, can be done later.

warning: in later versions of solidity constructor visibility is ignored but if you remove it your contract will no longer build due to the pragma solidity 0.5;

No, that wouldn't be useful. What I meant was:

pragma solidity 0.6;

contract Fibonacci {
    constructor() {}
}

This should produce a warning like follows:

Warning: Solidity 0.6 requires constructors to have a visibility.

Which would remind the user that he is doing something that was not intended in Ethereum Solidity but works anyways in Solang.

[seanyoung]

It does make sense, to make it clear to the developer what has changed and how it affects their code - in case there are any potential pitfalls.

After some more testing I've re-jigged the code to match the actual behaviour better, and also better error messages which shows the actual change in behaviour between versions which helps the developer.

[seanyoung]

let me know what you think

[xermicus]

Great!

make it clear to the developer what has changed and how it affects their code - in case there are any potential pitfalls.

Yeah this is exactly what I meant :)