Skip to content

allow third party codfixers to be run in code cleanup #59631

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.

Sign up for GitHub

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

Jump to bottom
Merged
jmarolf merged 31 commits into from
Jun 14, 2022
Merged

allow third party codfixers to be run in code cleanup #59631

jmarolf merged 31 commits into from
Jun 14, 2022

Conversation

@jmarolf
Copy link
Contributor

@jmarolf jmarolf commented Feb 18, 2022 •
edited
Loading

Fixes #58089

This options looks like this in the UI:

image

To enable this functionality the user needs to

  1. have set a specific set of analyzer diagnostics as warning or error
  2. Go to the "configure code cleanup" menu

image

  1. Explicitly opt into this setting

image

3. Run the code cleanup profile where they modified that setting

image

  1. Or run the code cleanup profile on their project/solution

image

  1. Or have configured the profile to run on save

image

Rules for whether we apply a code fix with this option are as follows:

  1. The diagnostic is a warning or error
  2. The code fix for this diagnostic supports fix-all in document scope
  3. When applying the fix-all in document scope nothing outside the document changes

TODO:

  • Add a new option that is a superset of this option that allows you to "fix all warnings and errors set in editorconfig" that included formatting, code-style, and third-party analyzers.

@ghost ghost added the Area-IDE label Feb 18, 2022
@jmarolf jmarolf force-pushed the feature/run-third-party-codefixes-in-code-cleanup branch 2 times, most recently from 6d115eb to 5f62616 Compare February 18, 2022 00:24
@jmarolf jmarolf force-pushed the feature/run-third-party-codefixes-in-code-cleanup branch from 5f62616 to 811e06b Compare February 18, 2022 03:54
@jmarolf jmarolf mentioned this pull request Feb 22, 2022
Open
8 tasks
@jmarolf jmarolf force-pushed the feature/run-third-party-codefixes-in-code-cleanup branch 2 times, most recently from 5bc89f6 to 024a5df Compare February 23, 2022 01:23
@jmarolf jmarolf marked this pull request as ready for review February 23, 2022 01:29
@jmarolf jmarolf requested a review from a team as a code owner February 23, 2022 01:29
davidwengier
davidwengier reviewed Feb 23, 2022
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 23, 2022
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 23, 2022
View reviewed changes
src/Features/Core/Portable/CodeCleanup/AbstractCodeCleanupService.cs Outdated
if (enabledDiagnostics.RunThirdPartyFixers)
{
progressTracker.AddItems(1);
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add an item per 3rd party fixer?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unresolving :)

CyrusNajmabadi
CyrusNajmabadi reviewed Feb 23, 2022
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 23, 2022
View reviewed changes
src/Features/Core/Portable/CodeFixes/CodeFixService.cs
return null;

using var resultDisposer = ArrayBuilder<CodeFixCollection>.GetInstance(out var result);
var spanToDiagnostics = new SortedDictionary<TextSpan, List<DiagnosticData>>
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why a List and not an ImmutableArray?

Copy link
Contributor Author

@jmarolf jmarolf Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StreamFixesAsync takes an argument of type SortedDictionary<TextSpan, List<DiagnosticData>> here

roslyn/src/Features/Core/Portable/CodeFixes/CodeFixService.cs

Line 358 in fab4fb3

SortedDictionary<TextSpan, List<DiagnosticData>> spanToDiagnostics,

@CyrusNajmabadi I believe you wrote this code, do you recall why you used List<DiagnosticData>? Happy to change it to ImmutableArray<<DiagnosticData>>

CyrusNajmabadi
CyrusNajmabadi reviewed Feb 23, 2022
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 23, 2022
View reviewed changes
sharwell
sharwell previously requested changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In every analyzer package I've seen, at most a strict subset of analyzers had code fixes which could be safely applied without the risk of breaking code (or silently changing behavior). This feature blanket assumes that all code fixes are safe, and will either be unusable or require a complete rewrite of the analyzer severity in projects.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Feb 23, 2022 •
edited
Loading

I feel like we need an opt in or opt out option here. Preferably the former. Also, i mean: each fixer should opt into if it is safe to be used in this fashion.

@jmarolf jmarolf dismissed sharwell’s stale review February 25, 2022 00:35

addressed feedback

jmarolf and others added 18 commits June 7, 2022 14:46
@jmarolf
move old members to extension method
e5966af
@jmarolf
filter compiler diagnostics in Diagnostic Service call
32bcffa
@jmarolf
report progress per third party fixer
e737f03
@jmarolf
Do not apply fixes that change more than the current document
f5d6a2d
@jmarolf
add additional C# test cases
1323127
@jmarolf
adding additional VB tests
bbb242a
@jmarolf
use helper methods
0678c67
@jmarolf
enable nullable tracking for test fixes
3475811
@jmarolf @CyrusNajmabadi
Update src/Features/Core/Portable/CodeCleanup/AbstractCodeCleanupServ…
0c2c910 
…ice.cs

Co-authored-by: CyrusNajmabadi <[email protected]>
@jmarolf
rename method for clarity
ad987bf
@jmarolf
update progress to never "rewind" and report the correct number of fi…
be77781 
…xers up front
@jmarolf
pass a bool to let the diagnostic service filter out compiler diagnos…
c934e6c 
…tics
@jmarolf
do not make includeCompilerDiagnostics optional
5a1740a
@jmarolf
change wording to match mikas suggestion
777d073
@jmarolf
move the progress reporting to never "rewind" and instead report ever…
dbf67f3 
…ything in one list
@jmarolf
convert EnabledDiagnosticOptions to a record
23a0e4d
@jmarolf
add a an option that lets you run everything
e41f176
@jmarolf
pass parameter through to service
73f608a
@jmarolf jmarolf force-pushed the feature/run-third-party-codefixes-in-code-cleanup branch from 6e67de4 to 73f608a Compare June 7, 2022 21:46
@runfoapp runfoapp bot mentioned this pull request Jun 8, 2022
Closed
ryzngard
ryzngard approved these changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Hope to get start testing this soon!

src/Features/LanguageServer/Protocol/Features/CodeCleanup/AbstractCodeCleanupService.cs

// If changes were made to the solution snap shot outside the current document discard the changes.
// The assumption here is that if we are applying a third party code fix to a document it only affects the document.
// Symbol renames and other complex refactorings we do not want to include in code cleanup.
Copy link
Contributor

@ryzngard ryzngard Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do a follow up item to potentially log telemetry when this happens and notify the user?

@jmarolf jmarolf merged commit 06f579b into dotnet:main Jun 14, 2022
@ghost ghost added this to the Next milestone Jun 14, 2022
chsienki added a commit to chsienki/roslyn that referenced this pull request Jun 20, 2022
@chsienki
Revert "Merge pull request dotnet#59631 from jmarolf/feature/run-thir…
51618cd 
…d-party-codefixes-in-code-cleanup"

This reverts commit 06f579b, reversing
changes made to 9f6d52d.
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoeRobich JoeRobich JoeRobich left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jcouv jcouv jcouv left review comments

@mikadumont mikadumont mikadumont left review comments

@davidwengier davidwengier Awaiting requested review from davidwengier

+3 more reviewers

@sharwell sharwell sharwell left review comments

@AraHaan AraHaan AraHaan left review comments

@ryzngard ryzngard ryzngard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

17.3 P3

Development

Successfully merging this pull request may close these issues.

Code cleanup supports third party analyzers

10 participants

@jmarolf @CyrusNajmabadi @ryzngard @JoeRobich @davidwengier @sharwell @jcouv @AraHaan @mikadumont @RikkiGibson