Generate friendly syntax for nint/nuint #42999

jcouv · 2020-04-01T22:33:54Z

Fixes #42986
Also fixes classification for nint/nuint (should be a keyword when bound as native integers)

Taggging @jasonmalinowski @CyrusNajmabadi @mavasani for review.
Tagging @cston FYI

Relates to #38821

jcouv · 2020-04-01T22:35:34Z

src/Analyzers/CSharp/Tests/UseImplicitOrExplicitType/UseExplicitTypeTests.cs

@@ -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.


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

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.

can we explicitly update #41462 to mention nint/nuint?

it can link to this pr btw.

Added a note in #41462 with link to this PR

jcouv · 2020-04-01T22:36:46Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs

+#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:


📝 I'm not sure how to cover those cases, but made the change opportunistically anyway

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

I'll let @cston answer for design of nint type symbol.

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

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

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

jcouv · 2020-04-01T22:36:53Z

...ndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs

@@ -188,6 +188,24 @@ private TypeSyntax TryCreateSpecializedNamedTypeSyntax(INamedTypeSymbol symbol)
                    return CreateTupleTypeSyntax(symbol);
                }

+#if !CODE_STYLE // TODO: Remove the #if once IsNativeIntegerType is available.


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

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 :)

jcouv · 2020-04-01T22:38:41Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

+
+class Class : IInterface
+{
+    public (nint, nuint) Method(nint x, nuint x2)


📝 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.

@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.

CyrusNajmabadi · 2020-04-01T22:42:19Z

...ndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs

+                        Debug.Assert(symbol.SpecialType == SpecialType.System_UIntPtr);
+                        name = "nuint";
+                    }
+                    return SyntaxFactory.IdentifierName(name);


honestly, i'm ok with return SyntaxFactory.IdentifierName(symbol.SpecialType == SpecialType.System_IntPtr ? "nint" : "nuint"); :)

CyrusNajmabadi · 2020-04-01T22:43:52Z

So overall feedback is that i'm fine with this for now. one issue is that this will generate nint/nuint in projects that don't support that. i'm not a fan of that, but i recognize it would be a bunch of work to theread through the options from all the callers to here to support doing the right thing.

GIven that this is a niche feature and will likel be used primarily in projects all targetting new C#, i think this is an acceptable place to merge.

CyrusNajmabadi

Approving now. LMK if you make any major changes that i should rereview.

jasonmalinowski

Looks like this isn't doing dropping of the attributes when it should. I suspect the "implement interface" case does work which was surprising because the attributes are only appearing in PE-imported symbols, not ones from source.

jasonmalinowski · 2020-04-01T23:59:45Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

