Correctly handle simplification of Nullable{T} to T? inside crefs

Fixes #29

src/EditorFeatures/CSharpTest/Diagnostics/SimplifyTypeNames/SimplifyTypeNamesTests.cs

@@ -1817,6 +1817,201 @@ static void Main()
   compareTokens: false, index: 0);
         }
 
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref()
+        {
+            TestMissing(
+@"using System;
+/// <summary>
+/// <see cref=""[|Nullable{T}|]""/>
+/// </summary>
+class A { }");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref2()
+        {
+            TestMissing(
+@"/// <summary>
+/// <see cref=""[|System.Nullable{T}|]""/>
+/// </summary>
+class A { }");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref3()
+        {
+            TestMissing(
+@"/// <summary>
+/// <see cref=""[|System.Nullable{T}|].Value""/>
+/// </summary>
+class A { }");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref4()
+        {
+            TestMissing(
+@"using System;
+/// <summary>
+/// <see cref=""C{[|Nullable{T}|]}""/>
+/// </summary>
+class C<T> {  }");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref5()
+        {
+            TestMissing(
+@"/// <summary>
+/// <see cref=""A.M{[|Nullable{T}|]}()""/>
+/// </summary>
+class A 
+{
+    public void M<U>() where U : struct { }
+}");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref6()
+        {
+            TestMissing(
+@"using System;
+/// <summary>
+/// <see cref=""[|Nullable{int}|]""/>
+/// </summary>
+class A { }");
+        }
+
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref7()
+        {
+            TestMissing(
+@"using System;
+/// <summary>
+/// <see cref=""C{[|Nullable{int}|]}""/>
+/// </summary>
+class C<T> { }");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestMissingNullableSimplificationInsideCref8()
+        {
+            TestMissing(
+@"/// <summary>
+/// <see cref=""A.M{[|Nullable{int}|]}()""/>
+/// </summary>
+class A 
+{
+    public void M<U>() where U : struct { }
+}");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestNullableSimplificationInsideCref()
+        {
+            Test(
+@"/// <summary>
+/// <see cref=""A.M([|System.Nullable{A}|])""/>
+/// </summary>
+struct A
+{ 
+    public void M(A? x) { }
+}",
+@"/// <summary>
+/// <see cref=""A.M(A?)""/>
+/// </summary>
+struct A
+{ 
+    public void M(A? x) { }
+}");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestNullableSimplificationInsideCref2()
+        {
+            Test(
+@"using System;
+using System.Collections.Generic;
+/// <summary>
+/// <see cref=""A.M(List{[|Nullable{int}|]})""/>
+/// </summary>
+class A
+{ 
+    public void M(List<int?> x) { }
+}",
+@"using System;
+using System.Collections.Generic;
+/// <summary>
+/// <see cref=""A.M(List{int?})""/>
+/// </summary>
+class A
+{ 
+    public void M(List<int?> x) { }
+}");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestNullableSimplificationInsideCref3()
+        {
+            Test(
+@"using System;
+using System.Collections.Generic;
+/// <summary>
+/// <see cref=""A.M{U}(List{[|Nullable{U}|]})""/>
+/// </summary>
+class A
+{ 
+    public void M<U>(List<U?> x) where U : struct { }
+}",
+@"using System;
+using System.Collections.Generic;
+/// <summary>
+/// <see cref=""A.M{U}(List{U?})""/>
+/// </summary>
+class A
+{ 
+    public void M<U>(List<U?> x) where U : struct { }
+}");
+        }
+
+        [WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
+        public void TestNullableSimplificationInsideCref4()
+        {
+            Test(
+@"using System;
+using System.Collections.Generic;
+/// <summary>
+/// <see cref=""A.M{T}(List{Nullable{T}}, [|Nullable{T}|])""/>
+/// </summary>
+class A
+{ 
+    public void M<U>(List<U?> x, U? y) where U : struct { }
+}",
+@"using System;
+using System.Collections.Generic;
+/// <summary>
+/// <see cref=""A.M{T}(List{Nullable{T}}, T?})""/>
+/// </summary>
+class A
+{ 
+    public void M<U>(List<U?> x, U? y) where U : struct { }
+}");
+        }
+
         [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)]
         public void TestColorColorCase1()
         {

src/EditorFeatures/VisualBasicTest/Diagnostics/SimplifyTypeNames/SimplifyTypeNamesTests.vb

@@ -3,7 +3,6 @@
 Option Strict Off
 Imports Microsoft.CodeAnalysis.CodeFixes
 Imports Microsoft.CodeAnalysis.Diagnostics
-Imports Microsoft.CodeAnalysis.Diagnostics.SimplifyTypeNames
 Imports Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces
 Imports Microsoft.CodeAnalysis.Options
 Imports Microsoft.CodeAnalysis.Simplification
@@ -579,6 +578,199 @@ End Namespace
         NewLines("Imports System \n Module Program \n Dim x = [|Nullable(Of Guid).op_Implicit|](Nothing) \n End Module"))
         End Sub
 
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestMissingNullableSimplificationInsideCref()
+            TestMissing(
+"Imports System
+''' <summary>
+''' <see cref=""[|Nullable(Of T)|]""/>
+''' </summary>
+Class A
+End Class")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestMissingNullableSimplificationInsideCref2()
+            TestMissing(
+"''' <summary>
+''' <see cref=""[|System.Nullable(Of T)|]""/>
+''' </summary>
+Class A
+End Class")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestMissingNullableSimplificationInsideCref3()
+            TestMissing(
+"''' <summary>
+''' <see cref=""[|System.Nullable(Of T)|].Value""/>
+''' </summary>
+Class A
+End Class")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestMissingNullableSimplificationInsideCref4()
+            TestMissing(
+"Imports System
+''' <summary>
+''' <see cref=""[|Nullable(Of Integer)|].Value""/>
+''' </summary>
+Class A
+End Class")
+        End Sub
+
+        <WorkItem(2196, "https://github.com/dotnet/roslyn/issues/2196")>
+        <WorkItem(2197, "https://github.com/dotnet/roslyn/issues/2197")>
+        <WorkItem(29, "https: //github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestNullableSimplificationInsideCref()
+            ' NOTE: This will probably stop working if issues 2196 / 2197 related to VB compiler and semantic model are fixed.
+            ' It is unclear whether Nullable(Of Integer) is legal in the below case. Currently the VB compiller allows this while
+            ' C# doesn't allow similar case. If this Nullable(Of Integer) becomes illegal in VB in the below case then the simplification
+            ' from Nullable(Of Integer) -> Integer will also stop working and the baseline for this test will have to be updated.
+            Test(
+"Imports System
+''' <summary>
+        ''' <see cref=""C(Of [|Nullable(Of Integer)|])""/>
+        ''' </summary>
+Class C(Of T)
+End Class",
+"Imports System
+''' <summary>
+''' <see cref=""C(Of Integer?)""/>
+''' </summary>
+Class C(Of T)
+End Class")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <WorkItem(2189, "https://github.com/dotnet/roslyn/issues/2189")>
+        <WorkItem(2196, "https://github.com/dotnet/roslyn/issues/2196")>
+        <WorkItem(2197, "https://github.com/dotnet/roslyn/issues/2197")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestNullableSimplificationInsideCref2()
+            ' NOTE: This will probably stop working if issues 2196 / 2197 related to VB compiler and semantic model are fixed.
+            ' It is unclear whether Nullable(Of Integer) is legal in the below case. Currently the VB compiller allows this while
+            ' C# doesn't allow similar case. If this Nullable(Of Integer) becomes illegal in VB in the below case then the simplification
+            ' from Nullable(Of Integer) -> Integer will also stop working and the baseline for this test will have to be updated.
+            Test(
+"Imports System
+''' <summary>
+''' <see cref=""C.M(Of [|Nullable(Of Integer)|])""/>
+''' </summary>
+Class C
+    Sub M(Of T As Structure)()
+    End Sub
+End Class",
+"Imports System
+''' <summary>
+''' <see cref=""C.M(Of Integer?)""/>
+''' </summary>
+Class C
+    Sub M(Of T As Structure)()
+    End Sub
+End Class")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestNullableSimplificationInsideCref3()
+            Test(
+"Imports System
+''' <summary>
+''' <see cref=""A.M([|Nullable(Of A)|])""/>
+''' </summary>
+Structure A
+    Sub M(x As A?)
+    End Sub
+End Structure",
+"Imports System
+''' <summary>
+''' <see cref=""A.M(A?)""/>
+''' </summary>
+Structure A
+    Sub M(x As A?)
+    End Sub
+End Structure")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestNullableSimplificationInsideCref4()
+            Test(
+"Imports System
+Imports System.Collections.Generic
+''' <summary>
+''' <see cref=""A.M(List(Of [|Nullable(Of Integer)|]))""/>
+''' </summary>
+Structure A
+    Sub M(x As List(Of Integer?))
+    End Sub
+End Structure",
+"Imports System
+Imports System.Collections.Generic
+''' <summary>
+''' <see cref=""A.M(List(Of Integer?))""/>
+''' </summary>
+Structure A
+    Sub M(x As List(Of Integer?))
+    End Sub
+End Structure")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestNullableSimplificationInsideCref5()
+            Test(
+"Imports System
+Imports System.Collections.Generic
+''' <summary>
+''' <see cref=""A.M(Of T)(List(Of [|Nullable(Of T)|]))""/>
+''' </summary>
+Structure A
+    Sub M(Of U As Structure)(x As List(Of U?))
+    End Sub
+End Structure",
+"Imports System
+Imports System.Collections.Generic
+''' <summary>
+''' <see cref=""A.M(Of T)(List(Of T?))""/>
+''' </summary>
+Structure A
+    Sub M(Of U As Structure)(x As List(Of U?))
+    End Sub
+End Structure")
+        End Sub
+
+        <WorkItem(29, "https://github.com/dotnet/roslyn/issues/29")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
+        Public Sub TestNullableSimplificationInsideCref6()
+            Test(
+"Imports System
+Imports System.Collections.Generic
+''' <summary>
+''' <see cref=""A.M(Of U)(List(Of Nullable(Of Integer)), [|Nullable(Of U)|])""/>
+''' </summary>
+Structure A
+    Sub M(Of U As Structure)(x As List(Of Integer?), y As U?)
+    End Sub
+End Structure",
+"Imports System
+Imports System.Collections.Generic
+''' <summary>
+''' <see cref=""A.M(Of U)(List(Of Nullable(Of Integer)), U?)""/>
+''' </summary>
+Structure A
+    Sub M(Of U As Structure)(x As List(Of Integer?), y As U?)
+    End Sub
+End Structure")
+        End Sub
+
         <WorkItem(529930)>
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsSimplifyTypeNames)>
         Public Sub TestReservedNameInAttribute1()

src/Features/CSharp/Diagnostics/Analyzers/CSharpSimplifyTypeNamesDiagnosticAnalyzer.cs

@@ -30,9 +30,14 @@ public override void Initialize(AnalysisContext analysisContext)
 
         protected override void AnalyzeNode(SyntaxNodeAnalysisContext context)
         {
-            if (context.Node.Ancestors(ascendOutOfTrivia: false).Any(n => s_kindsOfInterest.Contains(n.Kind())))
+            if (context.Node.Ancestors(ascendOutOfTrivia: false).Any(n => !n.IsKind(SyntaxKind.QualifiedCref) && s_kindsOfInterest.Contains(n.Kind())))
             {
-                // Already simplified an ancestor of this node.
+                // Bail out early because we have already simplified an ancestor of this node (except in the QualifiedCref case).
+                // We need to keep going in case this node is under a QualifiedCref because it is possible to have multiple simplifications within the same QualifiedCref.
+                // For example, consider <see cref="A.M(Nullable{int})"/>. The 'A.M(Nullable{int})' here is represented by a single QualifiedCref node in the syntax tree.
+                // It is possible to have a simplification to remove the 'A.' qualification for the QualifiedCref itself as well as another simplfication to change 'Nullable{int}'
+                // to 'int?' in the GenericName for the 'Nullable{T}' that is nested inside this QualifiedCref. We need to keep going so that the latter simplification can be
+                // made available.
                 return;
             }
 

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

@@ -1363,9 +1363,7 @@ private static int GetNamespaceId(SyntaxList<MemberDeclarationSyntax> members, N
                     if (!name.IsVar && (symbol.Kind == SymbolKind.NamedType) && !name.IsLeftSideOfQualifiedName())
                     {
                         var type = (INamedTypeSymbol)symbol;
-                        if (!type.IsUnboundGenericType && // Don't rewrite unbound generic type "Nullable<>"
-                            type.IsNullable() &&
-                            aliasInfo == null)
+                        if (aliasInfo == null && CanSimplifyNullable(type, name))
                         {
                             GenericNameSyntax genericName;
                             if (name.Kind() == SyntaxKind.QualifiedName)
@@ -1440,6 +1438,54 @@ private static int GetNamespaceId(SyntaxList<MemberDeclarationSyntax> members, N
             return name.CanReplaceWithReducedName(replacementNode, semanticModel, cancellationToken);
         }
 
+        private static bool CanSimplifyNullable(INamedTypeSymbol type, NameSyntax name)
+        {
+            if (!type.IsNullable())
+            {
+                return false;
+            }
+
+            if (type.IsUnboundGenericType)
+            {
+                // Don't simplify unbound generic type "Nullable<>".
+                return false;
+            }
+
+            if (!InsideCrefReference(name))
+            {
+                // Nullable<T> can always be simplified to T? outside crefs.
+                return true;
+            }
+
+            // Inside crefs, if the T in this Nullable{T} is being declared right here
+            // then this Nullable{T} is not a constructed generic type and we should
+            // not offer to simplify this to T?.
+            //
+            // For example, we should not offer the simplification in the following cases where
+            // T does not bind to an existing type / type parameter in the user's code.
+            // - <see cref="Nullable{T}"/>
+            // - <see cref="System.Nullable{T}.Value"/>
+            //
+            // And we should offer the simplification in the following cases where SomeType and
+            // SomeMethod bind to a type and method declared elsewhere in the users code.
+            // - <see cref="SomeType.SomeMethod(Nullable{SomeType})"/>
+
+            var argument = type.TypeArguments.SingleOrDefault();
+            if (argument == null || argument.IsErrorType())
+            {
+                return false;
+            }
+
+            var argumentDecl = argument.DeclaringSyntaxReferences.FirstOrDefault();
+            if (argumentDecl == null)
+            {
+                // The type argument is a type from metadata - so this is a constructed generic nullable type that can be simplified (e.g. Nullable(Of Integer)).
+                return true;
+            }
+
+            return !name.Span.Contains(argumentDecl.Span);
+        }
+
         private static bool CanReplaceWithPredefinedTypeKeywordInContext(NameSyntax name, SemanticModel semanticModel, out TypeSyntax replacementNode, ref TextSpan issueSpan, SyntaxKind keywordKind, CancellationToken cancellationToken)
         {
             replacementNode = CreatePredefinedTypeSyntax(name, keywordKind);

src/Workspaces/VisualBasic/Portable/Extensions/ExpressionSyntaxExtensions.vb

@@ -1312,10 +1312,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
                     ' Don't rewrite in the case where Nullable(Of Integer) is part of some qualified name like Nullable(Of Integer).Something
                     If (symbol.Kind = SymbolKind.NamedType) AndAlso (Not name.IsLeftSideOfQualifiedName) Then
                         Dim type = DirectCast(symbol, INamedTypeSymbol)
-                        If (Not type.IsUnboundGenericType) AndAlso 'Don't rewrite unbound generic type "Nullable(Of )"
-                           (type.OriginalDefinition IsNot Nothing) AndAlso
-                           (type.OriginalDefinition.SpecialType = SpecialType.System_Nullable_T) AndAlso
-                           (aliasInfo Is Nothing) Then
+                        If aliasInfo Is Nothing AndAlso CanSimplifyNullable(type, name) Then
                             Dim genericName As GenericNameSyntax
                             If name.Kind = SyntaxKind.QualifiedName Then
                                 genericName = DirectCast(DirectCast(name, QualifiedNameSyntax).Right, GenericNameSyntax)
@@ -1376,6 +1373,58 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
             Return name.CanReplaceWithReducedName(replacementNode, semanticModel, cancellationToken)
         End Function
 
+        Private Function CanSimplifyNullable(type As INamedTypeSymbol, name As NameSyntax) As Boolean
+            If Not type.IsNullable Then
+                Return False
+            End If
+
+            If type.IsUnboundGenericType Then
+                ' Don't simplify unbound generic type "Nullable(Of )".
+                Return False
+            End If
+
+            If Not InsideCrefReference(name) Then
+                ' Nullable(Of T) can always be simplified to T? outside crefs.
+                Return True
+            End If
+
+            ' Inside crefs, if the T in this Nullable(Of T) is being declared right here
+            ' then this Nullable(Of T) is not a constructed generic type and we should
+            ' not offer to simplify this to T?.
+            '
+            ' For example, we should not offer the simplification in the following cases where
+            ' T does not bind to an existing type / type parameter in the user's code.
+            ' - <see cref="Nullable(Of T)"/>
+            ' - <see cref="System.Nullable(Of T).Value"/>
+            '
+            ' And we should offer the simplification in the following cases where SomeType and
+            ' SomeMethod bind to a type and method declared elsewhere in the users code.
+            ' - <see cref="SomeType.SomeMethod(Nullable(Of SomeType))"/>
+
+            If name.IsKind(SyntaxKind.GenericName) Then
+                If (name.IsParentKind(SyntaxKind.CrefReference)) OrElse ' cref="Nullable(Of T)"
+                   (name.IsParentKind(SyntaxKind.QualifiedName) AndAlso name.Parent?.IsParentKind(SyntaxKind.CrefReference)) OrElse ' cref="System.Nullable(Of T)"
+                   (name.IsParentKind(SyntaxKind.QualifiedName) AndAlso name.Parent?.IsParentKind(SyntaxKind.QualifiedName) AndAlso name.Parent?.Parent?.IsParentKind(SyntaxKind.CrefReference)) Then  ' cref="System.Nullable(Of T).Value"
+                    ' Unfortunately, unlike in corresponding C# case, we need syntax based checking to detect these cases because of bugs in the VB SemanticModel.
+                    ' See https://github.com/dotnet/roslyn/issues/2196, https://github.com/dotnet/roslyn/issues/2197
+                    Return False
+                End If
+            End If
+
+            Dim argument = type.TypeArguments.SingleOrDefault()
+            If argument Is Nothing OrElse argument.IsErrorType() Then
+                Return False
+            End If
+
+            Dim argumentDecl = argument.DeclaringSyntaxReferences.FirstOrDefault()
+            If argumentDecl Is Nothing Then
+                ' The type argument is a type from metadata - so this is a constructed generic nullable type that can be simplified (e.g. Nullable(Of Integer)).
+                Return True
+            End If
+
+            Return Not name.Span.Contains(argumentDecl.Span)
+        End Function
+
         Private Function TryReduceAttributeSuffix(
             name As NameSyntax,
             identifierToken As SyntaxToken,