Skip to content

[WIP] Unmanaged constructed types #6064

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[WIP] Unmanaged constructed types #6064

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ead4ea3
Allow generic structs to possibly be unmanaged at construction
TIHan Jan 6, 2019
5712fd9
Fixed nested generic structs for unmanaged
TIHan Jan 6, 2019
a8935b4
Updated comment
TIHan Jan 6, 2019
e24bc71
Changed nested generic struct condition slightly
TIHan Jan 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 44 additions & 10 deletions src/fsharp/TastOps.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1776,19 +1776,17 @@ let isRefTy g ty =
isUnitTy g ty
)

// ECMA C# LANGUAGE SPECIFICATION, 27.2
// An unmanaged-type is any type that isn't a reference-type, a type-parameter, or a generic struct-type and
// An unmanaged-type is any type that isn't a reference-type and
// contains no fields whose type is not an unmanaged-type. In other words, an unmanaged-type is one of the
// following:
// - sbyte, byte, short, ushort, int, uint, long, ulong, char, float, double, decimal, or bool.
// - Any enum-type.
// - Any pointer-type.
// - Any non-generic user-defined struct-type that contains fields of unmanaged-types only.
// [Note: Constructed types and type-parameters are never unmanaged-types. end note]
// - Any generic user-defined struct-type that can be statically determined to be 'unmanaged' at construction.
let rec isUnmanagedTy g ty =
let ty = stripTyEqnsAndMeasureEqns g ty
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
Contributor 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

let isEq tcref2 = tyconRefEq g tcref tcref2
if isEq g.nativeptr_tcr || isEq g.nativeint_tcr ||
isEq g.sbyte_tcr || isEq g.byte_tcr ||
Expand All @@ -1807,11 +1805,47 @@ let rec isUnmanagedTy g ty =
true
elif tycon.IsStructOrEnumTycon then
match tycon.TyparsNoRange with
| [] -> tycon.AllInstanceFieldsAsList |> List.forall (fun r -> isUnmanagedTy g r.rfield_type)
| _ -> false // generic structs are never
| [] -> 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
Contributor 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.

// 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
Contributor 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.

(typars, tinst)
||> List.iter2 (fun typar ty -> lookup.Add(typar.Stamp, ty))

tycon.AllInstanceFieldsAsList
|> List.forall (fun r ->
let fieldTy = stripTyEqnsAndMeasureEqns g r.rfield_type
match fieldTy with
| TType_var fieldTypar ->
match lookup.TryGetValue(fieldTypar.Stamp) with
| true, ty ->
match tryDestTyparTy g ty with
| ValueSome typar ->
typar.Constraints |> List.exists (function | TyparConstraint.IsUnmanaged _ -> true | _ -> false)
| _ -> isUnmanagedTy g ty
| _ -> false
| TType_app(fieldTcref, fieldTinst) when fieldTinst.Length > 0 ->
// This will handle nested generic structs
let fieldTinst =
fieldTinst
|> List.map (fun ty ->
match tryDestTyparTy g ty with
| ValueSome(typar) ->
match lookup.TryGetValue(typar.Stamp) with
| true, ty -> ty
| _ -> ty
| _ -> ty
)
isUnmanagedTy g (mkAppTy fieldTcref fieldTinst)
| _ -> isUnmanagedTy g fieldTy
)
else false
| ValueNone ->
false
// Handle struct tuples
| TType_tuple(TupInfo.Const true, tinst) ->
tinst |> List.forall (isUnmanagedTy g)
| _ -> false

let isInterfaceTycon x =
isILInterfaceTycon x || x.IsFSharpInterfaceTycon
Expand Down