Added CodeStyle option for 'using' placement by JoeRobich · Pull Request #30760 · dotnet/roslyn

JoeRobich · 2018-10-25T18:46:00Z

User is able to specify Preserve, Inside Namespaces, or Outside Namespaces. Default is Preserve.

Move Inside Namespace enabled:

Move Outside Namespace enabled:

Move Usings Options:

resolves #27179

/CC: @sharwell

CyrusNajmabadi · 2018-10-25T19:06:58Z

User is able to specify No Preference, Inside Namespaces, or Outside Namespaces. Default is No Preference.

We don't really ever have a 'no preference' option. There's always a preference, there is just a configured severity that determines if the user is pushed to fix things up or not. Should we just have "Inside/Output" along with a severity of 'None'?

CyrusNajmabadi · 2018-10-25T19:07:37Z

src/CodeStyle/Core/CodeFixes/Microsoft.CodeAnalysis.CodeStyle.Fixes.csproj

  <ItemGroup>
    <Compile Include="..\..\..\Features\Core\Portable\CodeFixes\PredefinedCodeFixProviderNames.cs" Link="PredefinedCodeFixProviderNames.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\Shared\Utilities\IProgressTracker.cs" Link="IProgressTracker.cs" />
+    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeFixes\FixAllOccurrences\DocumentBasedFixAllProvider.cs" Link="DocumentBasedFixAllProvider.cs" />


➡️ The original implementation was based on this approach. We already had the type in the code base so it was linked it to support the new code fix.

I'm not actually seeing FixAll tests, so i'm not sure how to evaluate this. It's also unclear to me why this is in src/CodeStyle/Core since the feature doesn't appear to be there.

DocumentBasedFixAllProvider was already in CodeStyle.Fixes. I moved it to Workspaces to make it accessible to the fixAll and other future fixAlls. I could have alternately refernced CodeStyle.Fixes from the CSharp.EditorFeatures project.

I could have alternately refernced CodeStyle.Fixes from the CSharp.EditorFeatures project.

This is not currently possible due to #30472

src/EditorFeatures/CSharp/CSharpEditorResources.resx

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.IndentationSettings.cs

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.SourceMap.cs

CyrusNajmabadi · 2018-10-25T19:10:32Z

Pausing my review. can you give a medium level overview of the implementation of this feature? There was a lot of unexpected complexity, and it would be good to have an understanding of what it is for, why existing facilities could not be used, and how ti all works. Thanks!

CyrusNajmabadi · 2018-10-25T19:16:19Z

src/Workspaces/CSharp/Portable/CodeStyle/UsingPlacementPreference.cs

+        ///<summary>
+        /// Prefer usings placed outside namespaces.
+        ///</summary>
+        OutsideNamespace


better names might be 'CompilationLevel' and 'NamespaceLevel'. You may not have a namespace in your code, so saying 'OutputNamespace' doesn't really make a ton of sense to me.

With only two values, this could potentially also be a bool. though i' mnot opposed to an enum.

I don't see the suggested alternatives as a clear improvement over the current names. The current names align with the documentation described by both StyleCop Classic and StyleCop Analyzers, so it seems consistent with what users would likely expect today.

I'm a weak advocate of the current names and would consider other alternatives if they were presented.

CyrusNajmabadi · 2018-10-25T19:17:22Z

src/Workspaces/CSharp/Portable/CSharpWorkspaceResources.resx

    <value>Wrapping preferences</value>
  </data>
+  <data name="using_preferences" xml:space="preserve">
+    <value>using preferences</value>


no comment that 'using' should be locked?

Should using be in single quotes?

@Neme12 The var in var_preferences isn't in quotes, so I think it is OK.

one niggle there is htat 'var preferences' isn't really an english phrase. whereas 'using preferences' is a bit ambiguous. Is it hte preferences around 'using directives'? Or is it 'using your preferences'? That said, this is minor enough that i think it's fine either way.

looking at this, i would put 'using' in quotes.

src/Features/Core/Portable/Diagnostics/Analyzers/IDEDiagnosticIds.cs

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsDiagnosticAnalyzer.cs

CyrusNajmabadi · 2018-10-25T19:23:52Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsDiagnosticAnalyzer.cs

+            nameof(CSharpEditorResources.Using_directives_must_be_placed_inside_a_namespace_declaration), CSharpEditorResources.ResourceManager, typeof(CSharpEditorResources));
+
+        private static readonly LocalizableResourceString s_localizableOutsideMessage = new LocalizableResourceString(
+            nameof(CSharpEditorResources.Using_directives_must_be_placed_outside_a_namespace_declaration), CSharpEditorResources.ResourceManager, typeof(CSharpEditorResources));


IMO, this would all be cleaner with two separate analyzers. That way you can also just use the normal pattern of passing title/message to the base type and whatnot.

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

CyrusNajmabadi · 2018-10-25T19:24:44Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+            if (CountNamespaces(compilationUnit.Members) > 1)
+            {
+                return;
+            }


why is this done here, instead of during analysis?

The using placement goes against the code style preference, but the code fix doesn't currently do the work of determining which namespaces to move each using into.

enhhhhhhhhh. i would think it would just be the outermost namespace. do we have people with another coding pattern somewhere?

enhhhhhhhhh

It's safe to assume that everything you hit like this was implemented as a direct result of a bug report filed to StyleCop Analyzers. This feature took more than a year to stabilize with some rather horrifying real-world edge cases.

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.UsingsSorter.cs

sharwell · 2018-10-25T19:25:40Z

Pausing my review. can you give a medium level overview of the implementation of this feature? There was a lot of unexpected complexity, and it would be good to have an understanding of what it is for, why existing facilities could not be used, and how ti all works. Thanks!

The implementation is derived from the work by @vweijsters in StyleCop Analyzers. The tests cover nearly all branches of the code fix; it turned out to be surprisingly complex for what seemed like a reasonably straightforward feature going in.

CyrusNajmabadi · 2018-10-25T19:27:39Z

The implementation is derived from the work by @vweijsters in StyleCop Analyzers. The tests cover nearly all branches of the code fix; it turned out to be surprisingly complex for what seemed like a reasonably straightforward feature going in.

That doesn't really help me understand anything better. For example, i want to know about what each part is needed for, because it's very possible the IDE already has logic somewhere to handle things, and we may just end up duplicating stuff unnecessarily.

CyrusNajmabadi · 2018-10-25T19:28:53Z

src/EditorFeatures/CSharpTest/MisplacedUsings/MisplacedUsingsCodeFixProviderTests.cs

+    /// <summary>
+    /// Unit tests for the <see cref="MisplacedUsingsDiagnosticAnalyzer"/> and <see cref="MisplacedUsingsCodeFixProvider"/>.
+    /// </summary>
+    public class MisplacedUsingsCodeFixProviderTests


i'm confused why this isn't just using our normal code fix base class.

Most of the deviations on this front are the result of avoiding unnecessary deviations from the original implementation in the initial submission, which allows @vweijsters to explain the implementation details and merge any bug fixes back to StyleCop Analyzers. There was no "normal code fix base class" in the original.

I'm ok with that as a first draft. But it does'nt make sense to me in terms of merging in. Will we just be producing more and more duplication of code that may already exist just because it's in StyleCop analyzers? I def do not like that. This just means more code to maintain and fixup when we encounter bugs and whatnot.

dotnet/roslyn is one of the remaining outliers that does not use Microsoft.CodeAnalysis.Testing, the new common library for analyzer and code fix testing. The default configuration for that library includes a large number of much stronger assertions regarding "well-behaved" implementations, so the long term goal is migrating all current testing to be built on that library.

CyrusNajmabadi · 2018-10-25T19:30:18Z

src/EditorFeatures/CSharpTest/MisplacedUsings/MisplacedUsingsCodeFixProviderTests.cs

