React to CheckForOverflowUnderflow in regex source generator

[stephentoub]

The regex source generator uses code patterns that might have arithmetic overflows, e.g. a bounds check with (uint)index < span.Length. These are intentional, and they're benign... unless the project/compilation has opted-in to overflow/underflow checking (CheckForOverflowUnderflow). In that case, the code for many patterns can start throwing false positive overflow exceptions, making the source generator unusable.

This commit causes the generator to look at the CheckOverflow setting in the compilation options, and if it's set, to emit unchecked { ... } around all the relevant code.

We'll want to backport this to release/7.0.

#78214

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The regex source generator uses code patterns that might have arithmetic overflows, e.g. a bounds check with (uint)index < span.Length. These are intentional, and they're benign... unless the project/compilation has opted-in to overflow/underflow checking (CheckForOverflowUnderflow). In that case, the code for many patterns can start throwing false positive overflow exceptions, making the source generator unusable.

This commit causes the generator to look at the CheckOverflow setting in the compilation options, and if it's set, to emit unchecked { ... } around all the relevant code.

We'll want to backport this to release/7.0.

Author:	stephentoub
Assignees:	-
Labels:	area-System.Text.RegularExpressions
Milestone:	-

[CyrusNajmabadi]

i dont' see any issue on the IG side.

[stephentoub]

i dont' see any issue on the IG side.

Great, thanks for looking

[joperezr]

NIT: no need to reset CI for this, but I think it is a bit more readable in general when we have local funcs at the end of the method (even better if they are after a return statement)

[joperezr]

I'm not very familiar with unchecked when related to performance so this question might not make sense, but would it make sense to just always emit the unchecked block when doing these types of bounds checks even when CheckOverflow is false? Or is the reason we don't want to do that mainly to improve code readability?

[stephentoub]

Or is the reason we don't want to do that mainly to improve code readability?

yup

[stephentoub]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3448484399