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.

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

Fix make static to not drop trivia #54216

Merged
merged 4 commits into from
Jul 30, 2021
Merged

Fix make static to not drop trivia #54216

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1fc8c65
Cleanup
Youssef1313 Jun 18, 2021
2bfa623
Fix make static to not drop trivia
Youssef1313 Jun 18, 2021
d099482
Fix
Youssef1313 Jun 18, 2021
6258179
Address feedback
Youssef1313 Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 40 additions & 28 deletions src/EditorFeatures/CSharpTest/MakeMemberStatic/MakeMemberStaticTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,53 +2,65 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using Microsoft.CodeAnalysis.CSharp.MakeMemberStatic;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.MakeMemberStatic;
using Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.CodeAnalysis.Testing;
using Roslyn.Test.Utilities;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.MakeMemberStatic
{
public class MakeMemberStaticTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest
using VerifyCS = CSharpCodeFixVerifier<
EmptyDiagnosticAnalyzer,
CSharpMakeMemberStaticCodeFixProvider>;

public class MakeMemberStaticTests
{
public MakeMemberStaticTests(ITestOutputHelper logger)
: base(logger)
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeMemberStatic)]
public async Task TestField()
{
await VerifyCS.VerifyCodeFixAsync(
@"
public static class Foo
{
int {|CS0708:i|};
}",
@"
public static class Foo
{
static int i;
}");
}

internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace)
=> (null, new CSharpMakeMemberStaticCodeFixProvider());

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeMemberStatic)]
public async Task TestField()
[WorkItem(54202, "https://github.com/dotnet/roslyn/issues/54202")]
public async Task TestTrivia()
{
await TestInRegularAndScript1Async(
await VerifyCS.VerifyCodeFixAsync(
@"
public static class Foo
{
int [||]i;
// comment
readonly int {|CS0708:i|};
}",
@"
public static class Foo
{
static int i;
// comment
readonly static int i;
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeMemberStatic)]
public async Task TestMethod()
{
await TestInRegularAndScript1Async(
await VerifyCS.VerifyCodeFixAsync(
@"
public static class Foo
{
void [||]M() { }
void {|CS0708:M|}() { }
}",
@"
public static class Foo
Expand All @@ -60,11 +72,11 @@ static void M() { }
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeMemberStatic)]
public async Task TestProperty()
{
await TestInRegularAndScript1Async(
await VerifyCS.VerifyCodeFixAsync(
@"
public static class Foo
{
object [||]P { get; set; }
object {|CS0708:P|} { get; set; }
}",
@"
public static class Foo
Expand All @@ -76,11 +88,11 @@ public static class Foo
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeMemberStatic)]
public async Task TestEventField()
{
await TestInRegularAndScript1Async(
await VerifyCS.VerifyCodeFixAsync(
@"
public static class Foo
{
event System.Action [||]E;
event System.Action {|CS0708:E|};
}",
@"
public static class Foo
Expand All @@ -92,15 +104,15 @@ public static class Foo
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeMemberStatic)]
public async Task FixAll()
{
await TestInRegularAndScript1Async(
await VerifyCS.VerifyCodeFixAsync(
@"namespace NS
{
public static class Foo
{
int {|FixAllInDocument:|}i;
void M() { }
object P { get; set; }
event System.Action E;
int {|CS0708:i|};
void {|CS0708:M|}() { }
object {|CS0708:P|} { get; set; }
event System.Action {|CS0708:E|};
}
}",
@"namespace NS
Expand Down
26 changes: 20 additions & 6 deletions src/Features/CSharp/Portable/MakeMemberStatic/CSharpMakeMemberStaticCodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System;
using System.Collections.Immutable;
using System.Composition;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.MakeMemberStatic;

namespace Microsoft.CodeAnalysis.CSharp.MakeMemberStatic
Expand All @@ -18,7 +18,7 @@ namespace Microsoft.CodeAnalysis.CSharp.MakeMemberStatic
internal sealed class CSharpMakeMemberStaticCodeFixProvider : AbstractMakeMemberStaticCodeFixProvider
{
[ImportingConstructor]
[SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public CSharpMakeMemberStaticCodeFixProvider()
{
}
Expand All @@ -28,8 +28,22 @@ public CSharpMakeMemberStaticCodeFixProvider()
"CS0708" // 'MyMethod': cannot declare instance members in a static class
);

protected override bool IsValidMemberNode(SyntaxNode node) =>
node is MemberDeclarationSyntax ||
(node.IsKind(SyntaxKind.VariableDeclarator) && node.Ancestors().Any(a => a.IsKind(SyntaxKind.FieldDeclaration) || a.IsKind(SyntaxKind.EventFieldDeclaration)));
protected override bool TryGetMemberDeclaration(SyntaxNode node, [NotNullWhen(true)] out SyntaxNode? memberDeclaration)
{
if (node is MemberDeclarationSyntax)
{
memberDeclaration = node;
return true;
}

if (node.IsKind(SyntaxKind.VariableDeclarator))
{
memberDeclaration = node.FirstAncestorOrSelf<MemberDeclarationSyntax>(a => a.IsKind(SyntaxKind.FieldDeclaration) || a.IsKind(SyntaxKind.EventFieldDeclaration));
Copy link
Member

Choose a reason for hiding this comment

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

instead of walking arbitrarily high, we should check for exactly the shapes we expect here. that prevents thigns like matching on a variable inside a lambda inside a field initializer. In this case, we jsut want to match node { Parent: VaraibleDeclaration { Parent: FieldDeclarationSyntax or EventFieldDeclarationSyntax } }

return memberDeclaration is not null;
}

memberDeclaration = null;
return false;
}
}
}
13 changes: 7 additions & 6 deletions src/Features/Core/Portable/MakeMemberStatic/AbstractMakeMemberStaticCodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ internal abstract class AbstractMakeMemberStaticCodeFixProvider : SyntaxEditorBa
{
internal sealed override CodeFixCategory CodeFixCategory => CodeFixCategory.Compile;

protected abstract bool IsValidMemberNode([NotNullWhen(true)] SyntaxNode? node);
protected abstract bool TryGetMemberDeclaration(SyntaxNode node, [NotNullWhen(true)] out SyntaxNode? memberDeclaration);
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need for an abstract class here, only C# is implemented. But I didn't cleanup this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a lot of optimism that this could be in VB :)

Fine to leave for now I think

Copy link
Member Author

Choose a reason for hiding this comment

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

@ryzngard The situation is the exact opposite in VB.

For C#, a static class requires all members to be static
For VB, a module requires all members to be non-Shared (they're Shared implicitly).

There is a code fix for the VB situation:

https://github.com/dotnet/roslyn/blob/main/src/Features/VisualBasic/Portable/RemoveSharedFromModuleMembers/VisualBasicRemoveSharedFromModuleMembersCodeFixProvider.vb


public sealed override Task RegisterCodeFixesAsync(CodeFixContext context)
{
if (context.Diagnostics.Length == 1 &&
IsValidMemberNode(context.Diagnostics[0].Location?.FindNode(context.CancellationToken)))
TryGetMemberDeclaration(context.Diagnostics[0].Location.FindNode(context.CancellationToken), out _))
{
context.RegisterCodeFix(
new MyCodeAction(c => FixAsync(context.Document, context.Diagnostics[0], c)),
Expand All @@ -38,12 +38,13 @@ protected sealed override Task FixAllAsync(Document document, ImmutableArray<Dia
{
for (var i = 0; i < diagnostics.Length; i++)
{
var declaration = diagnostics[i].Location?.FindNode(cancellationToken);
var declaration = diagnostics[i].Location.FindNode(cancellationToken);

if (IsValidMemberNode(declaration))
if (TryGetMemberDeclaration(declaration, out var memberDeclaration))
{
editor.ReplaceNode(declaration,
(currentDeclaration, generator) => generator.WithModifiers(currentDeclaration, DeclarationModifiers.Static));
var generator = SyntaxGenerator.GetGenerator(document);
var newNode = generator.WithModifiers(memberDeclaration, generator.GetModifiers(declaration).WithIsStatic(true));
editor.ReplaceNode(declaration, newNode);
}
}

Expand Down