Address additional PROTOTYPE comments for native integers #42419

cston · 2020-03-14T13:46:55Z

Test plan #38821

AlekseyTs · 2020-03-16T18:20:24Z

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs

@@ -79,6 +80,11 @@ private RetargetingTypeParameterSymbol CreateRetargetingTypeParameter(Symbol sym
            return new RetargetingTypeParameterSymbol(this, (TypeParameterSymbol)symbol);
        }

+        private NativeIntegerTypeSymbol CreateRetargetingNativeInteger(Symbol symbol)
+        {
+            return new NativeIntegerTypeSymbol((NamedTypeSymbol)symbol);


return new NativeIntegerTypeSymbol((NamedTypeSymbol)symbol); [](start = 12, length = 60)

This doesn't feel right. The helpers above are for declarations in source from a referenced compilation that needs retargeting. Native int types are not. #Closed

Also, I think this won't give us the right behavior when underlying type is missing, for example. We are not supposed to create NativeIntegerTypeSymbol instances in cases like that. Please add a test for this scenario with appropriate symbol validation.

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

Please add a test for this scenario

To clarify, the scenario is the types are not missing in compilation #1, but are missing in compilation #2 (the core library is not referenced by compilation #2, or it doesn't have them).

In reply to: 393228651 [](ancestors = 393228651,393226039)

AlekseyTs · 2020-03-16T18:22:29Z

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs

+                    return (NamedTypeSymbol)SymbolMap.GetOrAdd(
+                        RetargetNamedTypeDefinition(type.NativeIntegerUnderlyingType, options),
+                        _retargetingModule._createRetargetingNativeInteger);
+                }


return RetargetNamedTypeDefinition(type.NativeIntegerUnderlyingType, options).AsNativeInteger();? #Closed

AlekseyTs · 2020-03-16T18:26:35Z

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs

@@ -36,6 +36,7 @@ internal partial class RetargetingModuleSymbol
        private readonly Func<Symbol, RetargetingFieldSymbol> _createRetargetingField;
        private readonly Func<Symbol, RetargetingPropertySymbol> _createRetargetingProperty;
        private readonly Func<Symbol, RetargetingEventSymbol> _createRetargetingEvent;
+        private readonly Func<Symbol, NativeIntegerTypeSymbol> _createRetargetingNativeInteger;


private readonly Func<Symbol, NativeIntegerTypeSymbol> _createRetargetingNativeInteger; [](start = 8, length = 87)

Not needed, I think. #Closed

AlekseyTs · 2020-03-16T18:35:37Z

            Assert.True(type.IsNativeIntegerType);

I think we should strengthen the test and also assert containing assembly of the underlying [U]IntPtr type. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:500 in 6c17c5c. [](commit_id = 6c17c5c, deletion_comment = False)

AlekseyTs · 2020-03-16T18:39:38Z

    public void Retargeting()

I think we should also test scenario when we do retargeting for refA but that doesn't involve references of types from core library. We should assert that both compilations are sharing the same type instances. Also, for missing [U]IntPtr types. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:459 in 221b06e. [](commit_id = 221b06e, deletion_comment = False)

AlekseyTs · 2020-03-16T19:00:24Z

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs

