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.

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

Generate friendly syntax for nint/nuint #42999

Merged
merged 6 commits into from
Apr 2, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,62 @@ void Method(int? x)
await TestInRegularAndScriptAsync(before, after, options: ExplicitTypeExceptWhereApparent());
}

#if !CODE_STYLE // TODO: Skipped tests in CodeStyle layer depend on new compiler API (IsNativeIntegerType) that is not available in CodeStyle layer.
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 there is a similar region below for tests involving new nullability APIs

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, yes this is a know drawback of shared code between CodeStyle package based on earlier CodeAnalysis version and Features layer based on latest Roslyn. I plan to bring this up for discussion at next IDE design meeting on how to handle these changes in future.

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 explicitly update #41462 to mention nint/nuint?

Copy link
Member

Choose a reason for hiding this comment

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

it can link to this pr btw.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a note in #41462 with link to this PR

// https://github.com/dotnet/roslyn/issues/41462 tracks adding this support

[WpfFact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExplicitType)]
[WorkItem(42986, "https://github.com/dotnet/roslyn/issues/42986")]
public async Task InNativeIntIntrinsicType()
{
var before = @"
class Program
{
void Method(nint x)
{
[|var|] y = x;
}
}";
var after = @"
class Program
{
void Method(nint x)
{
nint y = x;
}
}";
// The type is intrinsic and not apparent
await TestInRegularAndScriptAsync(before, after, options: ExplicitTypeEverywhere());
await TestInRegularAndScriptAsync(before, after, options: ExplicitTypeForBuiltInTypesOnly());
await TestInRegularAndScriptAsync(before, after, options: ExplicitTypeExceptWhereApparent());
}

[WpfFact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExplicitType)]
[WorkItem(42986, "https://github.com/dotnet/roslyn/issues/42986")]
public async Task InNativeUnsignedIntIntrinsicType()
{
var before = @"
class Program
{
void Method(nuint x)
{
[|var|] y = x;
}
}";
var after = @"
class Program
{
void Method(nuint x)
{
nuint y = x;
}
}";
// The type is intrinsic and not apparent
await TestInRegularAndScriptAsync(before, after, options: ExplicitTypeEverywhere());
await TestInRegularAndScriptAsync(before, after, options: ExplicitTypeForBuiltInTypesOnly());
await TestInRegularAndScriptAsync(before, after, options: ExplicitTypeExceptWhereApparent());
}
#endif

