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

Fix 'misplaced usings' with file scoped namespaces #54943

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,21 @@
// 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.Collections.Immutable;
using Microsoft.CodeAnalysis.AddAccessibilityModifiers;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Formatting;
using Microsoft.CodeAnalysis.CSharp.LanguageServices;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Shared.Extensions;

namespace Microsoft.CodeAnalysis.CSharp.AddAccessibilityModifiers
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal class CSharpAddAccessibilityModifiersDiagnosticAnalyzer
: AbstractAddAccessibilityModifiersDiagnosticAnalyzer<CompilationUnitSyntax>
{
public CSharpAddAccessibilityModifiersDiagnosticAnalyzer()
{
}

private static CSharpSyntaxFacts SyntaxFacts => CSharpSyntaxFacts.Instance;

protected override void ProcessCompilationUnit(
Expand All @@ -38,22 +32,18 @@ private void ProcessMembers(
SyntaxList<MemberDeclarationSyntax> members)
{
foreach (var memberDeclaration in members)
{
ProcessMemberDeclaration(context, option, memberDeclaration);
}
}

private void ProcessMemberDeclaration(
SyntaxTreeAnalysisContext context,
CodeStyleOption2<AccessibilityModifiersRequired> option, MemberDeclarationSyntax member)
{
if (member.IsKind(SyntaxKind.NamespaceDeclaration, out NamespaceDeclarationSyntax namespaceDeclaration))
{
if (member is BaseNamespaceDeclarationSyntax namespaceDeclaration)
ProcessMembers(context, option, namespaceDeclaration.Members);
}

// If we have a class or struct, recurse inwards.
if (member.IsKind(SyntaxKind.ClassDeclaration, out TypeDeclarationSyntax typeDeclaration) ||
if (member.IsKind(SyntaxKind.ClassDeclaration, out TypeDeclarationSyntax? typeDeclaration) ||
member.IsKind(SyntaxKind.StructDeclaration, out typeDeclaration) ||
member.IsKind(SyntaxKind.RecordDeclaration, out typeDeclaration) ||
member.IsKind(SyntaxKind.RecordStructDeclaration, out typeDeclaration))
Expand Down Expand Up @@ -99,7 +89,7 @@ private void ProcessMemberDeclaration(
return;
}

var parentKind = member.Parent.Kind();
var parentKind = member.GetRequiredParent().Kind();
switch (parentKind)
{
// Check for default modifiers in namespace and outside of namespace
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// 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.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
Expand Down Expand Up @@ -52,19 +50,17 @@ public override DiagnosticAnalyzerCategory GetAnalyzerCategory()

protected override void InitializeWorker(AnalysisContext context)
{
context.RegisterSyntaxNodeAction(AnalyzeNamespaceNode, SyntaxKind.NamespaceDeclaration);
context.RegisterSyntaxNodeAction(AnalyzeNamespaceNode, SyntaxKind.NamespaceDeclaration, SyntaxKind.FileScopedNamespaceDeclaration);
context.RegisterSyntaxNodeAction(AnalyzeCompilationUnitNode, SyntaxKind.CompilationUnit);
}

private void AnalyzeNamespaceNode(SyntaxNodeAnalysisContext context)
{
var option = context.Options.GetOption(CSharpCodeStyleOptions.PreferredUsingDirectivePlacement, context.Node.SyntaxTree, context.CancellationToken);
if (option.Value != AddImportPlacement.OutsideNamespace)
{
return;
}

var namespaceDeclaration = (NamespaceDeclarationSyntax)context.Node;
var namespaceDeclaration = (BaseNamespaceDeclarationSyntax)context.Node;
ReportDiagnostics(context, s_outsideDiagnosticDescriptor, namespaceDeclaration.Usings, option);
}

Expand All @@ -89,7 +85,7 @@ private static bool ShouldSuppressDiagnostic(CompilationUnitSyntax compilationUn
// Suppress if there are nodes other than usings and namespaces in the
// compilation unit (including ExternAlias).
return compilationUnit.ChildNodes().Any(
t => !t.IsKind(SyntaxKind.UsingDirective, SyntaxKind.NamespaceDeclaration));
t => !t.IsKind(SyntaxKind.UsingDirective, SyntaxKind.NamespaceDeclaration, SyntaxKind.FileScopedNamespaceDeclaration));
}

private static void ReportDiagnostics(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// 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.Generic;
using System.Collections.Immutable;
Expand Down Expand Up @@ -62,7 +60,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
var document = context.Document;
var cancellationToken = context.CancellationToken;

var syntaxRoot = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var syntaxRoot = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var compilationUnit = (CompilationUnitSyntax)syntaxRoot;

#if CODE_STYLE
Expand All @@ -76,9 +74,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
// it manually.
var (placement, preferPreservation) = DeterminePlacement(compilationUnit, options);
if (preferPreservation)
{
return;
}

foreach (var diagnostic in context.Diagnostics)
{
Expand All @@ -90,7 +86,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)

internal static async Task<Document> TransformDocumentIfRequiredAsync(Document document, CancellationToken cancellationToken)
{
var syntaxRoot = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var syntaxRoot = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var compilationUnit = (CompilationUnitSyntax)syntaxRoot;

#if CODE_STYLE
Expand All @@ -114,10 +110,10 @@ private static async Task<Document> GetTransformedDocumentAsync(
AddImportPlacement placement,
CancellationToken cancellationToken)
{
var syntaxFactsService = document.GetLanguageService<ISyntaxFactsService>();
var syntaxFactsService = document.GetRequiredLanguageService<ISyntaxFactsService>();

// Expand usings so that they can be properly simplified after they are relocated.
var compilationUnitWithExpandedUsings = (CompilationUnitSyntax)await ExpandUsingDirectivesAsync(document, compilationUnit, cancellationToken).ConfigureAwait(false);
var compilationUnitWithExpandedUsings = await ExpandUsingDirectivesAsync(document, compilationUnit, cancellationToken).ConfigureAwait(false);

// Remove the file header from the compilation unit so that we do not lose it when making changes to usings.
var (compilationUnitWithoutHeader, fileHeader) = RemoveFileHeader(compilationUnitWithExpandedUsings, syntaxFactsService);
Expand All @@ -138,11 +134,12 @@ private static async Task<Document> GetTransformedDocumentAsync(
return await Simplifier.ReduceAsync(newDocument, Simplifier.Annotation, options, cancellationToken).ConfigureAwait(false);
}

private static async Task<SyntaxNode> ExpandUsingDirectivesAsync(Document document, CompilationUnitSyntax containerNode, CancellationToken cancellationToken)
private static async Task<CompilationUnitSyntax> ExpandUsingDirectivesAsync(Document document, CompilationUnitSyntax containerNode, CancellationToken cancellationToken)
{
// Get all using directives so they can be expanded at one time.
var allUsingDirectives = containerNode.DescendantNodes(
node => node.IsKind(SyntaxKind.CompilationUnit, SyntaxKind.NamespaceDeclaration)).OfType<UsingDirectiveSyntax>();
var allUsingDirectives = containerNode
.DescendantNodes(node => node is CompilationUnitSyntax or BaseNamespaceDeclarationSyntax)
.OfType<UsingDirectiveSyntax>();

// Create a map between the original node and the future expanded node.
var expandUsingDirectiveTasks = allUsingDirectives.ToDictionary(
Expand All @@ -153,7 +150,7 @@ private static async Task<SyntaxNode> ExpandUsingDirectivesAsync(Document docume
await Task.WhenAll(expandUsingDirectiveTasks.Values).ConfigureAwait(false);

// Replace using directives with their expanded version.
return ((SyntaxNode)containerNode).ReplaceNodes(
return containerNode.ReplaceNodes(
expandUsingDirectiveTasks.Keys,
(node, _) => expandUsingDirectiveTasks[node].Result);
}
Expand All @@ -176,7 +173,7 @@ private static CompilationUnitSyntax MoveUsingsInsideNamespace(CompilationUnitSy
var compilationUnitWithoutBlankLine = RemoveLeadingBlankLinesFromFirstMember(compilationUnitWithoutUsings);

// Fix the leading trivia for the namespace declaration.
var namespaceDeclaration = (NamespaceDeclarationSyntax)compilationUnitWithoutBlankLine.Members[0];
var namespaceDeclaration = (BaseNamespaceDeclarationSyntax)compilationUnitWithoutBlankLine.Members[0];
var namespaceDeclarationWithBlankLine = EnsureLeadingBlankLineBeforeFirstMember(namespaceDeclaration);

// Update the namespace declaration with the usings from the compilation unit.
Expand All @@ -189,7 +186,7 @@ private static CompilationUnitSyntax MoveUsingsInsideNamespace(CompilationUnitSy

private static CompilationUnitSyntax MoveUsingsOutsideNamespaces(CompilationUnitSyntax compilationUnit, SyntaxAnnotation warningAnnotation)
{
var namespaceDeclarations = compilationUnit.Members.OfType<NamespaceDeclarationSyntax>();
var namespaceDeclarations = compilationUnit.Members.OfType<BaseNamespaceDeclarationSyntax>();
var namespaceDeclarationMap = namespaceDeclarations.ToDictionary(
namespaceDeclaration => namespaceDeclaration, namespaceDeclaration => RemoveUsingsFromNamespace(namespaceDeclaration));

Expand Down Expand Up @@ -220,10 +217,10 @@ private static CompilationUnitSyntax MoveUsingsOutsideNamespaces(CompilationUnit
return compilationUnitWithSeparatorLine.ReplaceNode(firstMember, firstMember.WithPrependedLeadingTrivia(orphanedTrivia));
}

private static (NamespaceDeclarationSyntax namespaceWithoutUsings, IEnumerable<UsingDirectiveSyntax> usingsFromNamespace) RemoveUsingsFromNamespace(
NamespaceDeclarationSyntax usingContainer)
private static (BaseNamespaceDeclarationSyntax namespaceWithoutUsings, IEnumerable<UsingDirectiveSyntax> usingsFromNamespace) RemoveUsingsFromNamespace(
BaseNamespaceDeclarationSyntax usingContainer)
{
var namespaceDeclarations = usingContainer.Members.OfType<NamespaceDeclarationSyntax>();
var namespaceDeclarations = usingContainer.Members.OfType<BaseNamespaceDeclarationSyntax>();
var namespaceDeclarationMap = namespaceDeclarations.ToDictionary(
namespaceDeclaration => namespaceDeclaration, namespaceDeclaration => RemoveUsingsFromNamespace(namespaceDeclaration));

Expand Down Expand Up @@ -281,7 +278,7 @@ private static SyntaxList<MemberDeclarationSyntax> GetMembers(SyntaxNode node)
=> node switch
{
CompilationUnitSyntax compilationUnit => compilationUnit.Members,
NamespaceDeclarationSyntax namespaceDeclaration => namespaceDeclaration.Members,
BaseNamespaceDeclarationSyntax namespaceDeclaration => namespaceDeclaration.Members,
_ => throw ExceptionUtilities.UnexpectedValue(node)
};

Expand Down Expand Up @@ -357,9 +354,7 @@ private static (AddImportPlacement placement, bool preferPreservation) Determine
var preferPreservation = codeStyleOption.Notification == NotificationOption2.None;

if (preferPreservation || placement == AddImportPlacement.OutsideNamespace)
{
return (placement, preferPreservation);
}

// Determine if we can safely apply the InsideNamespace preference.

Expand All @@ -382,8 +377,9 @@ private static (AddImportPlacement placement, bool preferPreservation) Determine
private static bool HasOneNamespace(CompilationUnitSyntax compilationUnit)
{
// Find all the NamespaceDeclarations
var allNamespaces = compilationUnit.DescendantNodes(
node => node.IsKind(SyntaxKind.CompilationUnit, SyntaxKind.NamespaceDeclaration)).OfType<NamespaceDeclarationSyntax>();
var allNamespaces = compilationUnit
.DescendantNodes(node => node is CompilationUnitSyntax or BaseNamespaceDeclarationSyntax)
.OfType<BaseNamespaceDeclarationSyntax>();

// To determine if there are multiple namespaces we only need to look for at least two.
return allNamespaces.Take(2).Count() == 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.AddAccessibilityModifiers;
using Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions;
using Microsoft.CodeAnalysis.Test.Utilities;
Expand Down Expand Up @@ -528,5 +529,24 @@ public class Derived : TestClass
}
");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)]
public async Task TestFileScopedNamespaces()
{
await new VerifyCS.Test
{
TestCode = @"
namespace Test;

struct [|S1|] { }
",
FixedCode = @"
namespace Test;

internal struct S1 { }
",
LanguageVersion = LanguageVersion.CSharp10,
}.RunAsync();
}
}
}
Loading