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

[WIP] Unmanaged constructed types #6064

Closed
wants to merge 4 commits into from

Conversation

TIHan
Copy link
Member

@TIHan TIHan commented Jan 6, 2019

Matching C#'s feature: dotnet/csharplang#1937

Allows generic structs to be 'unmanaged' at construction if it can determine to be so.

A few examples:

[<Struct>]
type S<'T, 'U> =

    [<DefaultValue(false)>] val X : 'T

let test (x: 'T when 'T : unmanaged) = ()

let passing () =
    test (S<float, int>())

let passing2 () =
    test (S<float, obj>()) // passes as the 'obj' type arg is not used as part of the backing field of a struct

let passing3<'T when 'T : unmanaged> () =
    test (S<'T, obj>())

let not_passing () =
    test (S<obj, int>())

let not_passing2<'T> () =
    test (S<'T, obj>())

Another example:

[<Struct>]
type S<'T> =

    val X : 'T

    new (x) = { X = x }

let test (x: 'T when 'T : unmanaged) = ()

let passing () =
    test (S(1))

let not_passing () =
    test (S(obj()))

Example on nested generic structs:

[<Struct>]
type W<'T> = { x: 'T }

[<Struct>]
type S<'T> = { x: W<'T> }

let test (x: 'T when 'T : unmanaged) = ()

let passing () =
    test Unchecked.defaultof<S<int>>

let not_passing () =
    test Unchecked.defaultof<S<obj>>

Remaining work:

  • Tests
  • Emit IsUnmanagedAttribute on type arguments with the 'unmanaged' constraint for interop.
  • Consume type arguments from .NET assemblies that have the IsUnmanagedAttribute to mean they have the unmanaged constraint. This is also for interop.
  • Support anonymous struct records
  • Support struct unions
  • Fix struct tuples

Regarding the struct tuples, this works:

let passing () = 
    let x = struct(1, 2)
    test x

But this doesn't:

let not_passing () = 
    test (struct(1, 2))

After some debugging, it looks like at the point where isUnmanagedTy gets called, the type arguments for the tuple haven't been solved yet which seems weird to me; this is a separate bug in the type system.

@TIHan TIHan added this to the Unknown milestone Jan 6, 2019
@TIHan TIHan self-assigned this Jan 6, 2019
@TIHan TIHan changed the base branch from master to dev16.0 January 6, 2019 04:12
@TIHan TIHan changed the title Unmanaged constructed types [WIP] Unmanaged constructed types Jan 6, 2019
match tryDestAppTy g ty with
| ValueSome tcref ->
match ty with
| TType_app(tcref, tinst) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

You should almost never match on TType directly - always use tryAppTy (note, tryDestAppTy should really be called tryTcrefOfAppTy)

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason you should never match on TType is that it might be a solved type variable or a type abbreviation, both of which get expanded by tryAppTy and friends

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fair. I thought it was fine considering we were stripping eqns anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're correct

| [] -> tycon.AllInstanceFieldsAsList |> List.forall (fun r -> isUnmanagedTy g r.rfield_type)
| typars ->
// Handle generic structs
// REVIEW: This may not be the most optimal, but it's probably better than having to iterate over all type arguments for every field.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the comment, the code is fine perf wise

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. For what it was worth, I did do some perf testing on it. I think the number was 15ms if it was called 10 thousand times, which the only way for that to happen is to have 10 thousand written calls that test the unmanaged constraint.

// Handle generic structs
// REVIEW: This may not be the most optimal, but it's probably better than having to iterate over all type arguments for every field.
// However, what we have currently is probably just fine as unmanaged constraints are used infrequently; even more so when combined with generic struct construction.
let lookup = Dictionary(typars.Length)
Copy link
Contributor

Choose a reason for hiding this comment

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

The code is a little odd. Normally we would not enforce the transitive constraints like this - we would require that each type parameter itself has the unmanaged constraint, e.g. consider this:

    [<Struct>]
    type C1<'T when 'T : unmanaged> = 
        ...

    [<Struct>]
    type C2<'U when 'U : unmanaged> = 
        ... field C1<'U> ....

Then when checking whether C2<int> is unmanaged we would check whether the instantiated field type C1<int> is unmanaged. It looks like you're re-implementing substitution here.

What if you use simply this:

            elif tycon.IsStructOrEnumTycon then
                let tinst = mkInstForAppTy g ty
                tycon.AllInstanceFieldsAsList |> List.forall (fun r -> isUnmanagedTy g (actualTyOfRecdField tinst r))

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that actualTyOfRecdField gets the instantiated field type under a governing instantiation derived from ty

Copy link
Member Author

Choose a reason for hiding this comment

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

actualTyOfRecdField is exactly what I need and will simplify the existing code; I was reimplementing substitution. I had a hard time trying to find such a function before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to consider how to make it easier to find the things in TastOps.fs. Making them all instance extension methods on the various primary types would be one option.

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

Looks like the added code could be simpler

@TIHan
Copy link
Member Author

TIHan commented May 24, 2019

Not much progress on this yet. Still need to create a RFC. I don't see much progress happening soon as it's not a priority, unless someone is truly blocked by not having this feature.

@zpodlovics
Copy link

Native interop and representation is a long awaited[1] and critical functionality especially for [2]. It may seems a low hanging fruit, however performance (and correctness) is a feature now, as the cloud/on-prem cost will be roughly proportional to the performance.

[1] fsharp/fslang-suggestions#493
[2] dotnet/spark#41 (comment)

@zpodlovics
Copy link

Any update on this? Performance (and correctness) is a must have feature now, as the cloud/on-prem cost will be roughly proportional to the performance.

@cartermp cartermp changed the base branch from dev16.0 to master February 20, 2020 16:43
@cartermp
Copy link
Contributor

This isn't a priority for us at the moment, but we're happy to accept contributions from interested parties.

@cartermp cartermp assigned vzarytovskii and unassigned TIHan Jun 27, 2020
@vzarytovskii
Copy link
Member

I’ve added an RFC for it: fsharp/fslang-design#480

@brettfo brettfo changed the base branch from master to main August 19, 2020 20:05
@cartermp cartermp removed this from the Backlog milestone Sep 16, 2020
@vzarytovskii
Copy link
Member

Closing this one, gonna open new one on top of latest main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants