Simplify language version check in Regex generator

[Youssef1313]

No description provided.

[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]

Ideally, MethodSyntax shouldn't exist in the RegexMethod model at all. I could eliminate this usage, but not the other one. (the other usage can be changed to be Location instead of MethodDeclarationSyntax but that is not good as well)

So I simplified this usage for now.

Someone from Roslyn team should be able to advise about the other usage.

[stephentoub]

Simplify language version check in Regex generator

This isn't really "simpler". What are the benefits of this change? Without it, do we not correctly handle regenerating if the language version changes?

[Youssef1313]

Simplify language version check in Regex generator

This isn't really "simpler". What are the benefits of this change? Without it, do we not correctly handle regenerating if the language version changes?

It's not good to have MethodSyntax in the model. My goal was to eliminate its usage (there are two usages). One that is eliminated in this PR, but the other was hard to get rid of.

So, ideally, to have something more meaningful, the other usage should be eliminated as well. But I thought it's good to eliminate one usage for now.

The reason MethodSyntax isn't good to have in the middle is that it might not compare equal across different runs.

[Youssef1313]

Tagging @CyrusNajmabadi and @333fred as well.

[stephentoub]

The reason MethodSyntax isn't good to have in the middle is that it might not compare equal across different runs.

Can you point to where such comparisons are being performed and how that would impact things?

I'm fine with the change if there's benefit, I was just highlighting I don't think the change makes anything in the code "simpler" as the title suggests, so I'm trying to understand the ramifications of the change. I assume the primary benefit is if we're not currently including the language version in CompilationData type then we won't regenerate the output if/when the language version is changed?

[Youssef1313]

The reason MethodSyntax isn't good to have in the middle is that it might not compare equal across different runs.

Can you point to where such comparisons are being performed and how that would impact things?

I'm fine with the change if there's benefit, I was just highlighting I don't think the change makes anything in the code "simpler" as the title suggests, so I'm trying to understand the ramifications of the change. I assume the primary benefit is if we're not currently including the language version in CompilationData type then we won't regenerate the output if/when the language version is changed?

In both cases we will regenerate when language version changes. To be clear, the change in this PR alone doesn't have benefits on its own, but I think it is on the correct path to achieve a benefit. See discussion in dotnet/roslyn-analyzers#6352.

Regarding "where the comparisons are done", I'm not familiar with the compiler implementation in this area. But it is simply that when RegexMethod is cached, they cannot compare equal again if any change is done in the file containing this method.

[stephentoub]

Well, the intent of CompilationData:
https://github.com/dotnet/runtime/pull/81678/files#diff-1a7038f4ee623715d84de6ead48a49935bbbe3d9e95f81dfb77fbd8f891f85b1R39
was that it hold relevant state to be pulled out of the settings; for that reason alone, it's fine to make this change to pull the language version into it, as it should have been like that from the beginning.

But separate from that, if the problem is that the object being returned from a stage in the pipeline contains a reference to a MethodSyntax, and replacing that with a Location isn't sufficient to thwart the issues being cited, then this really doesn't do anything other than move the cheese around. How is code later in a pipeline supposed to report diagnostics if it can't reference the Location to report them at? It's not feasible to report all diagnostics as part of the very first discovery stage, unless the entire pipeline is collapsed down to that single stage, which then also defeats the purpose.

[Youssef1313]

Well, the intent of CompilationData: https://github.com/dotnet/runtime/pull/81678/files#diff-1a7038f4ee623715d84de6ead48a49935bbbe3d9e95f81dfb77fbd8f891f85b1R39 was that it hold relevant state to be pulled out of the settings; for that reason alone, it's fine to make this change to pull the language version into it, as it should have been like that from the beginning.

But separate from that, if the problem is that the object being returned from a stage in the pipeline contains a reference to a MethodSyntax, and replacing that with a Location isn't sufficient to thwart the issues being cited, then this really doesn't do anything other than move the cheese around. How is code later in a pipeline supposed to report diagnostics if it can't reference the Location to report them at? It's not feasible to report all diagnostics as part of the very first discovery stage, unless the entire pipeline is collapsed down to that single stage, which then also defeats the purpose.

Location stores the tree I believe, so that is not sufficient. IMO, it might be worth looking into separating the diagnostics to a separate DiagnosticAnalyzer to avoid having MethodSyntax or Location in the RegexMethod model.

[stephentoub]

IMO, it might be worth looking into separating the diagnostics to a separate DiagnosticAnalyzer to avoid having MethodSyntax or Location in the RegexMethod model.

That would end up duplicating a lot of the work that's being performed, as the generator would still need to perform all of the same work (the outcome of that work fuels not just the diagnostics but the actual generated code). That's really the model we encourage?