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

remove prohibition of unmanaged constructed types #604

Open
wants to merge 3 commits into
base: draft-v8
Choose a base branch
from

Conversation

RexJaeschke
Copy link
Contributor

No description provided.

@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Aug 4, 2022
@RexJaeschke RexJaeschke added this to the C# 8.0 milestone Aug 4, 2022
@RexJaeschke RexJaeschke marked this pull request as draft August 4, 2022 19:00
@RexJaeschke RexJaeschke removed the Review: pending Proposal is available for review label Aug 8, 2022
@RexJaeschke RexJaeschke changed the base branch from draft-v7 to draft-v8 August 8, 2022 13:05
@BillWagner BillWagner force-pushed the unmanaged-constructed-types branch 2 times, most recently from 1ac0cd3 to 629b005 Compare February 6, 2023 13:19
@RexJaeschke RexJaeschke added the type: feature This issue describes a new feature label Jul 22, 2023
@BillWagner
Copy link
Member

rebased on the latest draft v8 branch on 9/26/2023

@RexJaeschke RexJaeschke added the Review: pending Proposal is available for review label Oct 13, 2023
@RexJaeschke RexJaeschke marked this pull request as ready for review January 11, 2024 13:53
@jskeet jskeet added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Apr 30, 2024
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

This LGTM @RexJaeschke

I'm happy to merge at the next meeting.

@jskeet
Copy link
Contributor

jskeet commented May 9, 2024

The title talks about unmanaged constructed types - the text seems to be about pointer types. (It looks like constructed types still count as managed.)
I think I need a bit more context to understand this change.

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Added one possible suggestion.