+                Diagnostic(MisplacedUsingsDiagnosticAnalyzer._insideDescriptor).WithLocation(6, 1),
+                Diagnostic(MisplacedUsingsDiagnosticAnalyzer._insideDescriptor).WithLocation(7, 1),
+                Diagnostic(MisplacedUsingsDiagnosticAnalyzer._insideDescriptor).WithLocation(9, 1),
+                Diagnostic(MisplacedUsingsDiagnosticAnalyzer._insideDescriptor).WithLocation(10, 1),


why are we reporting so many misplaced diagnostics? Users should really only see one diagnostic (either for the span of the first item, or perhaps the span of the entire set).

I don't like this test because it's no longer declarative. i have no idea what 10,1 means, or how it relates to the code.

why are we reporting so many misplaced diagnostics? Users should really only see one diagnostic (either for the span of the first item, or perhaps the span of the entire set).

This is consistent with how we report other diagnostics. We have the option of tying the code fix to the helper diagnostic used by IDE0005.

src/EditorFeatures/CSharpTest/MisplacedUsings/MisplacedUsingsCodeFixProviderTests.cs

JoeRobich · 2019-04-12T16:09:51Z

@CyrusNajmabadi & @sharwell This PR is ready for another review.

Notable changes since October:

Replaced the StyleCop fixer (Sort, Group, and Move) to a fixer that just moves using directives
Split the Analyzer into one for the CompilationUnit and one for Namespaces
Updated the AddImportService to respect preferred placement

CyrusNajmabadi · 2019-04-12T17:47:34Z

eng/Versions.props

    <RoslynDiagnosticsNugetPackageVersion>2.6.2-beta2</RoslynDiagnosticsNugetPackageVersion>
    <CodeStyleLayerCodeAnalysisVersion>2.8.2</CodeStyleLayerCodeAnalysisVersion>
-    <MicrosoftCodeAnalysisTestingVersion>1.0.0-beta1-63310-01</MicrosoftCodeAnalysisTestingVersion>
+    <MicrosoftCodeAnalysisTestingVersion>1.0.0-beta1-63812-02</MicrosoftCodeAnalysisTestingVersion>


Newer versions of this library often come with usability enhancements for testing analyzers and code fixes.

CyrusNajmabadi · 2019-04-12T17:50:46Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+            }
+        }
+
+        private class MoveMisplacedUsingsCodeAction : CodeAction


we already have a CodeAction.DocumentChangeAction that we derive from for these cases.

CyrusNajmabadi · 2019-04-12T17:52:04Z

src/EditorFeatures/Core/Implementation/Diagnostics/CodingConventionAnalyzerHelpers.cs

+            // Provide a rooted path when filePath is just the file's name.
+            var fullPath = string.IsNullOrEmpty(Path.GetDirectoryName(filePath))
+                ? Path.Combine(Environment.CurrentDirectory, filePath)
+                : filePath;


seeing Environment in a test is scary to me.

CyrusNajmabadi · 2019-04-12T17:53:33Z

src/Features/Core/Portable/AddImport/References/PackageReference.cs


            public override async Task<AddImportFixData> TryGetFixDataAsync(
-                Document document, SyntaxNode node, bool placeSystemNamespaceFirst, CancellationToken cancellationToken)
+                Document document, SyntaxNode node, bool placeSystemNamespaceFirst, AddImportPlacement placement, CancellationToken cancellationToken)


consider an AddImportOptions struct that can pass along the placement and the PlaceSystemNamespaceFirst value.

CyrusNajmabadi · 2019-04-12T17:56:02Z

src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs

+                new CodeStylePreference(CSharpVSResources.Preserve, isChecked: false),
+                new CodeStylePreference(CSharpVSResources.Inside_namespace, isChecked: false),
+                new CodeStylePreference(CSharpVSResources.Outside_namespace, isChecked: false),
+            };


having a 'preserve' option seems odd to me. we don't generlaly have that. our position up till now has been that we always have a preference, but we can have no-severity, meaning we won't tell you there's an issue, or otherwise try to get you to change it.

