Ambiguous type codefix provider (using alias) #24022

MaStr11 · 2018-01-03T20:01:03Z

Customer scenario

Codefix for CS0104 'reference' is an ambiguous reference between 'identifier' and 'identifier' (BC30561 in VB).

Before:

using N1;
using N2;

namespace N1
{
    public class AmbiguousClass { }
}
namespace N2
{
    public class AmbiguousClass { }
}
namespace NTest
{
    public class Test: AmbiguousClass //CS0104
    {
    }
}

After

using N1;
using N2;
using AmbiguousClass = N1.AmbiguousClass; // Fix

namespace N1
{
    public class AmbiguousClass { }
}
namespace N2
{
    public class AmbiguousClass { }
}
namespace NTest
{
    public class Test: AmbiguousClass
    {
    }
}

Bugs this fixes

Feature request #23373.

Workarounds, if any

Fully qualify type.

Risk

Golden bar from failing code fix provider.

Performance impact

Low. Code fix is only triggered by one diagnostic (CS0104 or BC30561)

Is this a regression from a previous update?

New feature.

Root cause analysis

User request to get closer to parity with Resharper.

How was the bug found?

Customer reported #23373.

Test documentation updated?

No.

Remarks

Update 8/1/18: This is resolved:
I tried to use class GroupingCodeAction : CodeAction.CodeActionWithNestedActions to group the re-factorings like here but then all tests were failing and in VS it didn't made a difference whether I use CodeActionWithNestedActions or register a code fix per ambiguous type. The FeaturesResources.Alias_ambiguous_type_0 text template was created to work with CodeActionWithNestedActions and therefore the text template is inadequate in the context it is used now.

This reverts commit 3e6b04e.

CyrusNajmabadi · 2018-01-04T16:38:06Z

src/EditorFeatures/CSharpTest/Ambiguity/AmbiguousTypeTests.cs

+    }
+}";
+            await TestInRegularAndScriptAsync(initialMarkup, expectedMarkupTemplate.Replace("#", "using Ambiguous = N1.Ambiguous;"), 0);
+            await TestInRegularAndScriptAsync(initialMarkup, expectedMarkupTemplate.Replace("#", "using Ambiguous = N2.Ambiguous;"), 1);


FWIW, i'm not a fan of this sort of approach. With code actions, we explicitly spell out the before/after code without the user having to read the test code to figure out how the strings are manipulated.

This has the benefit of making it very easy to see and grok what will happen without any intermediary steps to understand.

nit: named param for 0 and 1.

CyrusNajmabadi · 2018-01-04T16:40:08Z

src/Features/CSharp/Portable/Ambiguity/CSharpAmbiguousTypeCodeFixProvider.cs

+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Shared.Extensions;
+
+namespace Microsoft.CodeAnalysis.CSharp.Ambiguity


nit: namespaces here should match the feature name. So this would be AliasAmbiguousType

The idea was that all ambiguity code fixer would go into this namespace. Other candidates would be CS0121 for extension methods (could offer full qualification with transformation in a call to a static method) or CS0433 for same class in same namespace in different assemblies and maybe others.

That's generally not how we organize the codebase though :-/

CyrusNajmabadi · 2018-01-04T16:40:37Z

src/Features/CSharp/Portable/Ambiguity/CSharpAmbiguousTypeCodeFixProvider.cs

+{
+    [ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.AliasAmbiguousType), Shared]
+    [ExtensionOrder(After = PredefinedCodeFixProviderNames.FullyQualify)]
+    internal class CSharpAmbiguousTypeCodeFixProvider : AbstractAmbiguousTypeCodeFixProvider


type name should match provider name. if provider name is AliasAmbiguousType, type names shoudl be CSharpAliasAmbiguousTypeCodeFixProvider and so on

CyrusNajmabadi · 2018-01-04T16:41:40Z

src/Features/Core/Portable/Ambiguity/AbstractAmbiguousTypeCodeFixProvider.cs

+using Microsoft.CodeAnalysis.Shared.Extensions;
+using static Microsoft.CodeAnalysis.CodeActions.CodeAction;
+
+namespace Microsoft.CodeAnalysis.AmbiguityCodeFixProvider


