Allow unmanaged generic structs #31148

RikkiGibson · 2018-11-13T21:43:45Z

Remaining:

Handle expanding cycles when performing closure to check if a constructed type is unmanaged

struct A<T> { B<A<A<T>>> b; }
struct B<T> { T t; }

jaredpar · 2018-11-13T21:46:37Z

This should target preview2

… added

tannergooding · 2018-11-14T23:26:13Z

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs

@@ -93,8 +93,7 @@ private static void StructDependsClosure(NamedTypeSymbol type, HashSet<Symbol> p
        /// IsManagedType is simple for most named types:
        ///     enums are not managed;
        ///     non-enum, non-struct named types are managed;
-        ///     generic types and their nested types are managed;
-        ///     type parameters are managed;
+        ///     type parameters are managed unless an 'unmanaged' constraint is present;


I think, based on https://github.com/dotnet/roslyn/pull/31148/files#diff-fbc5c7a6fe5b3b3c4b6b44d58fb585a1R3269, it should read something like:

type parameters are managed unless an 'unmanaged' constraint is present or the type can be statically determined to be 'unmanaged'

Some additional comment might be warranted. I think the language you suggest conflates type parameters with type arguments though.

Consider the following example

struct MyStruct<T> { T field; unsafe void Foo(MyStruct<T>* ptr) {} } static class MyClass { static unsafe void Bar(MyStruct<int>* ptr) {} }

MyStruct.Foo is invalid because the type parameter T must have an unmanaged constraint to be considered unmanaged. MyClass.Bar is fine, though, because the type instantiation MyStruct<int> can be statically determined to be unmanaged.

Right, that's basically what I meant, I just didn't word it as well as you 😄

src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs

… a type is managed

RikkiGibson · 2018-11-15T00:52:55Z

Added an ad-hoc attempt to prevent exploding the stack for an expanding generic argument in 8e5c646. I'm not sure if it's a correct solution--will revisit as I improve my grasp on the problem.

There may be tests failing because of this right now.

RikkiGibson · 2018-11-15T21:46:05Z

Found a diagnostics nit. When a usage of a type is not valid because it's inaccessible we also consider the type to be managed because its kind is ErrorType instead of Struct.

unsafe class C<U>
{
    // error CS0122: 'S<U>.R' is inaccessible due to its protection level
    // error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('S<U>.R')
    S<U>.R* f3;
}

struct S<T>
{
    struct R { }
}

Does this matter?

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

RikkiGibson · 2018-11-16T00:58:00Z

Seeing strange results in a few tests.

For instance in SemanticErrorTests.CS0208ERR_ManagedAddr02 a subset of the diff is

      -->                 Diagnostic(ErrorCode.ERR_ManagedAddr, "object*").WithArguments("object")
      ++>                 Diagnostic(ErrorCode.ERR_ManagedAddr, "object*").WithArguments("object")

Hard to tell what difference actually exists between these diagnostics.

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

src/Compilers/CSharp/Test/Emit/Emit/ResourceTests.cs

jcouv · 2018-11-26T20:20:54Z

src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs

@@ -3270,5 +3268,366 @@ void Test()
                //         UnmanagedWithInterface(&a);         // fail (does not match interface)
                Diagnostic(ErrorCode.ERR_GenericConstraintNotSatisfiedValType, "UnmanagedWithInterface").WithArguments("C.UnmanagedWithInterface<T>(T*)", "System.IDisposable", "T", "int").WithLocation(20, 9));
        }
+
+        [Fact]
+        public void UnmanagedGenericStructPointer()


UnmanagedGenericStructPointer [](start = 20, length = 29)

Consider testing IsManagedType in various symbols would be useful, even though it was not yet made a public API.

Will handle this in a future PR

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs

gafter · 2018-11-27T21:24:06Z

    IsUnmanaged<Wrapper<int>.S>();          // Invalid

Remove the // Invalid comment?

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs:2983 in 1c2f261. [](commit_id = 1c2f261, deletion_comment = False)

gafter · 2018-11-27T21:24:16Z

    IsUnmanaged<Wrapper<string>.S>();          // Invalid

Remove the // Invalid comment?

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs:2993 in 1c2f261. [](commit_id = 1c2f261, deletion_comment = False)

RikkiGibson · 2018-12-04T00:04:21Z

Addressed some of the easier feedback. Will look into testing IsManagedType.

Originally created when everything was VSTS. This gets us back inline with the new branding.

RikkiGibson · 2018-12-06T01:50:18Z

A few emit tests are failing now. Will look into why soon.