[WpfFact, Trait(Traits.Feature, Traits.Features.CodeActionsUseExplicitType)]
[WorkItem(27221, "https://github.com/dotnet/roslyn/issues/27221")]
public async Task WithRefIntrinsicType()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7517,6 +7517,34 @@ private T First<T>(List<T> list)
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)]
[WorkItem(42986, "https://github.com/dotnet/roslyn/issues/42986")]
public async Task MethodWithNativeIntegerTypes()
{
await TestInRegularAndScriptAsync(
@"class Class
{
void M(nint i, nuint i2)
{
(nint, nuint) d = [|NewMethod|](i, i2);
}
}",
@"using System;

class Class
{
void M(nint i, nuint i2)
{
(nint, nuint) d = NewMethod(i, i2);
}

private (nint, nuint) NewMethod(nint i, nuint i2)
{
throw new NotImplementedException();
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateMethod)]
public async Task MethodWithTuple()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,35 @@ public void Method1()
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
[WorkItem(42986, "https://github.com/dotnet/roslyn/issues/42986")]
public async Task TestMethodWithNativeIntegers()
{
await TestWithAllCodeStyleOptionsOffAsync(
@"interface IInterface
{
(nint, nuint) Method(nint x, nuint x2);
}

class Class : [|IInterface|]
{
}",
@"using System;

interface IInterface
{
(nint, nuint) Method(nint x, nuint x2);
}

class Class : IInterface
{
public (nint, nuint) Method(nint x, nuint x2)
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 expected we would produce attribute here (I saw logic to remove attribute in implemented interfaces, see AttributesToRemove) and I would have to filter the NativeIntegerAttribute. But it seems to be working okay as-is.

Copy link
Member

Choose a reason for hiding this comment

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

@jcouv Are the attributes only included if we're reading from metadata? I believe we have tests that test that case too. We get bitten by this.

{
throw new NotImplementedException();
}
}");
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
public async Task TestMethodWithTuple()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,54 @@ I am the very model of a modern major general.
Assert.Equal(expectedXMLFragment, extractedXMLFragment);
}

[Fact, Trait(Traits.Feature, Traits.Features.MetadataAsSource)]
[WorkItem(42986, "https://github.com/dotnet/roslyn/issues/42986")]
public async Task TestNativeInteger()
{
var metadataSource = "public class C { public nint i; public nuint i2; }";
var symbolName = "C";

await GenerateAndVerifySourceAsync(metadataSource, symbolName, LanguageNames.CSharp, languageVersion: "Preview", metadataLanguageVersion: "Preview",
expected: $@"#region {FeaturesResources.Assembly} ReferencedAssembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
// {CodeAnalysisResources.InMemoryAssembly}
#endregion

using System;
using System.Runtime.CompilerServices;

public class [|C|]
{{
[NativeIntegerAttribute]
Copy link
Member

Choose a reason for hiding this comment

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

We should be dropping these, right?

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 don't think so. Notice the test below for tuples which I included for reference.

Copy link
Member

Choose a reason for hiding this comment

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

@jcouv That's another bug. 😄

public nint i;
[NativeIntegerAttribute]
public nuint i2;

public C();
}}");
}

[Fact, Trait(Traits.Feature, Traits.Features.MetadataAsSource)]
public async Task TestTupleWithNames()
{
var metadataSource = "public class C { public (int a, int b) t; }";
var symbolName = "C";

await GenerateAndVerifySourceAsync(metadataSource, symbolName, LanguageNames.CSharp,
expected: $@"#region {FeaturesResources.Assembly} ReferencedAssembly, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
// {CodeAnalysisResources.InMemoryAssembly}
#endregion

using System.Runtime.CompilerServices;

public class [|C|]
{{
[TupleElementNames(new[] {{ ""a"", ""b"" }})]
public (int a, int b) t;

public C();
}}");
}

[Fact, WorkItem(26605, "https://github.com/dotnet/roslyn/issues/26605")]
public async Task TestValueTuple()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,11 @@ public static bool IsNumericType([NotNullWhen(returnValue: true)] this ITypeSymb
case SpecialType.System_Single:
case SpecialType.System_Double:
case SpecialType.System_Decimal:
#if !CODE_STYLE // TODO: Remove the #if once IsNativeIntegerType is available.
// https://github.com/dotnet/roslyn/issues/41462 tracks adding this support
case SpecialType.System_IntPtr when type.IsNativeIntegerType:
case SpecialType.System_UIntPtr when type.IsNativeIntegerType:
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'm not sure how to cover those cases, but made the change opportunistically anyway

Copy link
Member

Choose a reason for hiding this comment

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

interesting that it's not a special SpecialType. i.e. why not SpecialType.Nint?

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'll let @cston answer for design of nint type symbol.

Copy link
Member

@cston cston Apr 1, 2020

Choose a reason for hiding this comment

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

SpecialType has a very specific definition, and it did not seem appropriate to change that. See SpecialType.cs.


In reply to: 401951786 [](ancestors = 401951786)

Copy link
Member

Choose a reason for hiding this comment

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

@cston As a consumer of the API, how does that specific definition make sense? This feels like an implementation detail leaking in some way?

#endif
return true;
}
}
Expand Down Expand Up @@ -393,6 +398,11 @@ public static bool IsSpecialType([NotNullWhen(returnValue: true)] this ITypeSymb
case SpecialType.System_UInt16:
case SpecialType.System_UInt32:
case SpecialType.System_UInt64:
#if !CODE_STYLE // TODO: Remove the #if once IsNativeIntegerType is available.
// https://github.com/dotnet/roslyn/issues/41462 tracks adding this support
case SpecialType.System_IntPtr when symbol.IsNativeIntegerType:
case SpecialType.System_UIntPtr when symbol.IsNativeIntegerType:
#endif
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.FindSymbols.Finders;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Simplification;
Expand Down Expand Up @@ -71,6 +71,7 @@ public override TypeSyntax VisitArrayType(IArrayTypeSymbol symbol)
underlyingType = innerArray.ElementType;

#if !CODE_STYLE // TODO: Remove the #if once NullableAnnotation is available.
// https://github.com/dotnet/roslyn/issues/41462 tracks adding this support
if (underlyingType.NullableAnnotation == NullableAnnotation.Annotated)
{
// If the inner array we just moved to is also nullable, then
Expand Down Expand Up @@ -107,6 +108,7 @@ public override TypeSyntax VisitArrayType(IArrayTypeSymbol symbol)
TypeSyntax arrayTypeSyntax = SyntaxFactory.ArrayType(elementTypeSyntax, ranks.ToSyntaxList());

#if !CODE_STYLE // TODO: Remove the #if once NullableAnnotation is available.
// https://github.com/dotnet/roslyn/issues/41462 tracks adding this support
if (symbol.NullableAnnotation == NullableAnnotation.Annotated)
{
arrayTypeSyntax = SyntaxFactory.NullableType(arrayTypeSyntax);
Expand Down Expand Up @@ -188,6 +190,14 @@ private TypeSyntax TryCreateSpecializedNamedTypeSyntax(INamedTypeSymbol symbol)
return CreateTupleTypeSyntax(symbol);
}

#if !CODE_STYLE // TODO: Remove the #if once IsNativeIntegerType is available.
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 there is a similar region below for tests involving new nullability APIs

Copy link
Member

Choose a reason for hiding this comment

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

note: given the redundancy in checks in these places, IDE will likely just have an extension of IsNint and IsNuint since really, that's what we want to ask :)

// https://github.com/dotnet/roslyn/issues/41462 tracks adding this support
if (symbol.IsNativeIntegerType)
{
return SyntaxFactory.IdentifierName(symbol.SpecialType == SpecialType.System_IntPtr ? "nint" : "nuint");
}
#endif

if (symbol.IsNullable())
{
// Can't have a nullable of a pointer type. i.e. "int*?" is illegal.
Expand Down Expand Up @@ -260,6 +270,7 @@ public override TypeSyntax VisitNamedType(INamedTypeSymbol symbol)
}

#if !CODE_STYLE // TODO: Remove the #if once NullableAnnotation is available.
// https://github.com/dotnet/roslyn/issues/41462 tracks adding this support
if (symbol.NullableAnnotation == NullableAnnotation.Annotated)
{
typeSyntax = AddInformationTo(SyntaxFactory.NullableType(typeSyntax), symbol);
Expand Down