Sam brought up that when we update Organize Imports it would use the Preserve option to mean don't move imports. There is also the Add Import code fix, if we didn't have the Preserve option then the current location of using directives would get ignored in favor of the code style setting we defaulted to.

here is also the Add Import code fix, if we didn't have the Preserve option then the current location of using directives would get ignored in favor of the code style setting we defaulted to.

Hrmm... for add-import, i think we should respect the file no matter what the option is. Thoughts?

CyrusNajmabadi · 2019-04-12T17:57:34Z

src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs

+            defaultValue: PreservePlacementWithSilentEnforcement,
+            storageLocations: new OptionStorageLocation[]{
+                new EditorConfigStorageLocation<CodeStyleOption<AddImportPlacement>>(
+                    "csharp_using_directive_placement",


i think this name is good. but i would use 'using directive' more consistently in the rest of the UI. This is especially important as C# also has a 'using statement'

CyrusNajmabadi · 2019-04-12T17:58:09Z

src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs

+        public static readonly CodeStyleOption<AddImportPlacement> PreservePlacementWithSilentEnforcement =
+            new CodeStyleOption<AddImportPlacement>(AddImportPlacement.Preserve, NotificationOption.Silent);
+
+        public static readonly Option<CodeStyleOption<AddImportPlacement>> PreferredUsingDirectivesPlacement = CreateOption(


i don't like that you use Directives here, but Directive below. can you keep pluralization consistent?

CyrusNajmabadi · 2019-04-12T17:59:08Z

src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs


                        return false;
                    }
+


CyrusNajmabadi · 2019-04-12T17:59:59Z

src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs

                IsNullableTypeInPointerExpression(name, reducedName) ||
                name.IsNotNullableReplaceable(reducedName) ||
-                IsQualifiedNameInUsingDirective(semanticModel, name, reducedName))
+                (IsQualifiedNameInUsingDirective(semanticModel, name, reducedName) && !IsGlobalAliasQualifiedName(name)))


can you extract a named method for this? to keep all the cases just simple || checks with clear names?

Moved these changes into #34964

CyrusNajmabadi · 2019-04-12T18:00:24Z

src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs

+            return name is AliasQualifiedNameSyntax aliasName
+                ? aliasName.Alias.Identifier.IsKind(SyntaxKind.GlobalKeyword)
+                : false;
+        }


shoudl be commented.

can just be => name is AliasQual aliasName && ...;

CyrusNajmabadi · 2019-04-12T18:02:00Z

src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/DocumentBasedFixAllProvider.cs

        public override Task<CodeAction> GetFixAsync(FixAllContext fixAllContext)
        {
+            // Disable RS0005 as this is test code and we don't need telemetry for created code action.
+#pragma warning disable RS0005 // Do not use generic CodeAction.Create to create CodeAction


can you explain how this is test code?

I'll remove

CyrusNajmabadi · 2019-04-12T18:02:35Z

src/Workspaces/VisualBasic/Portable/Editing/VisualBasicImportAdder.vb

        End Function

+        Protected Overrides Function GetImportPlacement(options As OptionSet) As AddImportPlacement
+            Return AddImportPlacement.OutsideNamespace


maybe a simple omment that VB doesn't support imports inside namespaces.

CyrusNajmabadi · 2019-04-12T18:04:35Z

i haven't looked through this all, but this PR is large enough that i think it would benefit from a few initial PRs sent in that break things out to make the final PR much easier to read. For example, moving to a simple model of having AddImportOptions would make it easy to then add the Placement option and update only the couple of places that need it.

Similarly, moving the FixAll stuff around looks ike it can be done independently.

This PR is currently at the point that it triggerred my "too big to fit in my head' point, especially with keeping the big picutre, and the individual logic around at the same time.

JoeRobich · 2019-04-12T18:10:33Z

@CyrusNajmabadi Thanks for the review. I will look at extracting a some of these changes as you suggested.

ryzngard

First pass done

ryzngard · 2019-04-12T02:56:19Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+            }
+        }
+
+        private static int CountNamespaces(SyntaxList<MemberDeclarationSyntax> members)