+
+class Class : IInterface
+{
+    public (nint, nuint) Method(nint x, nuint x2)


@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.

jasonmalinowski · 2020-04-02T00:00:05Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.CSharp.cs

+
+public class [|C|]
+{{
+    [NativeIntegerAttribute]


We should be dropping these, right?

I don't think so. Notice the test below for tuples which I included for reference.

@jcouv That's another bug. 😄

jasonmalinowski · 2020-04-02T00:01:22Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ITypeSymbolExtensions.cs

+#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:


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

jcouv · 2020-04-02T00:53:10Z

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.

I couldn't find a test that actually uses metadata, but we have a test that simulates it (the only test I could find that breaks when removing the filtering logic). I did the same here and added the filtering logic for NativeIntegerAttribute as well.
Thanks

jcouv · 2020-04-02T05:45:11Z

Added tests for classification as keywords (conditionally on semantic model) and corresponding fix. Thanks @jasonmalinowski, I had not noticed the colorization difference with int in IDE.

jcouv · 2020-04-02T15:30:47Z

@CyrusNajmabadi @mavasani @jasonmalinowski I added filtering for the attribute and fixed classification.

cston · 2020-04-02T15:32:56Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

+
+interface IInterface
+{
+    [return: System.Runtime.CompilerServices.NativeInteger(new[] { true, true })]


[return: System.Runtime.CompilerServices.NativeInteger(new[] { true, true })] [](start = 4, length = 77)

Does this test involve metadata? If not, the attribute definition and explicit attributes are not needed.

Discussed and resolved offline. See comment above and in existing test (// Note: we're putting the attribute by hand to simulate metadata)

Addressed feedback

jasonmalinowski

Looks better but as called out the attributes appearing in Metadata as Source is a bug, and the tuple name situation is also a bug. 😄

jasonmalinowski · 2020-04-02T17:57:20Z

src/EditorFeatures/Test/MetadataAsSource/MetadataAsSourceTests.CSharp.cs

+
+public class [|C|]
+{{
+    [NativeIntegerAttribute]


@jcouv That's another bug. 😄

jasonmalinowski · 2020-04-02T18:05:59Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

+        [WorkItem(42986, "https://github.com/dotnet/roslyn/issues/42986")]
+        public async Task TestMethodWithNativeIntegers()
+        {
+            var nativeIntegerAttributeDefinition = @"


There's a way to have this generate a proper metadata reference:

roslyn/src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

Lines 436 to 462 in cdb15f3

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]

public async Task NoNullableAttributesInMethodFromMetadata()

{

var initial = @"

<Workspace>

<Project Language=""C#"" AssemblyName=""Assembly1"" CommonReferences=""true"">

<MetadataReferenceFromSource Language=""C#"" CommonReferences=""true"">

<Document>

#nullable enable

public interface IInterface

{

void M(string? s1, string s2);

string this[string? s1, string s2] { get; set; }

}

</Document>

</MetadataReferenceFromSource>

<Document>

#nullable enable

using System;

class C : [|IInterface|]

{

}</Document>

</Project>

</Workspace>";

That way you can just use the actual feature syntax in the reference, which also means you won't have any issues if this isn't quite expressing reality.

jcouv · 2020-04-02T18:11:48Z

Filed #43028 for MetadataAsSource issue.

There's a way to have this generate a proper metadata reference

Thanks. I missed that trick. I'll stick with current approach for now because trying to get this in so we can merge the feature branch in. Test tweak doesn't seem blocker.

jcouv added 2 commits April 1, 2020 15:32

Generate friendly syntax for nint/nuint

63455b8

Opportunistic change

d3de713

jcouv requested a review from a team as a code owner April 1, 2020 22:33

jcouv self-assigned this Apr 1, 2020

jcouv added the Area-IDE label Apr 1, 2020

jcouv commented Apr 1, 2020

View reviewed changes

CyrusNajmabadi reviewed Apr 1, 2020

View reviewed changes

CyrusNajmabadi approved these changes Apr 1, 2020

View reviewed changes

Use ternary and add comments

4ac5573

mavasani approved these changes Apr 1, 2020

View reviewed changes

Fix formatting

565f0c8

jasonmalinowski previously requested changes Apr 2, 2020

View reviewed changes

Remove NativeIntegerAttribute in generated source

d0a4148

Fix classification for nint/nuint

cdb15f3

jcouv requested a review from a team as a code owner April 2, 2020 05:34

cston reviewed Apr 2, 2020

View reviewed changes

cston approved these changes Apr 2, 2020

View reviewed changes

jcouv added the Area-Compilers label Apr 2, 2020

jcouv requested a review from jasonmalinowski April 2, 2020 17:59

jcouv mentioned this pull request Apr 2, 2020

MetadataAsSource should not display certain compiler attributes #43028

Closed

jasonmalinowski approved these changes Apr 2, 2020

View reviewed changes

333fred approved these changes Apr 2, 2020

View reviewed changes

jcouv merged commit cdafef6 into dotnet:features/NativeInt Apr 2, 2020

jcouv deleted the nint-syntax branch April 2, 2020 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate friendly syntax for nint/nuint #42999

Generate friendly syntax for nint/nuint #42999

jcouv commented Apr 1, 2020 •

edited

Loading

jcouv Apr 1, 2020

mavasani Apr 1, 2020

CyrusNajmabadi Apr 1, 2020

CyrusNajmabadi Apr 1, 2020

jcouv Apr 1, 2020

jcouv Apr 1, 2020

CyrusNajmabadi Apr 1, 2020

jcouv Apr 1, 2020

cston Apr 1, 2020 •

edited

Loading

jasonmalinowski Apr 2, 2020

jcouv Apr 1, 2020

CyrusNajmabadi Apr 1, 2020

jcouv Apr 1, 2020

jasonmalinowski Apr 1, 2020

CyrusNajmabadi Apr 1, 2020

CyrusNajmabadi commented Apr 1, 2020

CyrusNajmabadi left a comment

jasonmalinowski left a comment

jasonmalinowski Apr 1, 2020

jasonmalinowski Apr 2, 2020

jcouv Apr 2, 2020

jasonmalinowski Apr 2, 2020

jasonmalinowski Apr 2, 2020

jcouv commented Apr 2, 2020

jcouv commented Apr 2, 2020

jcouv commented Apr 2, 2020

cston Apr 2, 2020

jcouv Apr 2, 2020

jasonmalinowski left a comment

jasonmalinowski Apr 2, 2020

jasonmalinowski Apr 2, 2020

jcouv commented Apr 2, 2020

	[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsImplementInterface)]
	public async Task NoNullableAttributesInMethodFromMetadata()
	{
	var initial = @"
	<Workspace>
	<Project Language=""C#"" AssemblyName=""Assembly1"" CommonReferences=""true"">
	<MetadataReferenceFromSource Language=""C#"" CommonReferences=""true"">
	<Document>
	#nullable enable

	public interface IInterface
	{
	void M(string? s1, string s2);
	string this[string? s1, string s2] { get; set; }
	}
	</Document>
	</MetadataReferenceFromSource>
	<Document>
	#nullable enable

	using System;

	class C : [\|IInterface\|]
	{
	}</Document>
	</Project>
	</Workspace>";

Generate friendly syntax for nint/nuint #42999

Generate friendly syntax for nint/nuint #42999

Conversation

jcouv commented Apr 1, 2020 • edited Loading

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

cston Apr 1, 2020 • edited Loading

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 Apr 1, 2020

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

jasonmalinowski left a 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

jcouv commented Apr 2, 2020

jcouv commented Apr 2, 2020

jcouv commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Apr 2, 2020

jcouv commented Apr 1, 2020 •

edited

Loading

cston Apr 1, 2020 •

edited

Loading