Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/7.0] React to CheckForOverflowUnderflow in regex source generator #78256

Merged
merged 2 commits into from
Nov 14, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 11, 2022

Backport of #78228 to release/7.0
Fixes #78214

/cc @stephentoub

Customer Impact

The regex source generator produces code that throws overflow exceptions when the CheckForOverflowUnderflow compiler option is set. It uses code patterns that overflow by design (e.g. (uint)index < span.Length). The fix simply surrounds the code with unchecked.

Testing

Changed most tests to use the more conservative /checked. Added new ones for covering various configurations of checked and unchecked in combination with other features.

Risk

Low.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

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.
@ghost
Copy link

ghost commented Nov 11, 2022

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

Issue Details

Backport of #78228 to release/7.0

/cc @stephentoub

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

@stephentoub
Copy link
Member

@ViktorHofer, do I need to do anything here for package authoring?

@joperezr
Copy link
Member

Shouldn't need to. Regex ships inbox as part of the shared framework, so no special packaging changes should be needed here.

@joperezr joperezr added the Servicing-consider Issue for next servicing release review label Nov 11, 2022
@stephentoub
Copy link
Member

Regex ships inbox as part of the shared framework, so no special packaging changes should be needed here.

But the generator?

@joperezr
Copy link
Member

Same, it ships in the targeting pack (ref pack) which ships with no need of doing package authoring.

@joperezr
Copy link
Member

The net48 failure is a known issue and will be fixed via: #78250

@stephentoub
Copy link
Member

Great, thanks

@ViktorHofer
Copy link
Member

As Joe said, both components ship eclusively inbox and therefore package authoring changes aren't required.

@carlossanlop
Copy link
Member

Approved by Tactics. Signed off by area owners. CI failure is unrelated. No OOB package authoring changes needed.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit ba54d0a into release/7.0 Nov 14, 2022
@carlossanlop carlossanlop deleted the backport/pr-78228-to-release/7.0 branch November 14, 2022 19:57
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants