-
Notifications
You must be signed in to change notification settings - Fork 792
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
Conversation
match tryDestAppTy g ty with | ||
| ValueSome tcref -> | ||
match ty with | ||
| TType_app(tcref, tinst) -> |
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
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. |
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 |
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. |
This isn't a priority for us at the moment, but we're happy to accept contributions from interested parties. |
I’ve added an RFC for it: fsharp/fslang-design#480 |
Closing this one, gonna open new one on top of latest main. |
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:
Another example:
Example on nested generic structs:
Remaining work:
IsUnmanagedAttribute
on type arguments with the 'unmanaged' constraint for interop.IsUnmanagedAttribute
to mean they have theunmanaged
constraint. This is also for interop.Regarding the struct tuples, this works:
But this doesn't:
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.