This looks similar to functions for AbstractMoveToNamespaceService. Worth pulling out into an extension method?

ryzngard · 2019-04-12T03:00:31Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+            var newCompilationUnit = await ExpandUsingDirectivesAsync(document, compilationUnit, cancellationToken).ConfigureAwait(false);
+            ImmutableArray<SyntaxTrivia> fileHeader;
+
+            (newCompilationUnit, fileHeader) = RemoveFileHeader(newCompilationUnit);


Can you make new variables for each stage as file header is removed and added back so it's easier to follow?

ryzngard · 2019-04-12T03:04:05Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+        private static async Task<CompilationUnitSyntax> ExpandUsingDirectivesAsync(Document document, CompilationUnitSyntax compilationUnit, CancellationToken cancellationToken)
+        {
+            // Expand the usings declarations in the compilation unit.
+            var expandCompilationUnitUsings = compilationUnit.Usings.Select(


nit: expandCompilationUnitUsingsTasks

ryzngard · 2019-04-12T03:30:13Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+            // Expand the using declarations in the namespace declaration.
+            var namespaceDeclaration = newCompilationUnit.Members.OfType<NamespaceDeclarationSyntax>().First();
+
+            var expandNamespaceDeclarationUsings = namespaceDeclaration.Usings.Select(


nit: expandNamespaceDeclarationUsingsTasks

ryzngard · 2019-04-12T19:27:02Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+            }
+
+            var newTrivia = splitIntoLines(firstMemberTrivia)
+                .SkipWhile(trivia => trivia.All(t => t.IsWhitespaceOrEndOfLine()) && trivia.Last().IsKind(SyntaxKind.EndOfLineTrivia))


Do you need the && trivia.Last().IsKind(SyntaxKind.EndOfLineTrivia) in the SkipWhile? They should already be split into the last character being newline

I don't need to remove leading whitespace trivia if it is on the same line.

ryzngard · 2019-04-12T19:31:00Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+
+                    // Only move using declarations inside the namespace when
+                    // - There are no global attributes
+                    // - There is only a single namespace declared at the top level


Can you also add There are type definitions outside of the single top level namespace, which I presume is what the compilationUnit.Members.Count > 1 check is for

ryzngard · 2019-04-12T19:32:34Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+            return result;
+        }
+
+        private static (CompilationUnitSyntax, ImmutableArray<SyntaxTrivia>) RemoveFileHeader(CompilationUnitSyntax syntaxRoot)


nit: You can name the return tuple members to hint at what they mean. (CompilationUnitSyntax compilationWithoutHeader, ImmutableArray<SyntaxTrivia> header)

ryzngard · 2019-04-12T19:35:05Z

src/EditorFeatures/CSharp/MisplacedUsings/MisplacedUsingsCodeFixProvider.cs

+                        break;
+
+                    case SyntaxKind.EndOfLineTrivia:
+                        hasHeader = true;


Is this right? It looks like if there's a blank line at the beginning of a file it's considered a header.

ryzngard · 2019-04-12T19:40:57Z

src/EditorFeatures/CSharpTest/AddUsing/AddUsingTestsWithAddImportDiagnosticProvider.cs

+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddImport)]
+        public async Task TestAddsImportsInsideNamespaceIfPreferred()
+        {
+            await TestInRegularAndScriptAsync(


nit: It would probably be easier to read if you made a function like

TestInRegularAndScriptAsync(string original, string expected, OptionKey usingPlacementStyleOption, object codeStyleOption)

That way you don't have the new Dictionary<OptionKey, object>() all over the place and people can parse the information quicker

JoeRobich · 2019-04-22T22:24:35Z

Closing and will take these changes through #35009

Added CodeStyle option for 'using' placement

843ecf5

JoeRobich requested review from a team as code owners October 25, 2018 18:46

JoeRobich added the Area-IDE label Oct 25, 2018