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

Jump to bottom

Fix generate method type parameter nullability #39079

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix generate method type parameter nullability #39079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa7faa1
Ensure we generate ? on a type parameter when appropriate
jasonmalinowski Oct 4, 2019
21584df
Generate class? if the constraint has a type annotation
jasonmalinowski Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 66 additions & 0 deletions src/EditorFeatures/CSharpTest/Diagnostics/GenerateMethod/GenerateMethodTests.cs
View file
Open in desktop
Expand Up @@ -435,6 +435,71 @@ private void Goo(List<string?> l)
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)]
public async Task TestInvocationWithClassConstrainedTypeArgumentAndNullableTypes()
{
await TestInRegularAndScriptAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure to test nullable constraint with not-null flow analysis?

class C 
{
    public T M<T>() where T : class?, new
    {
        var t = new T();
        return [|M2|](t);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that the TestInvocationWithNullableClassConstrainedTypeArgumentAndNullableTypes test below?

Copy link
Contributor

Choose a reason for hiding this comment

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

oof, mondays...

@"#nullable enable

class C
{
public T? M<T>() where T : class
{
T? t = null;
return [|M2|](t);
}
}",
@"#nullable enable

using System;

class C
{
public T? M<T>() where T : class
{
T? t = null;
return M2(t);
}

private T? M2<T>(T? t) where T : class
{
throw new NotImplementedException();
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)]
public async Task TestInvocationWithNullableClassConstrainedTypeArgumentAndNullableTypes()
{
await TestInRegularAndScriptAsync(
@"#nullable enable

class C
{
public T M<T>() where T : class?
{
T t = M<T>();
return [|M2|](t);
}
}",
@"#nullable enable

using System;

class C
{
public T M<T>() where T : class?
{
T t = M<T>();
return M2(t);
}

private T M2<T>(T t) where T : class?
{
throw new NotImplementedException();
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)]
public async Task TestSimpleInvocationNamedValueArg()
Expand Down Expand Up @@ -4621,6 +4686,7 @@ internal static void Test(global::Outer.Inner inner)

[Theory]
[InlineData("class")]
[InlineData("class?")]
[InlineData("struct")]
[InlineData("new()")]
[InlineData("unmanaged")]
Expand Down
6 changes: 5 additions & 1 deletion src/Workspaces/CSharp/Portable/Extensions/ITypeParameterSymbolExtensions.cs
View file
Open in desktop
Expand Up @@ -42,7 +42,11 @@ internal static class ITypeParameterSymbolExtensions

if (typeParameter.HasReferenceTypeConstraint)
{
constraints.Add(SyntaxFactory.ClassOrStructConstraint(SyntaxKind.ClassConstraint));
var questionMarkToken = typeParameter.ReferenceTypeConstraintNullableAnnotation == NullableAnnotation.Annotated
? SyntaxFactory.Token(SyntaxKind.QuestionToken)
: default;

constraints.Add(SyntaxFactory.ClassOrStructConstraint(SyntaxKind.ClassConstraint, SyntaxFactory.Token(SyntaxKind.ClassKeyword), questionMarkToken));
}
else if (typeParameter.HasUnmanagedTypeConstraint)
{
Expand Down
13 changes: 12 additions & 1 deletion ...Workspaces/CSharp/Portable/Extensions/ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs
View file
Open in desktop
Expand Up @@ -306,7 +306,18 @@ public override TypeSyntax VisitPointerType(IPointerTypeSymbol symbol)

public override TypeSyntax VisitTypeParameter(ITypeParameterSymbol symbol)
{
return AddInformationTo(symbol.Name.ToIdentifierName(), symbol);
TypeSyntax typeSyntax = AddInformationTo(symbol.Name.ToIdentifierName(), symbol);

// Add a ? if the type parameter is annotated as nullable, and the constraints allow it. It's possible to end up with
// a T? where T is constrained to class? if we combined a type parameter with some flow state -- we'll ignore the ? here
// at this stage. We should consider moving that check earlier when we actually wrap the symbol.
Copy link
Member Author

Choose a reason for hiding this comment

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

I recognize this check isn't right -- I'm just getting things off the ground to see what else might be breaking here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate when this would be incorrect? Is it the case where flow state shows that we don't need to mark as nullable only because null will never be passed in?

Copy link
Member Author

Choose a reason for hiding this comment

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

The check for "HasReferenceTypeConstraint" is probably insufficient, because you can be constrainted to reference types if you have something like "T : SomeClass". And there's probably other cases too.

if (symbol.GetNullability() == NullableAnnotation.Annotated &&
!(symbol.HasReferenceTypeConstraint && symbol.ReferenceTypeConstraintNullableAnnotation == NullableAnnotation.Annotated))
{
typeSyntax = AddInformationTo(SyntaxFactory.NullableType(typeSyntax), symbol);
}

return typeSyntax;
}
}
}
Expand Down