Reconsider operators on ContractType

[hrkrshnn]

contract Test {}
contract Derived is Test {}
function f(Test a, Test b) {
    a < b;
    a == b;
    a > b;
    a <= b;
    a >= b;
}

function g(Derived a, Test b) {
    a < b;
    a == b;
    a > b;
    a <= b;
    a >= b;
}

Currently, the above code compiles. I think the comparison operators does not belong with the ContractType. Even the equality operator is questionable. Disallowing all these operators is especially important to be consistent with how operators are disallowed by default in user defined types: https://github.com/ethereum/solidity/issues/11531.

[chriseth]

Oh wow! I think this is a remnant of contracts being treated as addresses. If you want to compare contracts like that, I think it would be better to require conversion to address first, which then also makes it much clearer how they are compared.

[bshastry]

I was of the impression that this is permitted due to an implicit cast to addresses. The fuzzer is currently churning these out happily :-)

[cameel]

I think that disallowing <, <=, > and >= is a good thing.

With != and == it's not so clear but I would also be in favor of disallowing it.

While version without the cast looks cleaner and it's not hard to realize that it's a comparison of addresses (not state variables):

if (myERC20Token == whitelistedTokens[i]) { ... }

the one with casts reads much better as a condition even despite being longer and makes the meaning very clear:

if (address(myERC20Token) == address(whitelistedTokens[i])) { ... }

Also, in many cases the other side of the comparison is already an address so it's not even that much longer:

if (address(myERC20Token) == whitelistedTokens[i]) { ... }

[axic]

Do we have comparison operators on the contract type (i.e. Test == Derived or type(a) != type(b))? There I think != and == can make sense, but I agree that these operators should not exist on the contract instances.

[cameel]

Do we have comparison operators on the contract type (i.e. Test == Derived or type(a) != type(b))?

Looks like we don't:

Error: Operator == not compatible with types type(contract C) and type(contract C)
 --> test.sol:3:9:
  |
3 |         C == C;
  |         ^^^^^^

Error: Operator != not compatible with types type(contract C) and type(contract C)
 --> test.sol:4:9:
  |
4 |         C != C;
  |         ^^^^^^

Error: Operator < not compatible with types type(contract C) and type(contract C)
 --> test.sol:5:9:
  |
5 |         C < C;
  |

[axic]

With != and == it's not so clear but I would also be in favor of disallowing it.

The following may be a reason to disallow it: does it mean comparison between the type or the instance? The type may be equal in the case the child contract has no changes (contract B is A {}), but what should it return in case the underlying address is different? Because of this confusing aspect, it is better not to allow comparison of instances, but potentially allow comparison of the type, and of course comparison of the address is supported via explicit cast.

[hrkrshnn]

Decision: disallow everything. Even ==.

[Marenz]

On our side, everything is implemented in #12568 but we need to update our external tests, that would be the next step for this task

[cameel]

We should reevaluate this since so many ext tests are failing. Isn't it just too common?

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

[ekpyron]

Staging this for re-evaluation again for 0.9. At least comparisons between different contract types should probably be restricted. Or I'd still say removing all of them should be up for discussion.

[cameel]

I recently noticed that our docs on the contract types actually state that contracts have no operators:

Contracts do not support any operators.

So this is actually an undocumented feature :)

[ekpyron]

Ah, then let's just exercise our right to remove it despite breaking every second contract ;-P. Well, I guess we'll have to consider this an incidence of Hyrum's Law :-).

[peersky]

It would make sense for me if eq/neq operations compare address.codeHash, allowing in runtime to detect whether addresses contain equal bytecode.