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

Don't generate nullable type when applying value type with unconstrained nullable generic #49154

Merged
merged 8 commits into from
May 17, 2021
Merged

Don't generate nullable type when applying value type with unconstrained nullable generic #49154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dce77d1
Add tests for issue 48472.
huoyaoyuan Nov 3, 2020
3fe21a8
Don't generate NullableTypeSyntax for value type.
huoyaoyuan Nov 3, 2020
328759d
Add test for constrained generic inheritance.
huoyaoyuan Nov 3, 2020
b64f3f2
Add more tests to prevent regression.
huoyaoyuan Nov 4, 2020
083c3e3
Also add tests for implementing interface.
huoyaoyuan Nov 4, 2020
78074bf
Merge branch 'master'
huoyaoyuan Jan 6, 2021
fbb7d61
Remove duplicated tests.
huoyaoyuan Jan 6, 2021
3638c73
Merge branch 'main' into issue-48742
huoyaoyuan May 4, 2021
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
129 changes: 129 additions & 0 deletions src/EditorFeatures/CSharpTest/ImplementAbstractClass/ImplementAbstractClassTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1911,5 +1911,134 @@ public override void AbstractMethod()
}
}", parseOptions: TestOptions.RegularPreview);
}

[WorkItem(48742, "https://github.com/dotnet/roslyn/issues/48742")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementAbstractClass)]
public async Task TestUnconstrainedGenericNullable()
{
await TestAllOptionsOffAsync(
@"#nullable enable

abstract class B<T>
{
public abstract T? M();
}

class [|D|] : B<int>
{
}",
@"#nullable enable

abstract class B<T>
{
public abstract T? M();
}

class D : B<int>
{
public override int M()
{
throw new System.NotImplementedException();
}
}");
}

[WorkItem(48742, "https://github.com/dotnet/roslyn/issues/48742")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementAbstractClass)]
public async Task TestUnconstrainedGenericNullable2()
{
await TestAllOptionsOffAsync(
@"#nullable enable

abstract class B<T>
{
public abstract T? M();
}

class [|D<T>|] : B<T> where T : struct
{
}",
@"#nullable enable

abstract class B<T>
{
public abstract T? M();
}

class D<T> : B<T> where T : struct
{
public override T M()
{
throw new System.NotImplementedException();
}
}");
}
Copy link
Member

Choose a reason for hiding this comment

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

can we a few more tests.

  1. where T is unconstrained
  2. where T is constrained to class.
  3. where T is unconstrained, and passed as T? to the base type
  4. where T is constrained to class, and passed as T? to the base type
  5. where T is constrained to struct, and passed as T? to the base type

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, make sure that other cases doesn't regress.


[WorkItem(48742, "https://github.com/dotnet/roslyn/issues/48742")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementAbstractClass)]
public async Task TestUnconstrainedGenericNullable_Tuple()
{
await TestAllOptionsOffAsync(
@"#nullable enable

abstract class B<T>
{
public abstract T? M();
}

class [|D<T>|] : B<(T, T)>
{
}",
@"#nullable enable

abstract class B<T>
{
public abstract T? M();
}

class D<T> : B<(T, T)>
{
public override (T, T) M()
{
throw new System.NotImplementedException();
}
}");
}

[WorkItem(48742, "https://github.com/dotnet/roslyn/issues/48742")]
[Theory, Trait(Traits.Feature, Traits.Features.CodeActionsImplementAbstractClass)]
[InlineData("", "T")]
[InlineData(" where T : class", "T")]
[InlineData("", "T?")]
[InlineData(" where T : class", "T?")]
[InlineData(" where T : struct", "T?")]
public async Task TestUnconstrainedGenericNullable_NoRegression(string constraint, string passToBase)
{
await TestAllOptionsOffAsync(
$@"#nullable enable

abstract class B<T>
{{
public abstract T? M();
}}

class [|D<T>|] : B<{passToBase}>{constraint}
{{
}}",
$@"#nullable enable

abstract class B<T>
{{
public abstract T? M();
}}

class D<T> : B<{passToBase}>{constraint}
{{
public override T? M()
{{
throw new System.NotImplementedException();
}}
}}");
}
}
}
11 changes: 8 additions & 3 deletions ...xtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,10 @@ public override TypeSyntax VisitNamedType(INamedTypeSymbol symbol)
}
}

if (symbol.NullableAnnotation == NullableAnnotation.Annotated &&
!symbol.IsValueType)
if (symbol is { IsValueType: false, NullableAnnotation: NullableAnnotation.Annotated })
{
// value type with nullable annotation may be composed from unconstrained nullable generic
// doesn't mean nullable value type in this case
typeSyntax = AddInformationTo(SyntaxFactory.NullableType(typeSyntax), symbol);
}
Copy link
Member

Choose a reason for hiding this comment

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

what about other named types that can be value types. like tuples. can you update as well (or add tests showing they're k).

Copy link
Member Author

Choose a reason for hiding this comment

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

Are they special for this case? Here is adding top-level nullability, I couldn't imagine any type to have special semantics. The inner type has been handled above.

Copy link
Member

Choose a reason for hiding this comment

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

they're a named type that is a value type :) i just want to make sure it works properly and nothing unexpected happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding tests should be OK I think


Expand Down Expand Up @@ -355,8 +356,12 @@ public override TypeSyntax VisitPointerType(IPointerTypeSymbol symbol)
public override TypeSyntax VisitTypeParameter(ITypeParameterSymbol symbol)
{
TypeSyntax typeSyntax = AddInformationTo(symbol.Name.ToIdentifierName(), symbol);
if (symbol.NullableAnnotation == NullableAnnotation.Annotated)
if (symbol is { IsValueType: false, NullableAnnotation: NullableAnnotation.Annotated })
{
// value type with nullable annotation may be composed from unconstrained nullable generic
// doesn't mean nullable value type in this case
typeSyntax = AddInformationTo(SyntaxFactory.NullableType(typeSyntax), symbol);
}

return typeSyntax;
}
Expand Down