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

Add FixAll in containing type/member support for code fixes #59989

Merged
merged 32 commits into from
Apr 7, 2022

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented Mar 7, 2022

Currently, one can apply FixAll operations to Document, Project and Solution scopes. Now, we also support fixing all diagnostics in containing member and type scopes, wherever applicable.

Closes #60029

Screenshot

FixAllInContainingMemberAndType

Note

  1. Fix all in ContainingType scope fixes all diagnostics in all the partial definitions across files, not just the type declaration in current file from which the fix is triggered.
  2. If the trigger diagnostic span does not have a method level member declaration as it's containing symbol, we do not offer ContainingMember fix all option (for example make class static code fix for CA1822).
  3. Similarly, if the trigger diagnostic span does not have a type declaration as it's containing symbol, we do not offer ContainingType fix all option (for example remove unnecessary usings code fix for IDE0005).

Added Tests

  1. Unit tests: C# and VB unit tests tests for both FixAllScope.ContainingMember and FixAllScope.ContainingType in following buckets:
    1. Method body level code fix, so that both these fix all scopes are applicable for the code fix.
    2. Type declaration level code fix, so that only ContainingType fix all scope is applicable. We verify that ContainingMember fix all scope is not offered for such a code fix.
    3. File level code fix (remove unnecessary usings), so that neither of these fix all scopes are applicable. We verify that both these fix all scopes are not offered for such a code fix.
    4. FixAll tests for suppressions for both these scopes.
  2. Integration tests for both FixAllScope.ContainingMember and FixAllScope.ContainingType.

@mavasani mavasani added this to the 17.2.P2 milestone Mar 7, 2022
@mavasani mavasani marked this pull request as ready for review March 7, 2022 09:43
@mavasani mavasani requested review from 333fred, JoeRobich and a team as code owners March 7, 2022 09:43
Currently, one can apply FixAll operations to Document, Project and Solution scopes. Now, we also support fixing all diagnostics in containing member and type scopes, wherever applicable.
}
else
{
var diagnostics = ImmutableArray<Diagnostic>.Empty;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a builder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, for 99% cases we expect a single FixAll span entry here. We get multiple FixAll spans within a document only for the case where a single named type has multiple partial definitions defined in the same file, which is almost never the case in real code bases. I don't think we need to optimize for this corner case.

Copy link
Member

Choose a reason for hiding this comment

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

my general concern is simply that the builder approach is just as easy to write, and works well for the 1 to many case. I agree it's unlikely, but really rthe ImmutableArray.AddRange approach really stands out like a sort thumb as "this is just waiting for an unfortunate case. Note: sam's TemporaryArray is great here. just do:

using var diagnostics = TemporaryArray<Diagnostic>Empty;

As fast for the normal case (<4 items), and no pathlogical dropoff.

…e so TS/F# can also opt into ContainingType/Member scope for fix all.
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

done iwth pass. almost all nits. did see a concern with global statements.

@mavasani mavasani requested a review from a team as a code owner March 25, 2022 09:38
@mavasani
Copy link
Contributor Author

Thanks @CyrusNajmabadi - feedback addressed

@mavasani mavasani enabled auto-merge April 7, 2022 09:24
@mavasani mavasani merged commit a6d0805 into dotnet:main Apr 7, 2022
@ghost ghost modified the milestones: 17.3, Next Apr 7, 2022
@mavasani mavasani deleted the FixAllCodeFixes branch April 7, 2022 11:10
@dibarbet dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make FixAll APIs for ContainingType/Member scope public
5 participants