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

Analyzer/fixer for converting Regex use to source generator #68147

Closed
stephentoub opened this issue Apr 18, 2022 · 10 comments · Fixed by #68976
Closed

Analyzer/fixer for converting Regex use to source generator #68147

stephentoub opened this issue Apr 18, 2022 · 10 comments · Fixed by #68976
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.RegularExpressions blocking Marks issues that we want to fast track in order to unblock other important work code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Apr 18, 2022

Suggested severity: Info
Suggested category: Performance

For new Regex(...) and Regex.StaticMethod(...) involving patterns, options, and timeouts known at compile time, we should provide an analyzer and fixer that will transform the regex into its corresponding RegexGenerator form, including fixing up the call sites.

Flag

  • Use of Regex constructor or static method with inline literals like:
// Flag object creation invocations
Regex myRegex = new Regex("ab|cd", RegexOptions.None, TimeSpan.FromSeconds(2));
// Flag static method invocations
var match = Regex.Match(input, "ab|cd", RegexOptions.None, TimeSpan.FromSeconds(2));
  • Use of Regex constructor or static method when using constant values like:
const string pattern = "ab|cd";
const RegexOptions options = RegexOptions.None;

Regex myRegex = new Regex(pattern, options, TimeSpan.FromSeconds(2));
var match = Regex.Match(input, pattern, options, TimeSpan.FromSeconds(2));

Don't Flag

  • Use of non-constant variables as parameters like:
public void Foo(RegexOptions parameterOptions)
{
    string nonConstantPattern = "ab|cd";

    Regex myRegex = new Regex(nonConstantPattern , parameterOptions, TimeSpan.FromSeconds(2));
    var match = Regex.Match(input, nonConstantPattern , parameterOptions, TimeSpan.FromSeconds(2));
}
  • Options contain NonBacktracking (since this option is not supported by source generator yet) like:
Regex myRegex = new Regex("ab|cd", RegexOptions.NonBacktracking);

Fixer

The fixer will mainly do three things:

  • Add partial keyword (if it's not there already) to the type (or types for nested types) which contains the invocation that generated the diagnostic
  • Declare a new private static partial method with the RegexGenerator attribute and parameters so the source generator generates the Regex type.
  • Change the invocation that produced the diagnostic to use the new static partial method instead.
@stephentoub stephentoub added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.RegularExpressions labels Apr 18, 2022
@stephentoub stephentoub added this to the 7.0.0 milestone Apr 18, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 18, 2022
@ghost
Copy link

ghost commented Apr 18, 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

For new Regex(...) and Regex.StaticMethod(...) involving patterns known at compile time, we should provide an analyzer and fixer that will transform the regex into its corresponding RegexGenerator form, including fixing up the call sites

Author: stephentoub
Assignees: -
Labels:

api-suggestion, area-System.Text.RegularExpressions

Milestone: 7.0.0

@joperezr joperezr self-assigned this Apr 18, 2022
@joperezr
Copy link
Member

I already started looking into this on Thursday, so assigning to me.

@meziantou
Copy link
Contributor

meziantou commented Apr 18, 2022

I've implemented a similar analyzer/fixer on my side. You can take inspiration from it if you want Analyzer, Fixer, demo

@stephentoub
Copy link
Member Author

I've implemented a similar analyzer/fixer on my side.

Oh, cool, thanks for sharing!

@joperezr
Copy link
Member

@meziantou as @stephentoub pointed out, we really appreciate you sharing your work, I took a look and it indeed looks very similar to what we want to have, except that we want to make sure it ships on the same assembly as our source generator. We still need to get the analyzer approved, but once we do, would you be interested in contributing a PR for this analyzer?

@meziantou
Copy link
Contributor

We still need to get the analyzer approved, but once we do, would you be interested in contributing a PR for this analyzer?

Once it is approved, and if I have time at that moment, I can contribute to this analyzer

@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Apr 25, 2022
@joperezr joperezr added code-analyzer Marks an issue that suggests a Roslyn analyzer api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 28, 2022
@terrajobst
Copy link
Member

terrajobst commented May 3, 2022

Video

  • Looks good as proposed

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 3, 2022
@joperezr
Copy link
Member

joperezr commented May 3, 2022

Hey @meziantou, this has now been approved, are you still interested on contributing an implementation? If not I already have an initial prototype but wanted to check with you first before I finished it and send out the PR review.

@meziantou
Copy link
Contributor

@joperezr I'm on vacation for 2 weeks, so I won't be able to contribute at the moment. If you already have a prototype, you can create a PR. I will review the change when I get back to work and check if I can add something.

@joperezr
Copy link
Member

joperezr commented May 4, 2022

Sounds good, enjoy your time off.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 6, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 26, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.RegularExpressions blocking Marks issues that we want to fast track in order to unblock other important work code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants