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

Fix location where inheritdoc doc comment is placed #76024

Merged
merged 1 commit into from
Nov 22, 2024
Merged

Fix location where inheritdoc doc comment is placed #76024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 23 additions & 33 deletions src/Analyzers/CSharp/CodeFixes/AddInheritdoc/AddInheritdocCodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;
using Microsoft.CodeAnalysis.CSharp.Extensions;

namespace Microsoft.CodeAnalysis.CSharp.CodeFixes.AddInheritdoc;

using static CSharpSyntaxTokens;
using static SyntaxFactory;

[ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.AddInheritdoc), Shared]
[method: ImportingConstructor]
Expand All @@ -37,33 +37,21 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var document = context.Document;
var cancellationToken = context.CancellationToken;
var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
if (root is null)
{
return;
}
var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

it's a c# fixer. it always has a root.


SemanticModel? semanticModel = null;
var semanticModel = await context.Document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

semantic models cost nothing in fixers. no point deferring.

foreach (var diagnostic in context.Diagnostics)
{
var node = root.FindNode(diagnostic.Location.SourceSpan);
if (node.Kind() is not SyntaxKind.MethodDeclaration and not SyntaxKind.PropertyDeclaration and not SyntaxKind.VariableDeclarator)
{
continue;
}

if (node.IsKind(SyntaxKind.VariableDeclarator) && node.Parent?.Parent?.IsKind(SyntaxKind.EventFieldDeclaration) == false)
{
if (node.IsKind(SyntaxKind.VariableDeclarator) && node is not { Parent.Parent: EventFieldDeclarationSyntax })
continue;
}

semanticModel ??= await context.Document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);

var symbol = semanticModel.GetDeclaredSymbol(node, cancellationToken);
if (symbol is null)
{
continue;
}

if (symbol.Kind is SymbolKind.Method or SymbolKind.Property or SymbolKind.Event)
{
Expand All @@ -78,21 +66,15 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)

protected override async Task FixAllAsync(Document document, ImmutableArray<Diagnostic> diagnostics, SyntaxEditor editor, CancellationToken cancellationToken)
{
string? newLine = null;
SourceText? sourceText = null;
var options = await document.GetLineFormattingOptionsAsync(cancellationToken).ConfigureAwait(false);
var newLine = options.NewLine;
var sourceText = await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

we only got here because we want to fix things. deferring any of this stuff is pointless. the compiler already said there was a problem.


foreach (var diagnostic in diagnostics)
{
var node = editor.OriginalRoot.FindNode(diagnostic.Location.SourceSpan);
if (node.IsKind(SyntaxKind.VariableDeclarator) && !(node = node.Parent?.Parent).IsKind(SyntaxKind.EventFieldDeclaration))
{
continue;
}

if (newLine == null)
{
var options = await document.GetLineFormattingOptionsAsync(cancellationToken).ConfigureAwait(false);
newLine = options.NewLine;
}
if (node is VariableDeclaratorSyntax { Parent.Parent: EventFieldDeclarationSyntax eventField })
node = eventField;

// We can safely assume, that there is no leading doc comment, because that is what CS1591 is telling us.
// So we create a new /// <inheritdoc/> comment.
Expand All @@ -112,13 +94,21 @@ protected override async Task FixAllAsync(Document document, ImmutableArray<Diag
],
endOfComment: EndOfDocumentationCommentToken.WithoutTrivia());

sourceText ??= await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
var indentation = sourceText.GetLeadingWhitespaceOfLineAtPosition(node.FullSpan.Start);
var indentation = sourceText.GetLeadingWhitespaceOfLineAtPosition(node.Span.Start);
var newLeadingTrivia = TriviaList(
Whitespace(indentation),
Trivia(singleLineInheritdocComment));

editor.ReplaceNode(node, node.WithPrependedLeadingTrivia(newLeadingTrivia));
// Insert the new trivia after the existing trivia for the member (but before the whitespace indentation
// trivia on the line it starts on)
var finalLeadingTrivia = node.GetLeadingTrivia().ToList();
var insertionIndex = finalLeadingTrivia.Count;

if (finalLeadingTrivia is [.., (kind: SyntaxKind.WhitespaceTrivia)])
insertionIndex--;

finalLeadingTrivia.InsertRange(insertionIndex, newLeadingTrivia);
editor.ReplaceNode(node, node.WithLeadingTrivia(finalLeadingTrivia));
}
}
}
91 changes: 75 additions & 16 deletions src/Analyzers/CSharp/Tests/AddInheritdoc/AddInheritdocTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.CodeAnalysis.Testing;
using Roslyn.Test.Utilities;
using Xunit;

namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics.AddInheritdoc;
Expand All @@ -16,24 +17,21 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics.AddInheritd
AddInheritdocCodeFixProvider>;

[Trait(Traits.Feature, Traits.Features.CodeActionsAddInheritdoc)]
public class AddInheritdocTests
public sealed class AddInheritdocTests
{
private static async Task TestAsync(string initialMarkup, string expectedMarkup)
{
var test = new VerifyCS.Test
private static Task TestAsync(string initialMarkup, string expectedMarkup)
=> new VerifyCS.Test
{
TestCode = initialMarkup,
FixedCode = expectedMarkup,
CodeActionValidationMode = CodeActionValidationMode.Full,
};
await test.RunAsync();
}
}.RunAsync();

private static async Task TestMissingAsync(string initialMarkup)
=> await VerifyCS.VerifyCodeFixAsync(initialMarkup, initialMarkup);
private static Task TestMissingAsync(string initialMarkup)
=> VerifyCS.VerifyCodeFixAsync(initialMarkup, initialMarkup);

[Fact]
public async Task AddMissingInheritdocOnOverridenMethod()
public async Task AddMissingInheritdocOnOverriddenMethod()
{
await TestAsync(
"""
Expand Down Expand Up @@ -69,7 +67,7 @@ public override void M() { }
[InlineData("public void {|CS1591:OtherMethod|}() { }")]
[InlineData("public void {|CS1591:M|}() { }")]
[InlineData("public new void {|CS1591:M|}() { }")]
public async Task DoNotOfferOnNotOverridenMethod(string methodDefintion)
public async Task DoNotOfferOnNotOverriddenMethod(string methodDefinition)
{
await TestMissingAsync(
$$"""
Expand All @@ -82,7 +80,7 @@ public virtual void M() { }
/// Some doc.
public class Derived: BaseClass
{
{{methodDefintion}}
{{methodDefinition}}
}
""");
}
Expand Down Expand Up @@ -139,7 +137,7 @@ void IInterface.M() { }
""");
}
[Fact]
public async Task AddMissingInheritdocOnOverridenProperty()
public async Task AddMissingInheritdocOnOverriddenProperty()
{
await TestAsync(
"""
Expand Down Expand Up @@ -269,8 +267,8 @@ public virtual void M() { }
/// Some doc.
public class Derived: BaseClass
{
/// <inheritdoc/>
// Comment
/// <inheritdoc/>
Copy link
Member Author

Choose a reason for hiding this comment

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

the new form is as better than the old form.

public override void M() { }
}
""");
Expand Down Expand Up @@ -304,9 +302,9 @@ public virtual void M() { }
/// Some doc.
public class Derived: BaseClass
{
/// <inheritdoc/>
// Comment 1
/* Comment 2 */ public /* Comment 3 */ override void M /* Comment 4 */ () /* Comment 5 */ { } /* Comment 6 */
/* Comment 2 */ /// <inheritdoc/>
public /* Comment 3 */ override void M /* Comment 4 */ () /* Comment 5 */ { } /* Comment 6 */
Copy link
Member Author

Choose a reason for hiding this comment

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

this test case is pointless. no one writes cod ethis way. keeping things the same as they were before is not a desire here.

}
""");
}
Expand Down Expand Up @@ -397,4 +395,65 @@ public override void M() { }
}
""");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/61562")]
public async Task TestOverrideBelowMember()
{
await TestAsync(
"""
using System;

/// <summary>
/// Hello C1
/// </summary>
public abstract class Class1
{
/// <summary>
/// Hello C1.DoStuff
/// </summary>
public abstract void DoStuff();
}

/// <summary>
/// Hello C2
/// </summary>
public class Class2 : Class1
{
private const int Number = 1;

public override void {|CS1591:DoStuff|}()
{
throw new NotImplementedException();
}
}
""",
"""
using System;

/// <summary>
/// Hello C1
/// </summary>
public abstract class Class1
{
/// <summary>
/// Hello C1.DoStuff
/// </summary>
public abstract void DoStuff();
}

/// <summary>
/// Hello C2
/// </summary>
public class Class2 : Class1
{
private const int Number = 1;

/// <inheritdoc/>
public override void {|CS1591:DoStuff|}()
{
throw new NotImplementedException();
}
}
""");
}
}
Loading