Simplify UpgradeToRegexGenerator

[Youssef1313]

Previously, analyzer reported argument indices in its properties. Then codefix will get the argument values from the indices in GetNode. I changed the analyzer to report the values directly, which simplifies things a little bit.

[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

---

null

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

[Youssef1313]

Marking as a draft. Not ready for review until #72534 is merged and this branch is rebased.

[teo-tsirpanis]

This should be renamed to something like FormatRegexOptions. It's not clear what it does by the name and parameter type.

[Youssef1313]

Probably stringifiedRegexOptions to make it clear this is a RegexOptions that we called ToString on?

[Youssef1313]

@joperezr Can you take a look please? Thanks!

[joperezr]

Thanks for the PR @Youssef1313. Since you are changing this, one of the comments from the original PR that added the analyzer was particularly suggesting that ideally we shouldn't have the analyzer send any info into the fixer if possible in order to have both be easier to understand and maintain. Here is the relevant part of the comment by @mavasani:

Is there a reason to add this property bag to the diagnostic instead of computing all the required information in the fixer itself? Given the diagnostic location, the fixer can fetch the required syntax node from the location using root.FindNode(span) API. You can get the semantic model from the document using document.GetSemanticModelAsync API and then use semanticModel.GetOperation(node) API to get to the IInvocationOperation.
...
We generally don't recommend using such a property bag unless needed as you are now adding an additional dependency between the analyzer and fixer internals, which might make it more harder to maintain or understand.

I think if we are changing this now, we should re-evaluate that comment and perhaps moving the logic of grabbing the values out to the fixer itself. Thoughts?

[Youssef1313]

Sure. I'll try with that approach.

[Youssef1313]

@joperezr I applied the suggestion. The PR is ready for another look.

[joperezr]

Small NIT: The name of the method is technically not accurate any longer, now we are only doing validation of the Parameters so we could rename to ValidateParameters

[joperezr]

Looks like in other similar places we aren't adding this attribute, like:

runtime/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ConvertToLibraryImportFixer.cs

Line 23 in 39d82c9

[ExportCodeFixProvider(LanguageNames.CSharp)]

I'm not saying adding the attribute is wrong, but it also isn't very related to this PR so we should probably live that to a follow up PR.

[joperezr]

Thanks for the contribution @Youssef1313 !

[joperezr]

Will merge as soon as Build is green.