namespace doesn't match folder name. Folder name should be named something like AliasAmbiguousType

CyrusNajmabadi · 2018-01-04T16:42:55Z

src/Features/Core/Portable/Ambiguity/AbstractAmbiguousTypeCodeFixProvider.cs

+            var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+
+            // Innermost: We are looking for an IdentifierName. IdentifierName is sometimes at the same span as its parent (e.g. SimpleBaseTypeSyntax).
+            var diagnosticNode = root.FindNode(context.Span, getInnermostNodeForTie: true);


"getInnermostNodeForTie: true" is correct. But can you verify that this is working by writing a test case where your amibiguous type name is an argument to some method? That way we can demonstrate that FindNode is finding the name and not he ArgumentSyntax node.

I have some tests where getInnermostNodeForTie is relevant including this one for method arguments.

CyrusNajmabadi · 2018-01-04T16:44:52Z

src/Features/Core/Portable/Ambiguity/AbstractAmbiguousTypeCodeFixProvider.cs

+                                                                                  document.Project.Solution.Workspace,
+                                                                                  optionSet,
+                                                                                  cancellationToken).ConfigureAwait(false);
+                    var newRoot = addImportService.AddImport(compilation, root, diagnosticNode, aliasDirective, placeSystemNamespaceFirst);


nit, move this code into the lambda below to make RegisterCodeFixAsync do as little work as possible.

CyrusNajmabadi · 2018-01-04T16:45:39Z

src/Features/Core/Portable/Ambiguity/AbstractAmbiguousTypeCodeFixProvider.cs