@@ -184,6 +184,15 @@ private NamedTypeSymbol RetargetNamedTypeDefinition(NamedTypeSymbol type, Retarg
            {
                Debug.Assert(type.IsDefinition);

+                if (type.IsNativeIntegerType)
+                {
+                    if (!SymbolMap.TryGetValue(type, out var retargeted))


if (!SymbolMap.TryGetValue(type, out var retargeted)) [](start = 19, length = 54)

I do not think the use of SymbolMap is warranted here. Retarget should unify the "underlying" type and AsNativeInteger is supposed to reuse instances itself. #Closed

Also, it looks like using SymbolMap is not even appropriate here. It is for types defined in the compilation that needs retageting of type references. Native integers are not among the types, even their underlying types are not.

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

AlekseyTs · 2020-03-16T19:32:41Z

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs

+                {
+                    if (!SymbolMap.TryGetValue(type, out var retargeted))
+                    {
+                        retargeted = SymbolMap.GetOrAdd(type, Retarget(type.NativeIntegerUnderlyingType, options).AsNativeInteger());


Retarget [](start = 62, length = 8)

Also, do we really need to fall back to Retarget here? It looks like first thing it does is calling RetargetNamedTypeDefinition. Why the indirection? #Closed

AlekseyTs · 2020-03-16T19:46:09Z

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingSymbolTranslator.cs

+                {
+                    if (!SymbolMap.TryGetValue(type, out var retargeted))
+                    {
+                        retargeted = SymbolMap.GetOrAdd(type, Retarget(type.NativeIntegerUnderlyingType, options).AsNativeInteger());


options [](start = 105, length = 7)

Should we pass RetargetOptions.RetargetPrimitiveTypesByTypeCode here to force lookup as special type regardless of the options? #Closed

Otherwise things might fall apart if name of the core library is changed.

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

It looks like we can hit this scenario when native int type is used as base type (an error case, but still something that should be handled reasonably well). It feels like we don't want to force-change the options in this case (to be consistent with what we do for [U]IntPtr references), but it is not clear whether it is appropriate to create a native int wrapper symbol. Will the new underlying symbol even be able able to create one without asserting or failing otherwise? Let's try to come up with a test case.

In reply to: 393276776 [](ancestors = 393276776,393271200)

Added Retargeting_05 although it doesn't hit this code path.

In reply to: 393281988 [](ancestors = 393281988,393276776,393271200)

Added Retargeting_05 although it doesn't hit this code path.

Well, we need to find a way to hit it. One of the previous comments on this thread might help: "Otherwise things might fall apart if name of the core library is changed." Also, it is not clear what "it doesn't hit this code path" means exactly. Are we not doing retargeting at all? This feels suspicious.

In reply to: 394423057 [](ancestors = 394423057,393281988,393276776,393271200)

AlekseyTs · 2020-03-16T20:53:27Z

                new TopLevel(_containingModule, _namespaceName, name, arity, mangleName, isNativeInt: asNativeInt, _lazyErrorInfo, _lazyContainingNamespace, _lazyTypeId, TupleData);

Do we want to cache/reuse these symbols? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/MissingMetadataTypeSymbol.cs:337 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

AlekseyTs · 2020-03-16T20:58:00Z

src/Compilers/CSharp/Portable/Symbols/Source/CustomModifierUtils.cs

@@ -75,7 +75,6 @@ internal static TypeSymbol CopyTypeCustomModifiers(TypeSymbol sourceType, TypeSy
            // NOTE: ref is irrelevant here since we are just encoding/decoding the type out of the signature context
            ImmutableArray<bool> flags = CSharpCompilation.DynamicTransformsEncoder.EncodeWithoutCustomModifierFlags(destinationType, refKind);
            TypeSymbol typeWithDynamic = DynamicTypeDecoder.TransformTypeWithoutCustomModifierFlags(sourceType, containingAssembly, refKind, flags);
-            // PROTOTYPE: Should we use NativeIntegerTypeDecoder here?


// PROTOTYPE: Should we use NativeIntegerTypeDecoder here? [](start = 12, length = 58)

It is not clear why this comment is removed. It doesn't look like the encoder is called, but I think it should be called #Closed

#42500

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

AlekseyTs · 2020-03-16T20:59:55Z

        Debug.Assert(resultType.Equals(destinationType, TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds)); // Same object/dynamic, nullability and tuple names as destination type.

It feels like the assert should be adjusted to ensure native integer types are preserved at the same places. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/CustomModifierUtils.cs:105 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

AlekseyTs · 2020-03-16T21:15:11Z

public struct IntPtr { }

I think it is worth testing a scenario the other way around as well, i.e. when types are appearing rather than disappearing. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:525 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

AlekseyTs · 2020-03-16T21:19:18Z

        comp.VerifyDiagnostics();

I think we should run the test scenarios and observe expected results. A bunch of PROTOTYPE comments were removed in local rewriter, I am not sure if this test is related to that change, but I don't believe VerifyDiagnostics does lowering. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2156 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

cston · 2020-03-16T21:23:40Z

        comp.VerifyDiagnostics();

The comments removed from LocalRewriter were replaced by a single comment for #42452.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2156 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

AlekseyTs · 2020-03-16T21:23:58Z

public struct IntPtr { }

For this case, would also be good to verify that different type references "unify" after retargeting. As we do in the previous unit-test

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:525 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

AlekseyTs · 2020-03-16T21:27:12Z

        comp.VerifyDiagnostics();

Ok, what are we testing here and why it is not worth running the code to verify that it behaves as expected?

In reply to: 599764699 [](ancestors = 599764699,599763146)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2156 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

gafter · 2020-03-17T20:50:36Z

        unaryOp("+", "System.UIntPtr?", "nuint System.UIntPtr.op_UnaryPlus(System.UIntPtr value)", "((System.UIntPtr)3)", "3",

Why is the return type nuint? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:4711 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

gafter · 2020-03-17T20:52:07Z

        unaryOp("-", "System.IntPtr?", "nint System.IntPtr.op_UnaryNegation(System.IntPtr value)", "((System.IntPtr)3)", "-3",

Why is the return type nint? #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:4757 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

gafter · 2020-03-17T21:01:11Z

        unaryOp("~", "System.IntPtr?", "nint System.IntPtr.op_OnesComplement(System.IntPtr value)", "((System.IntPtr)3)", "-4",

Same question here and elsewhere. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:4825 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

gafter · 2020-03-17T21:16:02Z

Finished reviewing (iteration 8)

AlekseyTs · 2020-03-17T21:48:15Z

    public void Retargeting()

It doesn't look like Retargeting_03 asserts that.

I am not sure why this issue was marked as resolved. I see the test renamed to Retargeting_04 in iteration 8, but no other changes made to the test.

In reply to: 599783076 [](ancestors = 599783076,599699140)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:459 in 221b06e. [](commit_id = 221b06e, deletion_comment = False)

gafter

On Unary + applied to IntPtr, it appears that the return type is nint. That doesn't look right. I added a few comments where this issue applies.

AlekseyTs · 2020-03-17T21:51:59Z

Done with review pass (iteration 8) #Closed

cston · 2020-03-18T05:53:01Z

        unaryOp("+", "System.IntPtr?", "nint System.IntPtr.op_UnaryPlus(System.IntPtr value)", "((System.IntPtr)3)", "3",

This matches the code generated for the equivalent int? expression:

    static int? Evaluate(int? operand)
    {
        return +operand;
    }

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:4691 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

cston · 2020-03-18T06:41:14Z

        unaryOp("+", "System.IntPtr?", "nint System.IntPtr.op_UnaryPlus(System.IntPtr value)", "((System.IntPtr)3)", "3",

#42524

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:4691 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

gafter

AlekseyTs · 2020-03-18T16:36:38Z

        Assert.Equal(corLibB, typeB.ContainingAssembly);

I think at least we need to assert whether this is a native int symbol and whether it is an error. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:792 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

AlekseyTs · 2020-03-18T16:36:52Z

        var typeA = comp.GetMember<NamedTypeSymbol>("A").BaseTypeNoUseSiteDiagnostics;

I think at least we need to assert whether this is a native int symbol and whether it is an error. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:779 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

AlekseyTs · 2020-03-18T16:38:42Z

                new TopLevel(_containingModule, _namespaceName, name, arity, mangleName, isNativeInt: asNativeInt, _lazyErrorInfo, _lazyContainingNamespace, _lazyTypeId, TupleData);

Please open an issue to consider following up on this. Refer to the test where we do not unify retargeted symbols, I think you have a comment there about the fact

In reply to: 600123376 [](ancestors = 600123376,599874814,599780574,599777725,599753462)

Refers to: src/Compilers/CSharp/Portable/Symbols/MissingMetadataTypeSymbol.cs:337 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

AlekseyTs · 2020-03-18T16:46:32Z

    public void Retargeting()

The test still doesn't assert that "both compilations are sharing the same type instances". Both compilations are compilation B and compilation C, the one that defines fields and the one that references them. The test asserts something else.

In reply to: 600317374 [](ancestors = 600317374,599783076,599699140)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:459 in 221b06e. [](commit_id = 221b06e, deletion_comment = False)

AlekseyTs · 2020-03-18T16:49:11Z

    public void BuiltInConversions_CSharp8(bool useCompilationReference)

Consider adding WorkItem for dotnet/csharplang#3259 #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2326 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

AlekseyTs · 2020-03-18T16:50:14Z

    public void BuiltInOperators_CSharp8(bool useCompilationReference)

Consider adding WorkItem for this test as well. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:2437 in 733aabd. [](commit_id = 733aabd, deletion_comment = False)

AlekseyTs · 2020-03-18T16:56:24Z

Done with review pass (iteration 9) #Closed

cston · 2020-03-18T17:11:33Z

                new TopLevel(_containingModule, _namespaceName, name, arity, mangleName, isNativeInt: asNativeInt, _lazyErrorInfo, _lazyContainingNamespace, _lazyTypeId, TupleData);

#42535

In reply to: 600737146 [](ancestors = 600737146,600123376,599874814,599780574,599777725,599753462)

Refers to: src/Compilers/CSharp/Portable/Symbols/MissingMetadataTypeSymbol.cs:337 in c6a17a8. [](commit_id = c6a17a8, deletion_comment = False)

AlekseyTs

LGTM (iteration 15), assuming CI succeeds.

cston · 2020-03-18T22:24:18Z

Thanks for the detailed feedback!

cston added the Area-Compilers label Mar 14, 2020

cston force-pushed the various2 branch 2 times, most recently from 8f46a4a to 17a5d43 Compare March 15, 2020 00:54

Use abstract members

27da06a

cston force-pushed the various2 branch 2 times, most recently from 96b9f5e to ca39f2f Compare March 16, 2020 06:22

cston added 2 commits March 16, 2020 07:14

Add System.[U]IntPtr tests

c0127a7

Remove PROTOTYPE comments

6c17c5c

cston force-pushed the various2 branch from ca39f2f to 6c17c5c Compare March 16, 2020 14:19

cston marked this pull request as ready for review March 16, 2020 16:18

cston requested a review from a team as a code owner March 16, 2020 16:18

Retargeting

221b06e

AlekseyTs reviewed Mar 16, 2020

View reviewed changes

PR feedback

f550a3c

AlekseyTs reviewed Mar 16, 2020

View reviewed changes

PR feedback

c6a17a8

AlekseyTs reviewed Mar 16, 2020

View reviewed changes

gafter reviewed Mar 17, 2020

View reviewed changes

PR feedback

9606770

gafter approved these changes Mar 18, 2020

View reviewed changes

cston mentioned this pull request Mar 18, 2020

Consider reusing symbols in native integer symbols in MissingMetadataTypeSymbol #42535

Open

cston added 6 commits March 18, 2020 10:15

PR feedback

f47b7c2

Update test

b1c17a6

Update test

226acbb

Update fix

97f97e3

PR feedback

a87e423

PR feedback

8c80087

AlekseyTs approved these changes Mar 18, 2020

View reviewed changes

cston merged commit b0cafab into dotnet:features/NativeInt Mar 19, 2020

cston deleted the various2 branch March 19, 2020 15:49

Address additional PROTOTYPE comments for native integers #42419

Address additional PROTOTYPE comments for native integers #42419

Conversation

cston commented Mar 14, 2020 • edited by jcouv Loading

AlekseyTs Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 16, 2020 • edited Loading

AlekseyTs commented Mar 16, 2020 • edited Loading

AlekseyTs Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 16, 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

AlekseyTs commented Mar 16, 2020 • edited Loading

AlekseyTs Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 16, 2020 • edited Loading

AlekseyTs commented Mar 16, 2020 • edited Loading

AlekseyTs commented Mar 16, 2020 • edited Loading

cston commented Mar 16, 2020

AlekseyTs commented Mar 16, 2020

AlekseyTs commented Mar 16, 2020

gafter commented Mar 17, 2020 • edited by cston Loading

gafter commented Mar 17, 2020 • edited by cston Loading

gafter commented Mar 17, 2020 • edited by cston Loading

gafter commented Mar 17, 2020

AlekseyTs commented Mar 17, 2020

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 17, 2020 • edited Loading

cston commented Mar 18, 2020

cston commented Mar 18, 2020

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs commented Mar 18, 2020 • edited Loading

AlekseyTs commented Mar 18, 2020 • edited Loading

AlekseyTs commented Mar 18, 2020 • edited Loading

AlekseyTs commented Mar 18, 2020

AlekseyTs commented Mar 18, 2020 • edited Loading

AlekseyTs commented Mar 18, 2020 • edited Loading

AlekseyTs commented Mar 18, 2020 • edited Loading

cston commented Mar 18, 2020

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Mar 18, 2020

cston commented Mar 14, 2020 •

edited by jcouv

Loading

AlekseyTs Mar 16, 2020 •

edited

Loading

AlekseyTs Mar 16, 2020 •

edited

Loading

AlekseyTs Mar 16, 2020 •

edited

Loading

AlekseyTs commented Mar 16, 2020 •

edited

Loading

AlekseyTs commented Mar 16, 2020 •

edited

Loading

AlekseyTs Mar 16, 2020 •

edited

Loading

AlekseyTs Mar 16, 2020 •

edited

Loading

AlekseyTs Mar 16, 2020 •

edited

Loading

AlekseyTs commented Mar 16, 2020 •

edited

Loading

AlekseyTs Mar 16, 2020 •

edited

Loading

AlekseyTs commented Mar 16, 2020 •

edited

Loading

AlekseyTs commented Mar 16, 2020 •

edited

Loading

AlekseyTs commented Mar 16, 2020 •

edited

Loading

gafter commented Mar 17, 2020 •

edited by cston

Loading

gafter commented Mar 17, 2020 •

edited by cston

Loading

gafter commented Mar 17, 2020 •

edited by cston

Loading

AlekseyTs commented Mar 17, 2020 •

edited

Loading

AlekseyTs commented Mar 18, 2020 •

edited

Loading

AlekseyTs commented Mar 18, 2020 •

edited

Loading

AlekseyTs commented Mar 18, 2020 •

edited

Loading

AlekseyTs commented Mar 18, 2020 •

edited

Loading

AlekseyTs commented Mar 18, 2020 •

edited

Loading

AlekseyTs commented Mar 18, 2020 •

edited

Loading