agocke

LGTM. I think we need a few follow-on tests in a later PR:

Test for ref structs
Test the IsManagedType API as suggested by Julien
Test moveability (exiting rules should be sufficient)
Have at least one test spit some IL to look at and have a few tests run with "expected output" to make sure the CLR can handle the code.

RikkiGibson · 2018-12-06T20:32:18Z

will add to #31374

RikkiGibson · 2018-12-06T20:32:48Z

is a second sign off needed? or just when the feature is PRed to preview 2?

jaredpar · 2018-12-06T20:52:28Z

@RikkiGibson

is a second sign off needed? or just when the feature is PRed to preview 2?

Yes. The only times a second sign off is not needed:

Merge only PR to a feature branch: this needs zero sign offs.
Merge only PR to master branch: one sign off .
Test change only: need one sign off.

glenn-slayden · 2019-02-13T14:08:10Z

Does anyone know when this commit might show up in a Microsoft.Net.Compilers.*.*.* release for desktop/Framework?

jaredpar · 2019-02-13T16:16:23Z

It is already on our myget feed. It will appear on NuGet, in preview mode, around the time VS 2019 goes RTM.

* Add unmanaged generic struct tests * Don't bail out of managed type checks when type arguments are present * Simplify test * Delegate HasPointerType to underlying field in WrappedFieldSymbol * Add test for recursive expansion of struct with T : unmanaged * Delay constraint checking on field types until after type members are added * Update comment * Add expected diagnostic to UnmanagedRecursiveTypeArgument test * Add unmanaged recursive type argument tests with constraint violations * Don't search fields whose type is circular struct when determining if a type is managed * Update some tests * Use TypeSyntax.Location for Type.CheckAllConstraints * Add prototype comment * Update GenericConstraintsTests * Update IsManagedType asserts in UnsafeTests * Fix ManagedAddressOfTests.DisallowSizeof * Use SourceMemberFieldSymbol.SyntaxNode.Location for consistency in diagnostics * Add some tests and diagnostics * Fix more tests * Fix BaseClassTests * Add test for partially constructed generic struct which violates unmanaged constraint * Consider nullability when running AfterTypeMembersChecks * Update NullableReferenceTypesTests.Constraint_Oblivious due to differences from delayed constraint checks on field types * Check if NRT enabled in compilation to use correct nullability in constraint checks * Make UnmanagedConstraint_StructMismatchInImplements behave consistently * Disable ResourceTests.AddResourceToModule in Mono * Remove prototype comment * Remove invalid comments from UnmanagedConstraints_NestedInGenericType * Use ErrorLocation instead of SyntaxNode.Location * Rename our pipeline YAML files Originally created when everything was VSTS. This gets us back inline with the new branding. * Fix tests now that we use ErrorLocation for field constraint checks

RikkiGibson added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Nov 13, 2018

RikkiGibson requested a review from a team as a code owner November 13, 2018 21:43

jcouv added this to the 16.0.P2 milestone Nov 13, 2018

RikkiGibson added 5 commits November 13, 2018 17:38

Add unmanaged generic struct tests

0b21ea1

Don't bail out of managed type checks when type arguments are present

312e107

Simplify test

264917e

Delegate HasPointerType to underlying field in WrappedFieldSymbol

7e9dff7

Add test for recursive expansion of struct with T : unmanaged

6976f6b

RikkiGibson force-pushed the unmanaged-generic-struct branch from 0ff11ea to 6976f6b Compare November 14, 2018 01:45

RikkiGibson requested a review from a team as a code owner November 14, 2018 01:45

RikkiGibson changed the base branch from master to dev16.0-preview2 November 14, 2018 01:45

RikkiGibson added 2 commits November 14, 2018 12:27

Delay constraint checking on field types until after type members are…

4c40cc1

… added

Update comment

e0c6264

tannergooding reviewed Nov 14, 2018

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/GenericConstraintsTests.cs Outdated Show resolved Hide resolved

RikkiGibson added 4 commits November 14, 2018 15:32

Add expected diagnostic to UnmanagedRecursiveTypeArgument test

9cbac2b

Add unmanaged recursive type argument tests with constraint violations

94260f6

Don't search fields whose type is circular struct when determining if…

8e5c646

… a type is managed

Update some tests

5b94ec8

Use TypeSyntax.Location for Type.CheckAllConstraints

297d9ad

RikkiGibson commented Nov 15, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs Outdated Show resolved Hide resolved

RikkiGibson changed the base branch from dev16.0-preview2 to features/unmanaged-constructed-types November 15, 2018 23:49

RikkiGibson added 3 commits November 15, 2018 16:26

Add prototype comment

6d02643

Update GenericConstraintsTests

f2d16ff

Update IsManagedType asserts in UnsafeTests

16fad8e

cston reviewed Nov 26, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs Outdated Show resolved Hide resolved

cston reviewed Nov 26, 2018

View reviewed changes

src/Compilers/CSharp/Test/Emit/Emit/ResourceTests.cs Show resolved Hide resolved

jcouv reviewed Nov 26, 2018

View reviewed changes

gafter reviewed Nov 27, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/BaseTypeAnalysis.cs Outdated Show resolved Hide resolved

tannergooding mentioned this pull request Dec 3, 2018

Changing constraint from "struct" to "unmanaged" causes stack overflow and VS crash. #31439

Closed

RikkiGibson added 2 commits December 3, 2018 15:42

Remove prototype comment

ba82179

Remove invalid comments from UnmanagedConstraints_NestedInGenericType

f8a0900

RikkiGibson force-pushed the unmanaged-generic-struct branch from e554276 to f8a0900 Compare December 3, 2018 23:55

Use ErrorLocation instead of SyntaxNode.Location

6384e98

Rename our pipeline YAML files

af1b0b4

Originally created when everything was VSTS. This gets us back inline with the new branding.

agocke self-assigned this Dec 6, 2018

agocke approved these changes Dec 6, 2018

View reviewed changes

Fix tests now that we use ErrorLocation for field constraint checks

6c0329a

jaredpar approved these changes Dec 6, 2018

View reviewed changes

RikkiGibson merged commit 4c0a9ee into dotnet:features/unmanaged-constructed-types Dec 6, 2018

RikkiGibson deleted the unmanaged-generic-struct branch December 6, 2018 21:57

This was referenced Jan 5, 2019

Champion: Unmanaged constructed types (16.3, Core 3) dotnet/csharplang#1937

Open

Generic struct type whose fields are all unmanaged types is unmanaged fsharp/fslang-suggestions#692

Open

RikkiGibson restored the unmanaged-generic-struct branch January 16, 2019 21:27

RikkiGibson deleted the unmanaged-generic-struct branch January 16, 2019 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow unmanaged generic structs #31148

Allow unmanaged generic structs #31148

RikkiGibson commented Nov 13, 2018 •

edited

Loading

jaredpar commented Nov 13, 2018

tannergooding Nov 14, 2018

RikkiGibson Nov 15, 2018

tannergooding Nov 15, 2018

RikkiGibson commented Nov 15, 2018

RikkiGibson commented Nov 15, 2018 •

edited

Loading

RikkiGibson commented Nov 16, 2018

jcouv Nov 26, 2018

RikkiGibson Dec 5, 2018

gafter commented Nov 27, 2018

gafter commented Nov 27, 2018

RikkiGibson commented Dec 4, 2018

RikkiGibson commented Dec 6, 2018

agocke left a comment

RikkiGibson commented Dec 6, 2018

RikkiGibson commented Dec 6, 2018

jaredpar commented Dec 6, 2018

glenn-slayden commented Feb 13, 2019

jaredpar commented Feb 13, 2019

Allow unmanaged generic structs #31148

Allow unmanaged generic structs #31148

Conversation

RikkiGibson commented Nov 13, 2018 • edited Loading

jaredpar commented Nov 13, 2018

tannergooding Nov 14, 2018

Choose a reason for hiding this comment

RikkiGibson Nov 15, 2018

Choose a reason for hiding this comment

tannergooding Nov 15, 2018

Choose a reason for hiding this comment

RikkiGibson commented Nov 15, 2018

RikkiGibson commented Nov 15, 2018 • edited Loading

RikkiGibson commented Nov 16, 2018

jcouv Nov 26, 2018

Choose a reason for hiding this comment

RikkiGibson Dec 5, 2018

Choose a reason for hiding this comment

gafter commented Nov 27, 2018

gafter commented Nov 27, 2018

RikkiGibson commented Dec 4, 2018

RikkiGibson commented Dec 6, 2018

agocke left a comment

Choose a reason for hiding this comment

RikkiGibson commented Dec 6, 2018

RikkiGibson commented Dec 6, 2018

jaredpar commented Dec 6, 2018

glenn-slayden commented Feb 13, 2019

jaredpar commented Feb 13, 2019

RikkiGibson commented Nov 13, 2018 •

edited

Loading

RikkiGibson commented Nov 15, 2018 •

edited

Loading