+        private static string GetAliasFromDiagnosticNode(ISyntaxFactsService syntaxFacts, SyntaxNode diagnosticNode)
+        {
+            // The content of the node is a good candidate for the alias
+            // For attributes VB requires that the alias ends with 'Attribute' while C# is fine with or without the suffix.


if only VB needs this, then why is this code running for both C# and VB?

I changed the logic to rely on symbol.Name instead of the text of the IdentifierNameSyntaxNode. This way it is easier and also covers the attribute requirements of VB.

CyrusNajmabadi · 2018-01-04T16:46:14Z

src/Features/Core/Portable/Ambiguity/AbstractAmbiguousTypeCodeFixProvider.cs

+            {
+                if (!nodeText.EndsWith("Attribute"))
+                {
+                    nodeText += "Attribute";


while it's likely minor, this code is not going to be correct in a lot of edge cases for attributes.

CyrusNajmabadi · 2018-01-04T16:48:45Z

src/Features/Core/Portable/Ambiguity/AbstractAmbiguousTypeCodeFixProvider.cs

+
+        private static async Task<string> GetTextPreviewOfChangeAsync(SyntaxNode newNode, Workspace workspace, OptionSet optionSet, CancellationToken cancellationToken)
+        {
+            var formattedNode = await Formatter.FormatAsync(newNode, workspace, optionSet, cancellationToken).ConfigureAwait(false);


easier and cheaper to just call NormalizeWhitespace on newNode.

CyrusNajmabadi · 2018-01-04T16:49:12Z

src/Features/VisualBasic/Portable/Ambiguity/VisualBasicAmbiguousTypeCodeFixProvider.vb

+        'BC30561: '<name1>' is ambiguous, imported from the namespaces or types '<name2>'
+        Private Const BC30561 As String = NameOf(BC30561)
+
+        Public Overrides ReadOnly Property FixableDiagnosticIds As ImmutableArray(Of String)


can just say = ImmutableArray.Create...

CyrusNajmabadi · 2018-01-04T16:51:08Z

Very nice!

I tried to use class GroupingCodeAction : CodeAction.CodeActionWithNestedActions to group the re-factorings like here but then all tests were failing and in VS it didn't made a difference whether I use CodeActionWithNestedActions or register a code fix per ambiguous type.

I'd like to understand this a bit more. The way things work in VS is that VS will respect if an action is grouped if there are a lot of code actions to show. If there aren't that many, it will just flatten the children into the main list. This way if there are only a few actions, you can get to them directly. But if there are a lot, you don't have an OMG humongous list.

Grouping would be appropriate, and we should try to have that here.

CyrusNajmabadi · 2018-01-04T16:51:54Z

Question: will this only add aliases for types? or it is possible this diagnostic will trigger for other ambiguous situations (like namespace, or extension methods, etc?)?

CyrusNajmabadi · 2018-01-04T16:52:50Z

src/Features/Core/Portable/Ambiguity/AbstractAmbiguousTypeCodeFixProvider.cs

+               // Aliasing as a closed constructed type is possible but would require to remove the type arguments from the diagnosed node.
+               // It is unlikely that the user wants that and so generic types are not supported.
+               symbolInfo.CandidateSymbols.All(symbol => symbol.IsKind(SymbolKind.NamedType) &&
+                                                         symbol.GetArity() == 0);


ah, you check that it's a named type. good.

probably good to check that its containing symbol is a namespace as well.

probably good to check that its containing symbol is a namespace as well.

Nested classes can also be ambigues. The corresponding test is here. The test case is copied from NameCollisionTests.cs

Gotcha. Are there any issues if there are generics in the containing type (or an intermediary generic type)? might be good to have tests.

I think the ambiguity for nested types is only possible with generic classes in combination with using static. So containing generic types are already tested. The test is called TestAmbiguousNestedClass in line 208 in AmbiguousTypeTests.cs.

CyrusNajmabadi · 2018-01-04T16:53:30Z

src/Features/CSharp/Portable/Ambiguity/CSharpAmbiguousTypeCodeFixProvider.cs

+
+        protected override SyntaxNode GetAliasDirective(string typeName, ISymbol symbol)
+            => SyntaxFactory.UsingDirective(SyntaxFactory.NameEquals(typeName),
+                                            SyntaxFactory.IdentifierName(symbol.ToNameDisplayString()));


better would be to use the SyntaxGenerator to create a TypeExpression instead of using IdentifierName.

Note: SyntaxGenerator already has NamespaceImportDeclaration. Probably just want to add AliasImportDeclaration to that location. Then, you won't even need any VB/C# specific code here. The base type will just be able to call that new method on SyntaxGenerator.

MaStr11 · 2018-01-08T05:22:09Z

Sure. It looks like that approach would work. However, IMO, it's just as simple to just build the string yourself.

I gave this a try in b752f84 and did a simple $"using { alias } = { typeSymbol.ToNameDisplayString() };";. Doing so allows to also delay the call to syntaxGenerator.AliasImportDeclaration to the CodeAction phase.

MaStr11 · 2018-01-08T08:29:36Z

@CyrusNajmabadi I'm currently done with the changes on this PR.
Regarding some open points raised in the conversation:

In my VS the grouping didn't worked even for more than 10 entries.

That's resolved. The grouping is only active if there are more than three different kinds of suggestions present. So it is not related to the total number of entries.

Do you know of a way how to get an extern alias applied.

There is currently no way (afaik). You'd have to go update the SymbolDisplay code.

and

There are many open general problems with just adding a "using X = ....X" approach.

and

My recommendation would be to not go down this path and leave it as an open known issue

I will leave the extern alias bug as is. How should I document the issue?

It would be useful to test, whether the fixed code is without compiler errors. If not would you agree that it is useful to have such an option and could I implement the feature?

I looked into it and it seemed more complicated than I thought first. And because you don't see any value here it doesn't seems worth the effort.

MaStr11 · 2018-01-15T19:30:32Z

@CyrusNajmabadi could I get a second review please.

CyrusNajmabadi · 2018-01-15T19:49:14Z

src/Features/Core/Portable/AliasAmbiguousType/AbstractAliasAmbiguousTypeCodeFixProvider.cs

+{
+    internal abstract class AbstractAliasAmbiguousTypeCodeFixProvider : CodeFixProvider
+    {
+


extra new line.

CyrusNajmabadi · 2018-01-15T19:51:07Z

src/Features/Core/Portable/AliasAmbiguousType/AbstractAliasAmbiguousTypeCodeFixProvider.cs

+                        var newRoot = addImportService.AddImport(compilation, root, diagnosticNode, aliasDirective, placeSystemNamespaceFirst);
+                        return Task.FromResult(document.WithSyntaxRoot(newRoot));
+                    };
+                    codeActionsBuilder.Add(new MyCodeAction(codeActionPreviewText, CreateChangedDocument));


nit: our pattern is to generally use a lambda here. but it's not a big deal. if you prefer the above form, then keep it.

CyrusNajmabadi · 2018-01-15T19:52:13Z

src/Features/Core/Portable/AliasAmbiguousType/AbstractAliasAmbiguousTypeCodeFixProvider.cs

+            }
+        }
+
+        private class GroupingCodeAction : CodeActionWithNestedActions


no need for this type. CodeActionWithNestedActions can just be instanted directly. (however, keep MyCodeAction, that specific subclass is used for telemetry purposes).

CyrusNajmabadi · 2018-01-15T19:52:28Z

...ures/VisualBasic/Portable/AliasAmbiguousType/VisualBasicAliasAmbiguousTypeCodeFixProvider.vb

+Imports Microsoft.CodeAnalysis.CodeFixes
+
+Namespace Microsoft.CodeAnalysis.VisualBasic.AliasAmbiguousType
+


extra newline.

CyrusNajmabadi · 2018-01-15T19:52:35Z

...ures/VisualBasic/Portable/AliasAmbiguousType/VisualBasicAliasAmbiguousTypeCodeFixProvider.vb

+        Protected Overrides Function GetTextPreviewOfChange(aliasName As String, typeSymbol As ITypeSymbol) As String
+            Return $"Imports { aliasName } = { typeSymbol.ToNameDisplayString() }"
+        End Function
+


extra newline.

CyrusNajmabadi

Needs @dotnet/roslyn-ide review as well.

sharwell

My only issue with this is the new SyntaxGenerator relies on a symbol as input instead of a syntax node. This can be a barrier to future performance, so I'd like to see it updated to operate primarily on syntax nodes. The symbol argument could be provided via an overload for cases where it's already available.

sharwell · 2018-02-14T16:39:04Z

src/Workspaces/Core/Portable/Editing/SyntaxGenerator.cs

+        /// </summary>
+        /// <param name="aliasIdentifierName">The name of the alias.</param>
+        /// <param name="symbol">The namespace or type to be aliased.</param>
+        public abstract SyntaxNode AliasImportDeclaration(string aliasIdentifierName, INamespaceOrTypeSymbol symbol);


❗️ This should have the following signature:

public abstract SyntaxNode AliasImportDeclaration(string alias, SyntaxNode name);

If you want to keep a signature that operates on INamespaceOrTypeSymbol, you can use a second overload:

public SyntaxNode AliasImportDeclaration(string alias, INamespaceOrTypeSymbol symbol) => AliasImportDeclaration(alias, { see below });

The missing section of the helper method should call a method similar to TypeExpression(ITypeSyntax), except it should forward the call to GenerateNameSyntax instead of GenerateTypeSyntax.

…ntaxNode for the name to create an alias for.

MaStr11 · 2018-03-02T17:46:36Z

I think I did a mistake while I was solving the merge conflict. The file src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt should only contain these lines:

Microsoft.CodeAnalysis.Editing.SyntaxGenerator.AliasImportDeclaration(string aliasIdentifierName, Microsoft.CodeAnalysis.INamespaceOrTypeSymbol symbol) -> Microsoft.CodeAnalysis.SyntaxNode
abstract Microsoft.CodeAnalysis.Editing.SyntaxGenerator.AliasImportDeclaration(string aliasIdentifierName, Microsoft.CodeAnalysis.SyntaxNode name) -> Microsoft.CodeAnalysis.SyntaxNode
abstract Microsoft.CodeAnalysis.Editing.SyntaxGenerator.NameExpression(Microsoft.CodeAnalysis.INamespaceOrTypeSymbol namespaceOrTypeSymbol) -> Microsoft.CodeAnalysis.SyntaxNode

Can anyone confirm this?

sharwell · 2018-03-08T17:16:42Z

@MaStr11 The previous merge was incorrect, but I amended the commit to be what you intended and force-pushed to the branch so it should appear correct now.

…xProvider

MaStr11 added 14 commits December 29, 2017 23:16

Initial commit.

6ba1ac5

Use IAddImportsService.

0f8fd4d

Failed Use IAddImportFeatureService.

3e6b04e

Revert "Failed Use IAddImportFeatureService."

f1833f8

This reverts commit 3e6b04e.

Added support for VB BC30561

a2f8c45

More VB tests and some alignments between C#and VB.

5a3c419

Small changes.

4d569f3

Better Title for CodeAction.

2491170

Add FeatureRessources text template.

19001be

Reverted unwanted launchSettings.json commit.

63d1741

Delayed some initializations.

b7d2de0

Some re-factorings and more C# tests.

b9a1c96

Spelling corrections and improved naming clarity.

f5b3377

Removed useless comment.

737143e

MaStr11 requested a review from a team as a code owner January 3, 2018 20:01

Disable FixAll and RS1016.

c79b26c

CyrusNajmabadi reviewed Jan 4, 2018

View reviewed changes

Simplified generation of CodeAction title.

b752f84

MaStr11 mentioned this pull request Jan 8, 2018

[Proposal]: Support generics and generic type parameters in aliases dotnet/csharplang#1239

Open

Added test for code fix title.

b631e91

This was referenced Jan 8, 2018

Diagnostic CodeFix for CS0121: ambiguous extension methods. #24096

Open

Diagnostic CodeFix for CS0433: The type TypeName1 exists in both Assembly1 and Assembly2 #24099

Open

CyrusNajmabadi reviewed Jan 15, 2018

View reviewed changes

CyrusNajmabadi approved these changes Jan 15, 2018

View reviewed changes

jinujoseph assigned sharwell Jan 16, 2018

Formatting and small re-factorings (PR feedback).

5577f1a

sharwell requested changes Feb 14, 2018

View reviewed changes

sharwell added this to Community Priority in IDE: sharwell Feb 14, 2018

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 15, 2018

Added SyntaxGenerator.AliasImportDeclaration overload that takes a Sy…

a470e46

…ntaxNode for the name to create an alias for.

sharwell moved this from Community Priority to Needs Response from Me in IDE: sharwell Mar 1, 2018

sharwell approved these changes Mar 1, 2018

View reviewed changes

sharwell moved this from Needs Response from Me to Waiting to Merge in IDE: sharwell Mar 1, 2018

sharwell force-pushed the AmbiguousTypeCodefixProvider branch from d4f7d8a to 9287ce4 Compare March 8, 2018 17:15

Merge remote-tracking branch 'dotnet/master' into AmbiguousTypeCodefi…

0d7c640

…xProvider

sharwell force-pushed the AmbiguousTypeCodefixProvider branch from 9287ce4 to 0d7c640 Compare March 8, 2018 18:48

sharwell merged commit 0dd9b97 into dotnet:master Mar 13, 2018

sharwell removed this from Waiting to Merge in IDE: sharwell Mar 13, 2018

		Imports Microsoft.CodeAnalysis.CodeFixes

		Namespace Microsoft.CodeAnalysis.VisualBasic.AliasAmbiguousType

Ambiguous type codefix provider (using alias) #24022

Ambiguous type codefix provider (using alias) #24022

Conversation

MaStr11 commented Jan 3, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Remarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 4, 2018

CyrusNajmabadi commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaStr11 Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaStr11 commented Jan 8, 2018

MaStr11 commented Jan 8, 2018 • edited Loading

MaStr11 commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaStr11 commented Mar 2, 2018

sharwell commented Mar 8, 2018

MaStr11 commented Jan 3, 2018 •

edited

Loading

MaStr11 Jan 4, 2018 •

edited

Loading

MaStr11 commented Jan 8, 2018 •

edited

Loading