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

Add an option to ignore aliases when moving using directives outside a namespace #77291

Merged
merged 3 commits into from
Feb 20, 2025
Merged

Add an option to ignore aliases when moving using directives outside a namespace #77291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fb5d62
Add an option to ignore aliases when moving using directives outside …
CyrusNajmabadi Feb 20, 2025
4d21154
Add test
CyrusNajmabadi Feb 20, 2025
aa00cb4
Add work items
CyrusNajmabadi Feb 20, 2025
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
12 changes: 10 additions & 2 deletions ...s/CSharp/Analyzers/MisplacedUsingDirectives/MisplacedUsingDirectivesDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,22 @@ protected override void InitializeWorker(AnalysisContext context)
private void AnalyzeNamespaceNode(SyntaxNodeAnalysisContext context)
{
var option = context.GetCSharpAnalyzerOptions().UsingDirectivePlacement;
if (option.Value != AddImportPlacement.OutsideNamespace
if (option.Value == AddImportPlacement.InsideNamespace
|| ShouldSkipAnalysis(context, option.Notification))
{
return;
}

var namespaceDeclaration = (BaseNamespaceDeclarationSyntax)context.Node;
ReportDiagnostics(context, s_outsideDiagnosticDescriptor, namespaceDeclaration.Usings, option);
ReportDiagnostics(
context,
s_outsideDiagnosticDescriptor,
// Move all usings outside of the namespace. Ignore using-aliases if the user has set the option saying
// that they're ok with them inside a namespace.
option.Value is AddImportPlacement.OutsideNamespaceIgnoringAliases
? namespaceDeclaration.Usings.Where(u => u.Alias is null)
: namespaceDeclaration.Usings,
option);
}

private void AnalyzeCompilationUnitNode(SyntaxNodeAnalysisContext context)
Expand Down
36 changes: 25 additions & 11 deletions ...zers/CSharp/CodeFixes/MisplacedUsingDirectives/MisplacedUsingDirectivesCodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Simplification;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Simplification;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Formatting;
Expand All @@ -24,10 +25,11 @@
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Simplification;
using Roslyn.Utilities;
using Microsoft.CodeAnalysis.CSharp.CodeStyle;

namespace Microsoft.CodeAnalysis.CSharp.MisplacedUsingDirectives;

using static SyntaxFactory;

/// <summary>
/// Implements a code fix for all misplaced using statements.
/// </summary>
Expand Down Expand Up @@ -149,7 +151,7 @@ private static async Task<Document> GetTransformedDocumentAsync(

var newCompilationUnit = placement == AddImportPlacement.InsideNamespace
? MoveUsingsInsideNamespace(compilationUnitWithoutHeader)
: MoveUsingsOutsideNamespaces(compilationUnitWithoutHeader);
: MoveUsingsOutsideNamespaces(compilationUnitWithoutHeader, ignoringAliases: placement == AddImportPlacement.OutsideNamespaceIgnoringAliases);

// Re-attach the header now that using have been moved and LeadingTrivia is no longer being altered.
var newCompilationUnitWithHeader = AddFileHeader(newCompilationUnit, fileHeader);
Expand Down Expand Up @@ -213,11 +215,13 @@ private static CompilationUnitSyntax MoveUsingsInsideNamespace(CompilationUnitSy
return compilationUnitWithoutBlankLine.ReplaceNode(namespaceDeclaration, namespaceDeclarationWithUsings);
}

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

// Replace the namespace declarations in the compilation with the ones without using directives.
var compilationUnitWithReplacedNamespaces = compilationUnit.ReplaceNodes(
Expand Down Expand Up @@ -247,23 +251,33 @@ private static CompilationUnitSyntax MoveUsingsOutsideNamespaces(CompilationUnit
}

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

// Get the using directives from the namespaces.
var usingsFromNamespaces = namespaceDeclarationMap.Values.SelectMany(result => result.usingsFromNamespace);
var allUsings = usingContainer.Usings.AsEnumerable().Concat(usingsFromNamespaces).ToImmutableArray();
var usings = ignoringAliases
? usingContainer.Usings.Where(u => u.Alias is null)
: usingContainer.Usings;
var allUsings = usings.Concat(usingsFromNamespaces).ToImmutableArray();

// Replace the namespace declarations in the compilation with the ones without using directives.
var namespaceDeclarationWithReplacedNamespaces = usingContainer.ReplaceNodes(
namespaceDeclarations, (node, _) => namespaceDeclarationMap[node].namespaceWithoutUsings);

// Remove usings and fix leading trivia for namespace declaration.
var namespaceDeclarationWithoutUsings = namespaceDeclarationWithReplacedNamespaces.WithUsings(default);
var namespaceDeclarationWithoutBlankLine = RemoveLeadingBlankLinesFromFirstMember(namespaceDeclarationWithoutUsings);
var namespaceDeclarationWithoutUsings = namespaceDeclarationWithReplacedNamespaces
.WithUsings(ignoringAliases
? List(namespaceDeclarationWithReplacedNamespaces.Usings.Where(u => u.Alias != null))
: default);

var namespaceDeclarationWithoutBlankLine = namespaceDeclarationWithoutUsings.Usings.Count == 0
? RemoveLeadingBlankLinesFromFirstMember(namespaceDeclarationWithoutUsings)
: namespaceDeclarationWithoutUsings;

return (namespaceDeclarationWithoutBlankLine, allUsings);
}
Expand Down Expand Up @@ -372,7 +386,7 @@ private static (AddImportPlacement placement, bool preferPreservation) Determine
var placement = styleOption.Value;
var preferPreservation = styleOption.Notification == NotificationOption2.None;

if (preferPreservation || placement == AddImportPlacement.OutsideNamespace)
if (preferPreservation || placement != AddImportPlacement.InsideNamespace)
return (placement, preferPreservation);

// Determine if we can safely apply the InsideNamespace preference.
Expand Down
128 changes: 114 additions & 14 deletions src/Analyzers/CSharp/Tests/MisplacedUsingDirectives/MisplacedUsingDirectivesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.AddImport;
using Microsoft.CodeAnalysis.CodeFixes;
Expand All @@ -19,13 +20,9 @@

namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.MisplacedUsingDirectives;

public class MisplacedUsingDirectivesTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor
public sealed class MisplacedUsingDirectivesTests(ITestOutputHelper logger)
: AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor(logger)
{
public MisplacedUsingDirectivesTests(ITestOutputHelper logger)
: base(logger)
{
}

internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace)
=> (new MisplacedUsingDirectivesDiagnosticAnalyzer(), new MisplacedUsingDirectivesCodeFixProvider());

Expand All @@ -41,43 +38,54 @@ internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProvider
internal static readonly CodeStyleOption2<AddImportPlacement> OutsideNamespaceOption =
new(AddImportPlacement.OutsideNamespace, NotificationOption2.Error);

protected const string ClassDefinition = """
internal static readonly CodeStyleOption2<AddImportPlacement> OutsideNamespaceIgnoringAliasesOption =
new(AddImportPlacement.OutsideNamespaceIgnoringAliases, NotificationOption2.Error);

private const string ClassDefinition = """
public class TestClass
{
}
""";

protected const string StructDefinition = """
private const string StructDefinition = """
public struct TestStruct
{
}
""";

protected const string InterfaceDefinition = """
private const string InterfaceDefinition = """
public interface TestInterface
{
}
""";

protected const string EnumDefinition = """
private const string EnumDefinition = """
public enum TestEnum
{
TestValue
}
""";

protected const string DelegateDefinition = @"public delegate void TestDelegate();";
private const string DelegateDefinition = @"public delegate void TestDelegate();";

private TestParameters GetTestParameters(CodeStyleOption2<AddImportPlacement> preferredPlacementOption)
=> new(options: new OptionsCollection(GetLanguage()) { { CSharpCodeStyleOptions.PreferredUsingDirectivePlacement, preferredPlacementOption } });

private protected Task TestDiagnosticMissingAsync(string initialMarkup, CodeStyleOption2<AddImportPlacement> preferredPlacementOption)
private Task TestDiagnosticMissingAsync(
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
CodeStyleOption2<AddImportPlacement> preferredPlacementOption)
=> TestDiagnosticMissingAsync(initialMarkup, GetTestParameters(preferredPlacementOption));

private protected Task TestMissingAsync(string initialMarkup, CodeStyleOption2<AddImportPlacement> preferredPlacementOption)
private Task TestMissingAsync(
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
CodeStyleOption2<AddImportPlacement> preferredPlacementOption)
=> TestMissingAsync(initialMarkup, GetTestParameters(preferredPlacementOption));

private protected Task TestInRegularAndScriptAsync(string initialMarkup, string expectedMarkup, CodeStyleOption2<AddImportPlacement> preferredPlacementOption, bool placeSystemNamespaceFirst)
private Task TestInRegularAndScriptAsync(
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string expectedMarkup,
CodeStyleOption2<AddImportPlacement> preferredPlacementOption,
bool placeSystemNamespaceFirst)
{
var options = new OptionsCollection(GetLanguage())
{
Expand Down Expand Up @@ -715,6 +723,98 @@ namespace N1

#endregion

#region OutsideNamespaceIgnoringAliases

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/43271")]
public Task WhenOutsideIgnoringAliasesPreferred_UsingsInNamespace_UsingsMoved()
{
var testCode = """
namespace TestNamespace
{
[|using System;
using System.Threading;|]
using SCG = System.Collections.Generic;
}
""";
var fixedTestCode = """
{|Warning:using System;|}
{|Warning:using System.Threading;|}

namespace TestNamespace
{
using SCG = System.Collections.Generic;
}
""";

return TestInRegularAndScriptAsync(testCode, fixedTestCode, OutsideNamespaceIgnoringAliasesOption, placeSystemNamespaceFirst: true);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/43271")]
public Task WhenOutsideIgnoringAliasesPreferred_UsingsInNamespace_UsingsMoved_InnerType()
{
var testCode = """
namespace TestNamespace
{
[|using System;
using System.Threading;|]
using SCG = System.Collections.Generic;

class C
{
}
}
""";
var fixedTestCode = """
{|Warning:using System;|}
{|Warning:using System.Threading;|}

namespace TestNamespace
{
using SCG = System.Collections.Generic;

class C
{
}
}
""";

return TestInRegularAndScriptAsync(testCode, fixedTestCode, OutsideNamespaceIgnoringAliasesOption, placeSystemNamespaceFirst: true);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/43271")]
public Task WhenOutsideIgnoringAliasesPreferred_UsingsInNamespace_UsingsMoved_AliasInMiddle()
{
var testCode = """
namespace TestNamespace
{
[|using System;
using SCG = System.Collections.Generic;
using System.Threading;|]

class C
{
}
}
""";
var fixedTestCode = """
{|Warning:using System;|}
{|Warning:using System.Threading;|}

namespace TestNamespace
{
using SCG = System.Collections.Generic;

class C
{
}
}
""";

return TestInRegularAndScriptAsync(testCode, fixedTestCode, OutsideNamespaceIgnoringAliasesOption, placeSystemNamespaceFirst: true);
}

#endregion

#region Test InsideNamespace

/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions .../SharedUtilitiesAndExtensions/Compiler/CSharp/CodeStyle/CSharpCodeStyleOptions_Parsing.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public static CodeStyleOption2<AddImportPlacement> ParseUsingDirectivesPlacement
{
"inside_namespace" => new CodeStyleOption2<AddImportPlacement>(AddImportPlacement.InsideNamespace, notification),
"outside_namespace" => new CodeStyleOption2<AddImportPlacement>(AddImportPlacement.OutsideNamespace, notification),
"outside_namespace_ignoring_aliases" => new CodeStyleOption2<AddImportPlacement>(AddImportPlacement.OutsideNamespaceIgnoringAliases, notification),
Copy link
Member Author

Choose a reason for hiding this comment

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

intentionally using hte word ignoring (as opposed to excluding or except) as i don't want to convey taht aliases must then be required to be inside the namespace. Instead, this is trying to say that aliases are simply ignored and can appear either outside or inside the namespace (for example, roslyn follows this model).

_ => throw new NotSupportedException(),
};
}
Expand All @@ -70,6 +71,7 @@ public static string GetUsingDirectivesPlacementEditorConfigString(CodeStyleOpti
{
AddImportPlacement.InsideNamespace => $"inside_namespace{notificationString}",
AddImportPlacement.OutsideNamespace => $"outside_namespace{notificationString}",
AddImportPlacement.OutsideNamespaceIgnoringAliases => $"outside_namespace_ignoring_aliases{notificationString}",
_ => throw new NotSupportedException(),
};
}
Expand Down
7 changes: 6 additions & 1 deletion src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/AddImportPlacement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,10 @@ internal enum AddImportPlacement
/// <summary>
/// Place imports outside the namespace definition.
/// </summary>
OutsideNamespace
OutsideNamespace,

/// <summary>
/// Place imports outside the namespace definition, ignoring import aliases (which can stay inside the namespace).
/// </summary>
OutsideNamespaceIgnoringAliases
}
Loading