F# unnecessary parentheses analyzer shim

[brianrourkeboll]

Changes

• Add a shim for the F# "IDE0047: remove unnecessary parentheses" analyzer.

Background

• F# Shim - Round 2 #35591 (I followed the approach used for these existing shims)
• Analyzer & code fix for ~IDE0047~ FS3583: remove unnecessary parentheses fsharp#16078,
• Analyzer & code fix for ~IDE0047~ FS3583: remove unnecessary parentheses fsharp#16079 (comment)
• Analyzer & code fix for ~IDE0047~ FS3583: remove unnecessary parentheses fsharp#16079 (review)
• VS checkbox for controlling the "Remove parens" diag+fix fsharp#16211

CC @vzarytovskii, @psfinaki, @T-Gro

[CyrusNajmabadi]

My preference is to not go this route. We should be using lsp for new features. Especially diagnostics and code fixes.

[brianrourkeboll]

@CyrusNajmabadi

My preference is to not go this route. We should be using lsp for new features. Especially diagnostics and code fixes.

Yes, I believe that is still the F# team's long-term plan as well.

This PR is meant as a shorter-term way for us to enable the F# parentheses analyzer by default in VS.

Right now, the parentheses analyzer (when enabled) is called in series with some other critical editor functionality, which means that it could theoretically adversely affect editor responsiveness, etc.

Adding this shim would remove that limitation and let us enable the analyzer by default without needing to wait for the F# LSP work to land.

Presumably this shim would simply be removed along with the existing ones whenever the F# VS integration switched over to using LSP.

I'm not familiar enough with LSP–Roslyn–VS interactions to know whether I could look down that route myself without getting involved in a bigger undertaking or interfering with the F# team's plans in that area.

I think the F# team was on board with doing something like this as a temporary measure (I hope I'm not putting words in your mouth @vzarytovskii — please correct me if I'm wrong), and I personally think it would be a shame if the parens analyzer had to wait for the full F# switchover to LSP to be enabled by default, but I'll defer to you if you think this needs to wait.

[vzarytovskii]

Yes, we definitely want it as a short term solution while we working on lsp (which will take a bunch of time).

[psfinaki]

@CyrusNajmabadi LSP transition is in works, meanwhile - this is a really powerful and useful analyzer which we'd like to promote more, so it would be great to have a shim for it for the reasons Brian described above.

[CyrusNajmabadi]

Not quite sure why this needs to be broken out. And can't just be something that runs in the normal analyzer. This is a purely syntactic check right? (It is in c#/vb at least). So it adds like zero cost over the much more expensive semantic analysis passes. Also, it's been a while since I've checked (and I'm in my phone on vacation), but don't we already split our diagnostic callbacks into syntax and semantic callbacks? So this would just need to be added by you to the existing syntax callback we make into you.

In other words, I think you should already be able to provide support for this feature without needing to add these specialized EA interfaces :-)

[vzarytovskii]

Not quite sure why this needs to be broken out. And can't just be something that runs in the normal analyzer. This is a purely syntactic check right? (It is in c#/vb at least). So it adds like zero cost over the much more expensive semantic analysis passes. Also, it's been a while since I've checked (and I'm in my phone on vacation), but don't we already split our diagnostic callbacks into syntax and semantic callbacks? So this would just need to be added by you to the existing syntax callback we make into you.

In other words, I think you should already be able to provide support for this feature without needing to add these specialized EA interfaces :-)

It for sure can be running as "normal" analyzer, and as Brian suggested, we can setup some timeout for those so they don't affect perf too much (and they way they're implemented now, they will be affecting tooling perf unfortunately). It feels more consistent with things like "remove unnecessary imports" diagnostic to make it via shims in the EA. All this code will go away eventually (when we fully move to LSP extension and deprecate current one) but it will take some time.

[CyrusNajmabadi]

It feels more consistent with things like "remove unnecessary imports" diagnostic

I see these cases very differently.

'remove unn imports' is very special. Both because it has special privileges with lightbulbs to show up above the rest and run with high priority. And also because it is hooked up to an explicit editor command "organize imports".

Remove parens is very very different. It def doesn't need to run with any special priority, and there's no special command to invoke it.

and they way they're implemented now, they will be affecting tooling perf unfortunately

Can you clarify that? Can this not just run in the AnalyzeSyntax callback without impact? I'd like to understand this better.

I also don't see how breaking this out makes perf any better. Thanks!