[ruff] Detect more strict-integer expressions (RUF046)

[InSyncWithFoo]

Summary

Part 2 of the big change introduced in #14828 and the follow-up to #14832.

Test Plan

cargo nextest run and cargo insta test.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+1 -0 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

rotki/rotki (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ rotkehlchen/tests/utils/ethereum.py:231:15: RUF046 [*] Value being casted is already an integer

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF046	1	1	0	0	0

[MichaReiser]

Hmm, I can't change the base branch if the target is from a fork. I'll wait reviewing this PR until the target is merged.

[MichaReiser]

Would you mind writing a short summary of what you're changing in the PR description? It's hard to review code if the change is unclear.

Did you review through the ecosystem changes? It's unclear to me why we're no longer flagging

https://github.com/RasaHQ/rasa/blob/b8de3b231126747ff74b2782cb25cb22d2d898d7/rasa/shared/core/training_data/loading.py#L86

[InSyncWithFoo]

@MichaReiser These changes are all expected, even if they don't seem correct at first sight. For example:

#                                                   vvvvvvvvvvvvv Not `int`.
def load_data_from_files(..., exclusion_percentage: Optional[int] = None) -> ...:
    ...
#                   vvvvvvvvvvvvvvvvvvvvvvvvvvvv This is classified as "Maybe" rather than "Likely".
    idx = int(round(exclusion_percentage / 100.0 * len(story_steps)))
#                   \______This thus might also not be `int`______/

However, I also agree that this is suboptimal. round() with no ndigits is indeed likely to produce an int. In any case, since #14832 and this have diverged, I can only work on improving this after the former is merged.

[MichaReiser]

Would you mind updating your PR with a short summary outlining the change? It's otherwise very difficult to review the PR without even knowing what the intended behavior is.

[InSyncWithFoo]

The goal of this PR is to correctly detect 1 & 1 and other kinds of expressions to be strict instances of int. Other cases can be found in RUF046.py.

To give an example, int(len(foo) // 2) would not be detected as a violation prior to this PR. It now is. Existing violations are also preserved.

There are, however, limitations. foo is not bar is classified as IsStrictlyInt::False, resulting in 1 + (foo is not bar) being IsStrictlyInt::Maybe, even though 1 + True is 2.

[MichaReiser]

Thanks for the extra explanation. That's helpful.

I don't think the pattern is common enough to justify the increase in complexity. Unless we move some of the logic into typing::is_int where it can be used for other rules as well.

[InSyncWithFoo]

@MichaReiser On the other hand, it is:

• \bint\(\s*\d+\s*(?:[-+*^&|]|//)\s*\d+\s*\): 500+ results
• \bint\(\s*\d+\s*\): 10000+ results

That's without function calls, variables and such.

There's also \bint\(\s*\d+\s*/\s*\d+\s*\) with ~900 results, but I suppose that should be another rule.

[MichaReiser]

I'm not suggesting that we should not improve the rule at all, but I'm wary of making rules unnecessarily complex by implementing too much ad hoc type inference. It results in inconsistent behavior between rules and makes Ruff harder to maintain.

Can we focus on the most important cases you outlined above: single numbers, mathematical expressions, and a few known function calls? It also makes me wonder if we could move some or most of the logic into typing::is_int (we can only move the "strict" part). The logic would then benefit all rules instead of just a few.

[InSyncWithFoo]

Can we focus on the most important cases you outlined above: single numbers, mathematical expressions, and a few known function calls?

But I'm already focused on such expressions. All three kinds you mention, yes. expr_is_strictly_int() just recurses into subexpressions, nothing more. I really don't think I can simplify it any further (save for the Expr::Compare branch?).

typing::is_int() attempts to infer the type of a binding, not an arbitrary expression. I could move expr_is_strictly_int() to somewhere inside typing, but that's about it.

[MichaReiser]

A is_int_expr next to is_int that returns true for expressions that are guaranteed to be int seems like a good outcome. What do you think? The maybe-int cases can then be handled inside the rule itself.

[InSyncWithFoo]

expr_is_strictly_int() is now located in typing.rs with its name changed to is_strictly_int_expr. I'm not sure if that changes anything, though.

[MichaReiser]

@AlexWaygood made me aware that we already have ResolvedPythonType which implements a very similar infrastructure. I think we should integrate your changes there but without the True, Maybe distinction. Instead, it only infers Int if the type is guaranteed to be an int and it returns Unknown otherwise.

My suggestion for this PR is to simply use ResolvedPythonType without changing its implementation. We can improve ResolvedPythonType if needed in a separate PR

That means that the handling of round must remain in this rule.

[InSyncWithFoo]

@MichaReiser I'm not sure if I follow. How would I detect int(round(...) * 42) with ResolvedPythonType? As I see, it doesn't handle calls, which means round(...) * 42 would be resolved to Unknown. Or do you mean we should not fix such cases for now?

[MichaReiser]

@InSyncWithFoo

We shouldn't fix those cases for now. We can handle round calls directly in RUF046 and anything more involved using round won't be detected because ResolvedPythonType doesn't support it.

[MichaReiser]

Perfect, thank you! I think this is a great improvement to the rule without significantly increasing its complexity.

[InSyncWithFoo]

@MichaReiser Why was the file renamed to round_applicability? That's not the name of the rule.