Comment on lines 703 to 704
- Any user-defined *struct_type* that is not a constructed type and contains instance fields of *unmanaged_type*s only.
- In unsafe code ([§23.2](unsafe-code.md#232-unsafe-contexts)), any *pointer_type* ([§23.3](unsafe-code.md#233-pointer-types)).
Copy link
Member

@BillWagner BillWagner May 13, 2024

Choose a reason for hiding this comment

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

Suggested update (Assuming the type parameter must have the unmanaged constraint):

Suggested change
- Any user-defined *struct_type* that is not a constructed type and contains instance fields of *unmanaged_type*s only.
- In unsafe code ([§23.2](unsafe-code.md#232-unsafe-contexts)), any *pointer_type* ([§23.3](unsafe-code.md#233-pointer-types)).
- Any user-defined *struct_type* that contains instance fields of *unmanaged_type*s or where all arguments for all type parameters in a closed constructed types are of *unmanaged_type*s only.
- In unsafe code ([§23.2](unsafe-code.md#232-unsafe-contexts)), any *pointer_type* ([§23.3](unsafe-code.md#233-pointer-types)).

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear that the unmanaged constraint is required, just that the provided type is unmanaged.

Have fun with the below:

  • With the where clause the current VS set to 8.0
    • rejects the 3rd & 4th CheckUnmanaged() calls at compile time – they are not unmanaged types
    • accepts the second even though there is no unmanaged constraint on Coords<>
  • However take out the where and it runs and reports only one of the types is managed, which is a little worrying! But then I copied the test code from StackOverflow…
using System;
using System.Collections.Generic;
using System.Reflection;

namespace UnmanagedConstructed
{
    public struct Coords<T>
    {
        public T X;
        public T Y;
    }

    public static class UnmanagedTypeExtensions
    {
        // U and IsUnmanaged copied from StackOverflow...
        class U<T> where T : unmanaged { }
        public static bool IsUnmanaged(this Type t)
        {
            try { typeof(U<>).MakeGenericType(t); return true; }
            catch (Exception) { return false; }
        }
    }

    public class UnmanagedTypes
    {
        public static void Main()
        {
            CheckUnmanaged<int>();
            CheckUnmanaged<Coords<int>>();
            CheckUnmanaged<object>();           // invalid if below where clause is uncommented
            CheckUnmanaged<Coords<object>>();   // ditto
        }

        private static void CheckUnmanaged<T>() // where T : unmanaged
        {
            Console.WriteLine($"{typeof(T)} is {(typeof(T).IsUnmanaged() ? "unmanaged" : "managed")}");
        }
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussion, we agree with Nigel's analysis, and think we don't need to refer to type parameters, but we want an example (potentially like Nigel's, but simpler - just the method with an unmanaged constraint).

Copy link
Member

Choose a reason for hiding this comment

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

@jskeet Is someone assigned to create the example? (I can't remember from the last meeting)

Copy link
Contributor

Choose a reason for hiding this comment

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

@BillWagner: Me - the example along with the other changes to this PR. If I can just find the time...

Copy link
Contributor

@jskeet jskeet Jul 1, 2024

Choose a reason for hiding this comment

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

I've been having another look at this, and trying to get my head back into where we were in May... and I've failed. I'll mark this as "meeting discuss" again to see if we can get it right together during the next meeting.

I think there's some subtlety around type parameters though - because a type parameter with no constraints won't count as as unmanaged, but a type parameter with the unmanaged constraint will:

class Program
{
    static void Main()
    {
        Check1<int>();
        Check2<int>();
    }

    static void Check1<T>()
    {
        CheckUnmanaged<T>(); // Error, because T *might* not be unmanaged
    }

    static void Check2<T>() where T : unmanaged
    {
        CheckUnmanaged<T>();
    }

    static void CheckUnmanaged<T>() where T : unmanaged
    {
    }
}

Copy link
Contributor

@Nigel-Ecma Nigel-Ecma left a comment

Choose a reason for hiding this comment

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

One comment added in context as its in the range of the changes.

Lines 515, 617 & 699 probably also need to be changed. Also need to consider the unmanaged_type grammar (line 693), should it now allow a type parameter with the unmanaged constraint?

Comment on lines 703 to 704
- Any user-defined *struct_type* that is not a constructed type and contains instance fields of *unmanaged_type*s only.
- In unsafe code ([§23.2](unsafe-code.md#232-unsafe-contexts)), any *pointer_type* ([§23.3](unsafe-code.md#233-pointer-types)).
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear that the unmanaged constraint is required, just that the provided type is unmanaged.

Have fun with the below:

  • With the where clause the current VS set to 8.0
    • rejects the 3rd & 4th CheckUnmanaged() calls at compile time – they are not unmanaged types
    • accepts the second even though there is no unmanaged constraint on Coords<>
  • However take out the where and it runs and reports only one of the types is managed, which is a little worrying! But then I copied the test code from StackOverflow…
using System;
using System.Collections.Generic;
using System.Reflection;

namespace UnmanagedConstructed
{
    public struct Coords<T>
    {
        public T X;
        public T Y;
    }

    public static class UnmanagedTypeExtensions
    {
        // U and IsUnmanaged copied from StackOverflow...
        class U<T> where T : unmanaged { }
        public static bool IsUnmanaged(this Type t)
        {
            try { typeof(U<>).MakeGenericType(t); return true; }
            catch (Exception) { return false; }
        }
    }

    public class UnmanagedTypes
    {
        public static void Main()
        {
            CheckUnmanaged<int>();
            CheckUnmanaged<Coords<int>>();
            CheckUnmanaged<object>();           // invalid if below where clause is uncommented
            CheckUnmanaged<Coords<object>>();   // ditto
        }

        private static void CheckUnmanaged<T>() // where T : unmanaged
        {
            Console.WriteLine($"{typeof(T)} is {(typeof(T).IsUnmanaged() ? "unmanaged" : "managed")}");
        }
    }
}

@jskeet jskeet self-assigned this May 15, 2024
@jskeet
Copy link
Contributor

jskeet commented May 15, 2024

(@jskeet to have another pass at this - in this PR.)

@jskeet
Copy link
Contributor

jskeet commented May 15, 2024

And look for other places, e.g. 515.

@jskeet
Copy link
Contributor

jskeet commented May 15, 2024

Also (try to) remove the grammar as type parameters can be unmanaged - instead, make references to it specify that the type must be unmanaged. (Do this in a separate commit.)

@jskeet
Copy link
Contributor

jskeet commented Oct 28, 2024

We should talk about this again - I'm finding it hard to get my head back into it, so when we talk, let's make verbose notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting Review: pending Proposal is available for review type: feature This issue describes a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants