Warn about not explicitly requested conversions from rational number types to their mobile type.

[donutcrypto]

Description

The ternary operator doesn't appear to be producing the correct return type.

Environment

• Compiler version: 8.11
• Framework/IDE (e.g. Truffle or Remix): hardhat
• EVM execution environment / backend / blockchain client: binance smart chain, hardhat testnet

Steps to Reproduce

Deploying the following contract will overflow on uint8 addition. The expected value of result is 385.

contract TestTernary
{
    mapping(uint8 => bool) flags;

    uint256 public result;

    constructor()
    {
        flags[uint8(1)] = true;
        flags[uint8(2)] = true;
        flags[uint8(3)] = true;

        result = 
            (flags[0] ? 100 : 0) +
            (flags[1] ? 110 : 0) +
            (flags[2] ? 125 : 0) +
            (flags[3] ? 150 : 0);
    }
}

[bshastry]

Thanks for reporting the issue @donutcrypto

Notes:

Fuzzer wouldn't have helped because both legacy and IR fail on construction. Semantic test below:

contract TestTernary
{
    mapping(uint8 => bool) flags;

    uint256 public result;

    constructor()
    {
        flags[uint8(1)] = true;
        flags[uint8(2)] = true;
        flags[uint8(3)] = true;

        result = 
            (flags[0] ? 100 : 0) +
            (flags[1] ? 110 : 0) +
            (flags[2] ? 125 : 0) +
            (flags[3] ? 150 : 0);
    }
}
// ====
// compileViaYul: also
// ----
// constructor() -> FAILURE
// gas irOptimized: 139402
// gas legacy: 140313
// gas legacyOptimized: 135217

[chriseth]

The reason is that unlimited precision only works as long as you are dealing with arithmetic expressions on literal constants. There are other similar situations where this can happen. I think we could warn when conversions from rational constants to their mobile type happen instead of a conversion to an explicit type.

[donutcrypto]

Thanks for the reponse. I just wanted to make sure I understand, but the values of 0, 100, 110, etc. will be compiled to a uint8?

For example:

function f(uint8 _x);
function f(uint256 _x);

function g()
{
    f(1); // calls the uint8 version?
}

[hrkrshnn]

@donutcrypto That would fail with "No unique declaration found after argument-dependent lookup.", because such a lookup is ambiguous. There is probably an open issue about choosing the closest one.

[cameel]

I think we could warn when conversions from rational constants to their mobile type happen instead of a conversion to an explicit type.

Do you mean in general or just in the ternary operator? In a general case this could be pretty annoying. For example in something like uint64 x = a + 1 the 1 gets converted to its mobile type. You'd have to write uint64 x = a + uint64(1) to avoid the warning. This a really common thing to do so warning about this might not be a good idea.

[cameel]

Answer from gitter:
@cameel

I think that even very simple stuff like a + 1 involves a conversion to the mobile type. We don't want to warn about that, right?

@chriseth

indeed. it is a conversion to a type that does not "magically appear"
so if you have a binary operator and convert to one of the operands types, it's fine
only if the target type is different from both source types is it problematic

Still, I'm not really convinced that it's the rule we need here.
In (flags[0] ? 100 : 0) + (flags[1] ? 110 : 0) we have operands converted from literals to uint8 and the result is uint8.
The same thing occurs in (100 + x) + (110 + x) (assuming uint8 x) but we want to warn about the former and not about the latter.

On the other hand with some operators like ==, the result type is always different and we don't want to warn about them either.

I think it's really not just a different mobile type but also the fact that the expression directly returns the value one of its arguments so the user would expect the literal to remain a literal.

Maybe let's try to figure out which specific situations are problematic. So far I came up with these:

• Ternary operator: 1 + (c ? 2 : 3)
• Assignment operator: 1 + (a = 2) * 3 EDIT: not problematic after all.
• Array indexing: [1, 2, 3][1]

On the other hand, I think a warning is not necessary for these:

