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

A better descriptor Id for an F# analyzer #69313

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

psfinaki
Copy link
Member

@psfinaki psfinaki commented Aug 1, 2023

Related to this ticket.

Basically we ended up having our compiler and this analyzer often emitting the same diagnostics which causes confusion. The right way is to distinguish them, this is what C# compiler does and what other F# analyzers to here, so it's basically aligning the approach.

Picking IDE0059 because this is the closest match to what we need.

@psfinaki psfinaki requested a review from a team as a code owner August 1, 2023 16:32
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 1, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Aug 1, 2023
@psfinaki
Copy link
Member Author

psfinaki commented Aug 1, 2023

cc @dotnet/fsharp-team-msft @T-Gro @tmat

@tmat
Copy link
Member

tmat commented Aug 7, 2023

@mavasani Does this look good?

@mavasani
Copy link
Contributor

mavasani commented Aug 7, 2023

Tagging @gewarren for confirmation from documentation perspective.

Regarding re-using diagnostic IDs across .NET languages, we are already using the same diagnostic ID across two .NET languages (C# and VB), so using it for F# as well seems reasonable.

However, the IDE0059 flags unused value assignments, not unused variables. There is separate language specific compiler diagnostic ID for the latter. For example https://learn.microsoft.com/en-us/dotnet/csharp/misc/cs0219 for C#. I am not sure IDE0059 is the diagnostic ID that you want for F# unused declarations.

@psfinaki
Copy link
Member Author

psfinaki commented Aug 7, 2023

@mavasani so this is complicated :)

There are compiler diagnostics and then there are diagnostics from analyzers - serving different purposes. In F#, we have a similar thing to CS0219 and that's FS1182 - the diag number I am replacing in this PR, so that we can distinguish diags in a similar fashion to C#.

A different issue is that in C#, there are different IDE diags for different kinds of "unnecessary" stuff, in particular IDE0058, IDE0059, IDE0060. And that's how it should be, otherwise handling them all is a nightmare for code fixes. Eventually that's what I'd like to have in F# as well because yes now the analyzer and the code fixer try to handle all those cases and do so poorly. What is really handled right now mostly resembles IDE0059 hence the choice.

But yeah I believe we'll add more analyzers in the future to better align with C# here and make life easier for us as well.

@gewarren
Copy link
Contributor

gewarren commented Aug 7, 2023

If this is merged, please create an issue in the https://github.com/dotnet/docs repo so I can update the docs for IDE0059.

@mavasani
Copy link
Contributor

mavasani commented Aug 8, 2023

What is really handled right now mostly resembles IDE0059 hence the choice.

Alright, I got a bit confused by the analyzer name. It seems fine to me to re-use this ID. Please do create a tracking doc issue and work with @gewarren to get the docs updated appropriately.

@psfinaki
Copy link
Member Author

psfinaki commented Aug 8, 2023

@gewarren @mavasani there you go.

Thanks! As for the docs, actually it would make sense to update the docs about the other two guys in this folder as well - added this to the ticket above.

@psfinaki
Copy link
Member Author

I don't seem to have the rights to set this for auto-merge - could you let this in? :)

@mavasani mavasani merged commit 1368be9 into dotnet:main Aug 10, 2023
24 checks passed
@ghost ghost added this to the Next milestone Aug 10, 2023
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants