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

Adding analyzer/fixer for the Regex Source Generator (#68976) #69872

Merged
merged 1 commit into from
May 27, 2022

Conversation

joperezr
Copy link
Member

Adding analyzer/fixer for the Regex Source Generator

This feature just missed the snap for preview 5 for a few hours, and we would really like to have it be part of preview 5 in order to get feedback on the analyzer, as well as get a (hopefully) larger exposure to the source generator to folks consuming the preview.

Description

Adding Roslyn analyzer and code fixer that will suggest the use of the Regex Source Generator whenever possible.

This change was merged to main branch here: #68976

Customer Impact

After this goes in, customers using Regex in their apps with constant and known-at-compile-time inputs will get an informational diagnostic suggesting converting into the Regex Source Generator instead, which is expected to be a lot more performant (plus many other benefits) than the rest of the engines that would be used instead. It also provides a code-fixer so that if the customer does decide to use the source generator, all of the changes will be done for them.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

All Roslyn analyzers and code-fixers are visible to the user since they are features for the IDE, which is why they always come with some risk. That said, any issues that the fixer and analyzer may have will cause the IDE to automatically turn off the analyzer/fixer as a mitigation. We mainly want to get this out in preview 5 in order to help flush any issues that may exist with it.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

* Adding analyzer/fixer for the Regex Source Generator

* Adding some tests to the analyzer and fixer

* Fix build and reference live ref pack

* Address remaining feedback and fix top-level statement programs

* Addressing PR Feedback

* Disabling the tests for Mono
@joperezr joperezr added Servicing-consider Issue for next servicing release review area-System.Text.RegularExpressions labels May 26, 2022
@joperezr joperezr requested a review from stephentoub May 26, 2022 17:50
@ghost ghost assigned joperezr May 26, 2022
@ghost
Copy link

ghost commented May 26, 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

Adding analyzer/fixer for the Regex Source Generator

This feature just missed the snap for preview 5 for a few hours, and we would really like to have it be part of preview 5 in order to get feedback on the analyzer, as well as get a (hopefully) larger exposure to the source generator to folks consuming the preview.

Description

Adding Roslyn analyzer and code fixer that will suggest the use of the Regex Source Generator whenever possible.

This change was merged to main branch here: #68976

Customer Impact

After this goes in, customers using Regex in their apps with constant and known-at-compile-time inputs will get an informational diagnostic suggesting converting into the Regex Source Generator instead, which is expected to be a lot more performant (plus many other benefits) than the rest of the engines that would be used instead. It also provides a code-fixer so that if the customer does decide to use the source generator, all of the changes will be done for them.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

All Roslyn analyzers and code-fixers are visible to the user since they are features for the IDE, which is why they always come with some risk. That said, any issues that the fixer and analyzer may have will cause the IDE to automatically turn off the analyzer/fixer as a mitigation. We mainly want to get this out in preview 5 in order to help flush any issues that may exist with it.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A
Author: joperezr
Assignees: -
Labels:

Servicing-consider, area-System.Text.RegularExpressions

Milestone: -

@joperezr
Copy link
Member Author

The failures in MSQuic tests are because the snap missed #69798 which disabled those tests. cc: @rzikm @wfurt

@joperezr
Copy link
Member Author

This has been approved via email, so marking this as servicing-approved.

@joperezr joperezr added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 26, 2022
@ericstj
Copy link
Member

ericstj commented May 27, 2022

Test failures are #69792

@ericstj ericstj merged commit 44d8b31 into dotnet:release/7.0-preview5 May 27, 2022
return;
}

// If the constructor also has a timeout argument, then don't emit a diagnostic.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious why? RegexGeneratorAttribute allows timeouts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have added more info into this comment, sorry about that. The main issue here is that Regex constructor and static methods take in a TimeSpan as a timeout argument, but since TimeSpan values are not accepted as attribute parameters, the RegexGeneratorAttribute take int milleseconds instead. I originally did have support and was doing the conversion from TimeSpan -> milliseconds on the fly, but @stephentoub pointed out that this was much more logic than we would ideally want to have in the analyzer, and it could be error prone as we would have to account for a lot of possible cases. For reference, here is what a very similar analyzer is doing to make this conversion so we would basically need to have something similar to this https://github.com/meziantou/Meziantou.Analyzer/blob/b6b41fa2a835fff4dc8cb2a58d692ea1a39f22ab/src/Meziantou.Analyzer/Rules/UseRegexSourceGeneratorAnalyzer.cs#L173-L313

Given it is very rare for folks to even use a timeout to begin with, we opted for now to not emit diagnostics when the call-site has a timeout parameter.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 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.

3 participants