• Arithmetic operator: 1 + (a + 2) * 3
• Arithmetic assignment operator: 1 + (a += 2) * 3
• Tuple: (a, b, c) = (1, 2, 3) (unless we implement Tuple elements index access #12635)

[chriseth]

I think the probelmatic case is when a type is derived only from rational numbers, i.e. only from mobile types. As soon as an actual "nameable" type that does not come from a mobile type dictates the result type, it's fine.

Can you explain why the assignment operator is problematic?

[cameel]

ok, maybe it's not. Depends on how we define its return type. I guess the most obvious way is to make it the type of the variable on the left side - and then you're right, if user is aware of that, there's no surprise. I treated it as if it was returning the type of the expression on the right side.

[cameel]

We discussed this briefly on the call today:

1. The ternary operator is the only important case - we're going to deal with array literal case along with Array literals should be convertible to both dynamically-sized and statically-sized arrays #11879 if necessary.
2. We do think the behavior is not intuitive to users and we should do something about it.
   • It's interesting that it only came up now. Probably due to checked arithmetic but even that has been present in the compiler for 12 past releases. So maybe this usage is rare in practice?
3. This has potential to generate many false positives and unnecessarily force people to add even more casts. It would be a good idea to check how much it affects existing projects.

@nishant-sachdeva regarding 3), could you replace the warning for ternary operator with an error as suggested by @chriseth and then see how many t_ext_test jobs actually fail?

Also, I think this behavior is not even documented. Could you mention it on the documentation page about Operators?

[chriseth]

Why do you think it is not documented? Rational literals use unlimited precision until they are used together with a non rational literal, and the condition in the ternary operator is never a rational literal, but we can of course make it more explicit.

[cameel]

ok, you're right. It is documented:

Number literal expressions retain arbitrary precision until they are converted to a non-literal type (i.e. by using them together with a non-literal expression or by explicit conversion). This means that computations do not overflow and divisions do not truncate in number literal expressions.

But yeah, it's easy to miss all the consequences of this paragraph unless you really carefully go through the possible operators. I think we should add a note there mentioning ternary operator. I think I'd mention it under Operators too.

[cameel]

Also, this paragraph is not entirely true. I just checked and this reverts:

contract C {
    function f() public {
        (true ? 255 : 254) + (true ? 255 : 254);
    }
}

According to that paragraph it should not overflow - it should be computed in unlimited precision. There's no explicit conversion and no non-literal expressions.

[chriseth]

The issue is that true is not a number literal expression, but yeah, please just clarify the paragraph!

[cameel]

Here's what @nishant-sachdeva's check found.

• 5 projects have at least one use of ternary operator that would be affected. In all cases a warning would be a false-positive because the result does not get used in operations where the smaller range would matter:
  1. OpenZeppelin: return a / b + (a % b == 0 ? 0 : 1);
  2. Uniswap: uint256 ratio = absTick & 0x1 != 0 ? 0xfffcb933bd6fad37aa2d162d1a594001 : 0x100000000000000000000000000000000;
  3. Trident: uint256 ratio = absTick & 0x1 != 0 ? 0xfffcb933bd6fad37aa2d162d1a594001 : 0x100000000000000000000000000000000;
  4. Gnosis Safe: uint flag = response ? 1 : 0;
  5. ElementFi: baseIndex = underlyingFirst ? 0 : 1;
• 4 projects do not have any:
  1. ENS
  2. Pool Together
  3. Yield Liquidator
  4. PRBMath
• In 4 remaining cases it's unclear because the check actually caught stuff with only a single literal. We'll need to should recheck them:
  1. Perpetual Pools: if (negativeY) return absoluteX > absoluteY ? -1 : int8 (1);
  2. Euler: return eTokenAddress == address(0) ? 0 : IERC20(token).balanceOf(eulerAddress);
  3. Bleeps: return eta == 1 ? 0 : eta;
  4. Colony: return unsatisfactory ? 0 : task.payouts[_role][_token];

[github-actions]

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

[github-actions]

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.