Make ConstantEvaluator truncate the result of shift operations

[matheusaaguiar]

Fix #16596.
Spotted in #16456 (comment).

[matheusaaguiar]

I guess it could be done only for left shift.

[matheusaaguiar]

Not sure about a Changelog entry...

[clonker]

Some code structure feedback and questions. Overall looks correct to me. I think BitNot has similar issues. For example ~uint8(0) and ~uint8(-128) both fail constant eval. If you agree, this could be folded into this pr or be a follow up.

Not sure about a Changelog entry...

It's user-facing, isn't it? So imo having an entry is the right move.

[matheusaaguiar]

This refactor was suggested here and it is simple enough that I think it can be slipped in this PR...

[matheusaaguiar]

This is actually due to an issue in the SMT Checker.
I was worried that it had something to do with the changes in this PR and investigated it.
Apparently, the SMTEncoder visitor was written when only literals were constant evaluated.
Later when support for constant folding of variables was added, the relevant part of SMT Checker was not properly adapted and since there were no tests covering, it went unnoticed.

In short, when visiting the unary operation, SMT Encoder checks whether the result is a rational number, which would indicate a literal expression as operand.
Since literal expressions have unlimited precision, it would then skip visiting the operand.
However, the Constant Evaluator later gained the ability to evaluate other expressions and the SMT Encoder then skips visiting those, assuming they are literals.

For example, using unary minus can also trigger the warning:

contract C {
    int256 constant signedConstant = 42;
    function test() public pure returns (int) {
        return -signedConstant;
    }
}

Before the changes in this PR, this test was not triggering the warning because the bitwise not was generating a value out of range of integer, and thus the Constant Evaluator was not returning a rational number, which would make that the operand was visited.

I will open an issue for this.

[clonker]

The test has gone from the PR, is there an issue now?

[matheusaaguiar]

It was appearing in the PR only because of the issue I mentioned in the comment and #16654 fixed it.

[clonker]

looks pretty good to me! i'd love to see these additional tests (wouldn't expect anything to fail there but at least then the boundaries are tied down) and the unnecessary string copying gone :)

[github-actions]

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

[clonker]

Hey, sorry for not getting back to this any sooner.

There is no test covering left shift by >= type width with a small type (e.g. U8_64 << 8); the wrap tests all shift by less than the width except the full-256-bit case, right? If so, I think it should be added.

There's also another small inconsistency with how the runtime behaves wrt large shift constants:

uint256 constant ONE = 1;
uint256 constant A = ONE << 300;   // exact 2^300 fits the 4096-bit precision cap
uint256 constant B = ONE << 5000;  // exact 2^5000 exceeds it

contract C {
    uint[A + 1] a;                                        // needs compile-time evaluation
    uint[B + 1] b;                                        // needs compile-time evaluation
    function f() public pure returns (uint256) { return B; }  // runtime only
}

the a case compiles under this PR, the b case doesn't. It's probably not extremely important but still. :)

[clonker]

Suggested change

	// SPDX-License-Identifier: MIT
	pragma solidity ^0.8.24;

[clonker]

Suggested change

int16 constant I16_POSITIVE_127 = 127;

unused

[clonker]

Suggested change

	int8 constant I8_NEGATIVE_128 = -128; // 1000 0000
	int8 constant I8_POSITIVE_127 = 127; // 0111 1111

unused

[clonker]

in shift right no wrapping should occur, so i find _WRAP a bit misleading in the variable names

[clonker]

in shift right no wrapping should occur, so i find _WRAP a bit misleading in the variable names

[matheusaaguiar]

the a case compiles under this PR, the b case doesn't. It's probably not extremely important but still. :)

Yes, in b case the constant evaluator returns "empty" or no value because it overflows the 4096 bit width, however codegen executes outputting 0 as a result.
I modified the code so constant evaluator returns 0 as well. I am running some tests to check there's no problem.