fix: avoid integer overflow in ParameterExpression arithmetic - #16980

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

• @Qiskit/terra-core

It's not obvious that this actually is the correct solution for a symbolic engine. This makes it not error, but silently lifts the logic from integer to floating point, which affects the semantics of all subsequent operations. The alternative is of course to add a BigInt variety to the tree, which might have significant knock-on implications because of the complexity of the current code.

Lifting to float may be appropriate because Qiskit it's not a general-purpose symbolic engine, but it is something that needs to have a positive decision and probably a bit of discussion, which might have been better in a bug report / feature request.

Lifting to float may be appropriate because Qiskit it's not a general-purpose symbolic engine, but it is something that needs to have a positive decision and probably a bit of discussion, which might have been better in a bug report / feature request.

Yeah that also made me a bit uneasy when reading it, yet we already do have some lifting to float present, e.g. for divisions or trigonometric functions (the slightly strange thing there is that we sometimes try to cast back to int when it happens to hit an integer value).

The other point is backward compat, those overflows would've worked with the Python version of parameter expressions, right? (Maybe points for a more detailed discussion)

Most general-purpose symbolic engines will support arbitrary-width integers, whether in Python or not. We're less general purpose, so we don't have to do that.

I'm not saying I know the right answer, or necessarily want/need to be part of the decision here, I'm just saying that there is a concrete decision that should be made, considering the options.

This mirrors the coercion the same operators already apply when mixing Int and Real, so in-range integer math is unchanged and the boundary degrades to a finite float instead of panicking/wrapping.

This is a completely different situation. Mixing Real and Int necessarily promotes to Real and is explicitly done by interaction. Overflowing an arbitrary internal width boundary in library code not something the user made a positive choice to do; if they wanted to cast to Real in general, they'd have the option to do the cast.

Let's block this PR on the discussion resolution in #17217

Thanks Julien Gacon (@Cryoris). On div:

• Dividing by a Real never produces an Int, so it can't overflow i64: the result is l as f64 / r. A divisor within SYMEXPR_EPSILON of zero hits the existing rhs == 0.0 check and becomes inf, which binding rejects with ZeroDivisionError: attempted to bind infinite value to parameter. Above that threshold you just get a large float (p / q with p=3, q=1e-14 gives 3e14).
• With an Int divisor, |r| >= 1, so the only out-of-range quotient is i64::MIN / -1. From Python, p / -1 is rewritten as a negation, which this PR already covers, so I could only reach it at the Value level. There t as i64 saturates to i64::MAX instead of panicking.

Checking that path turned up two more cases of the same "compute in f64, cast back with as i64" pattern, both reachable from Python and not touched by this PR so far:

p, q = Parameter("p"), Parameter("q")
(p ** q).assign(p, 2).assign(q, 70.0).numeric()          # 9223372036854775807, saturated
(p / q).assign(p, 3 * 2**60 + 3).assign(q, 3).numeric()   # 2**60, should be 2**60 + 1

The latest commit:

• Int / Int now divides in integers with checked_rem/checked_div. It returns an exact Int when divisible, a float otherwise, and i64::MIN / -1 falls back to a float like the other operators.
• Int ** Real only narrows to Int when the result fits in i64.
• Adds the Python regression test test_integer_overflow_falls_back_to_float. It covers the global_phase example from the description, each operator at the i64 boundary, and the two cases above, and it fails on the previous commit. The Rust unit test is extended to match.

On Jake Lishman (@jakelishman)'s point: agreed, falling back to a float is a choice that should be made on purpose. I kept every operator on the same rule, so whatever #17217 decides (float, BigInt, or raising) is a mechanical change across them. I'll rework it once that's settled.