Produce compiler error for compound division and mod zero operations

[clonker]

Fixes #14702

As a side-effect, this will change the place in which an error for expressions of the form a = a / 0 is raised:

• in terms of types, such expressions yield an IntegerType on the left-hand side, so no explicit value is associated with it as well
• pretending the lhs to be 0 lets me reuse some of the functionality but it also doesn't 100%ly capture the nature of the problem ("unary f: x->x/0 is bad")
• without this change, an error for a=a/0 will be raised in static analyzer. Static analyzer states in its docs, that it has to be possible to write equivalent code that does not generate the warning, so perhaps it was not the right place for raising it to begin with?

[r0qs]

Hey @clonker thanks a lot for your contribution. I have some initial comments :)

[ekpyron]

Did you actually in your call earlier conclude that this is the better option :-)? I'd have expected us to just add an analog case to the existing Division by/Modulo zero errors (with those as error messages) in the Assignment visitor of the StaticAnalyzer (but I'm mainly being curious here - for this change I'd need to check, if it actually affects anything else and am surprised that it doesn't e.g. also change the error message for existing / 0 cases and such)

[cameel]

We mainly concluded that doing it here produces the same error and in the same cases as the % and / case, so it should be fine.

Now that you mention it, I haven't actually looked at the place where % and / are handled currently. But I don't think this is done in StaticAnalyzer. Or is it? Will have to check in the final review.

[ekpyron]

solidity/libsolidity/analysis/StaticAnalyzer.cpp

Line 308 in ae79d13

(_operation.getOperator() == Token::Div) ? "Division by zero." : "Modulo zero."

I actually also now realized why this change doesn't change the error message in the test for this error: in the test case it's actually an expression on literals there (or well, constants - which are treated as literals).

Anyways - since I also now read in the PR description:

Static analyzer states in its docs, that it has to be possible to write equivalent code that does not generate the warning, so perhaps it was not the right place for raising it to begin with?

The equivalent code in these cases is revert(); - since that's what a division by zero does :-).

But yeah, none of this matters too much, this was just a random issue to get started anyways.

[ekpyron]

However, that also explains why we have this weird error in the first place: it wasn't ever meant to apply for runtime divisions, but only for compile-time divisions (and divisions of constants, which are compile-time divisions).

So given that, actually, the better fix here, is in fact (as @cameel, you suggested earlier today in chat) to do the opposite and allow this for runtime division.

Which should be doable by just adding

*_operation.leftExpression().annotation().isPure &&
ConstantEvaluator::evaluate(m_errorReporter, _operation.leftExpression())) &&

to the condition in

solidity/libsolidity/analysis/StaticAnalyzer.cpp

Line 300 in ae79d13

*_operation.rightExpression().annotation().isPure &&

That should still catch divisions by zero on compile-time evaluated constants - but leave uint x; x / 0; in peace - and we also solved the inconsistency, since /= 0 is impossible for compile-time constants.

So I'd suggest we just do that instead - but yeah, this was actually a rather weird issue that I picked at random here :-D. The lesson to be learned is: never blindly do exactly what people ask ;-).

[clonker]

While it's a different issue in a sense, I feel like having a warning regarding div / mod zero (also for compounds) isn't such a bad thing and wouldnt introduce inconsistencies. But yeah, a bit weird :-D

[ekpyron]

While it's a different issue in a sense, I feel like having a warning regarding div / mod zero (also for compounds) isn't such a bad thing and wouldn't introduce inconsistencies. But yeah, a bit weird :-D

In principle: yes.
The problem is, that we can't do it well anyways - we don't have full compile-time-constant expression evaluation.
So you'd get a warning for

uint x; x / 0;

but not for

uint x = 1; uint y = 1; uint z;
z / (x - y);

So in the end just for the boring and obvious cases - which is arguably worse than not at all...
Conversely, proper compile-time-constant expression evaluation would be nice of course, but also quite a bit of effort, especially since it'd need to match EVM semantics completely accurately.