Struct records

[TIHan]

I hope I'm doing this correctly. I'm open to change anything in the PR, and I'm open to toss it if it is done incorrectly.

Here is the original user voice post. http://fslang.uservoice.com/forums/245727-f-language/suggestions/6547517-record-types-can-be-marked-with-the-struct-attribu

The idea: Record types should be able to be marked as a struct, effectively making the record have the semantics of value types.

How to use:

type Vector3 = { X: float; Y: float; Z: float }

Put the StructAttribute on the type.

[<Struct>]
type Vector3 = { X: float; Y: float; Z: float }

Key differences in struct records:

• Any fields marked as mutable can't be mutated if the corresponding struct instance itself is not marked as mutable. This is how structs work in general for F#.
• You cannot have cyclic references to the same type being defined. ex: type T = { X: T }
• You also cannot call the default ctor, like you could with normal F# structs.
• When marked with the CLIMutableAttribute, it will not create a default ctor, because structs implicitly have one, though you can't call it from F#.

I have provided typecheck tests for most if not all these cases, inlining tests, and basic unit tests for checking equality.

From the code changes themselves, I am most worried about what I am doing in TastPickle.fs. At the moment I can't think of a better way unless someone tells me its fine, it could be cleaned up, or could be done differently.

Anyway, this feature has been long overdue, and I'm happy that it is finally here. If this works out, I'll try to tackle single case union structs as it will be roughly similar to how struct records are accomplished.

[vasily-kirichenko]

This is fantastic! And

If this works out, I'll try to tackle single case union structs as it will be roughly similar to how struct records are accomplished

equally interesting.

[krauthaufen]

Would be awesome to See this feature since it would simplify unmanaged interop a lot!
Will the record fields be allowed to have attributes like MarshalAs, FieldOffset, etc. and will it be able to use the StructLayoutAttribute on them?

[vasily-kirichenko]

@krauthaufen see the tests in this PR in tests/fsharp/optimize/inline/lib.fs

[TIHan]

@krauthaufen you could always do this with records even if they were not value types. Hmm, makes me think record types could have always worked with interop.

[dsyme]

@krauthaufen Great question re interop. Please continue to list possible feature interactions like this!

Historically unassessed feature interactions when adding new features have been the biggest source of bugs in F#. I'm hopeful that having multiple people thinking about this in an open source context will help us avoid a bug trail.

p.s. One C# user found many bugs in the production compiler simply by looking through the C# language spec for feature interactions and thinking "what mistakes would I make if I implemented this". :)

[dsyme]

@TIHan Instead of changing the TAST and the pickle format, you should add a new flag to EntityFlags https://github.com/TIHan/visualfsharp/blob/struct_records/src/fsharp/tast.fs#L351 using the existing space in the metadata format. A value of 0 would indicate a class record, a value of 1 a struct record.

Thanks
don

[TIHan]

@dsyme ok! I will do that. :) It should make the implementation simpler. I will also write interop tests.

@krauthaufen thank you for bringing interop up. I think that is something that could have been overlooked on my part.

[panesofglass]

💯

[latkin]

Cool, great work!

On the syntax side, struct types can be defined today with either [<Struct>] or type Foo = struct ... end syntax. Is there interest/proposal for an equivalent of the latter for records?

Some interactions/tests that should be included/considered:

• Top consideration: Expose a struct record in a down-targeted library, and consume it from an older compiler -- what happens? Will the 3.1 compiler see that the type is a record and start making incorrect assumptions that it's a ref type? To avoid this, maybe you can only define a struct record if you are targeting 4.0-era runtime?
• Create some codegen tests
• We can prevent F# users from doing a direct initobj, but zero-init struct instances will still exist at runtime due to Unchecked.defaultOf<_>, Array.zeroCreate, C# interop, etc. The auto-generated equality/hash/etc need to handle this gracefully and sensibly, without nullrefing.
• Generally speaking I worry about various bits of the compiler making the assumption that record type => reference type, or that struct type => not record type. Besides lots of testing and grepping for comments along these lines, I don't know if there is a good solution to that.
• Does this interact with [<CustomEquality>] / [<CustomComparison>]?
• The pretty-printer/IDE tools indicate ref type/struct type in tooltips, etc. Need to make sure that works here, too.

That's what I can think of for now, will chime in w/ more if I think of anything.

Within reason, we should review all tests of the form "verify property X specific to struct types" or "verify property Y specific to record types" and add a variation for struct records.

[dsyme]

On the syntax side, struct types can be defined today with either [] or type Foo = struct ... end syntax. Is there interest/proposal for an equivalent of the latter for records?

No, we don't need this. [<Struct>] should be considered the normal way of defining these types. The attribute form was added partly in the expectation that structs and unions could one day be annotated as structs.

Top consideration: Expose a struct record in a down-targeted library, and consume it from an older compiler -- what happens? Will the 3.1 compiler see that the type is a record and start making incorrect assumptions that it's a ref type? To avoid this, maybe you can only define a struct record if you are targeting 4.0-era runtime?

Good point. I presume it's unrealistic to add this feature to F# 4.0 OOB1 for this reason. Maybe we could add a warning or error to the OOB1 of Visual F# Tools 4.0 if it encounters a type definition with the top bit set?

However I think that would be an unfortunate limitation to require FSharp.Core 4.x.x.x since the feature has nothing to do with FSharp.Core and there are perfectly good reasons to use it when targeting 4.3.1.0, 4.4.0.0 etc.

The auto-generated equality/hash/etc need to handle this gracefully and sensibly, without nullrefing.

We already auto-generate hash/equality for structs and I'd imagine that using defaultof does already give nullrefs. I don't think it's unexpected. e.g.

type S1(x:int list) = 
   struct end

type S2(x:S1) = 
   struct end

Unchecked.defaultof<S2> = Unchecked.defaultof<S2>

System.NullReferenceException: Object reference not set to an instance of an object.

I'm ok with this behaviour but it should be pinned down by tests.

Does this interact with [] / [] ?

Good one. Add lots of tests for all the combinations here.

Within reason, we should review all tests of the form "verify property X specific to struct types" or "verify property Y specific to record types" and add a variation for struct records.

Yes!

[latkin]

How does this interact with the [<DefaultValue>] requirement on regular structs? I have always been fuzzy on that guy. Is it not relevant here because records only have a single ctor?

[dsyme]

Yes, the interaction with DefaultValue needs to be pinned down. You can use DefaultValue with record fields (though I would not encourage this - using this is dangerous and can be an inadvertent source of nulls):

type R = { [<DefaultValue>] x : C; y : int }
{ y = 1 } 

[dsyme]

As an aside, while looking at the code for the DefaultValue attribute I noticed an improvement we can make: http://fslang.uservoice.com/forums/245727-f-language/suggestions/9679206-defaultvalue-attribute-should-require-mutable-wh

[dsyme]

@TIHan I made some progress on the errors. I think FSharp.Core.Unittests is now failing for the CoreCLR build due to missing APIs. Here are the failures:

         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(226,23): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(243,23): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(270,14): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(271,14): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(279,14): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(280,14): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(299,14): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(300,14): error FS0044: This construct is deprecated. OffsetOf(Type, string) may be unavailable in future releases. Instead, use OffsetOf<T>(string). For more info, go to http://go.microsoft.com/fwlink/?LinkID=296511 [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(308,43): error FS0039: The field, constructor or member 'IsValueType' is not defined [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(308,5): error FS0041: A unique overload for method 'IsTrue' could not be determined based on type information prior to this program point. A type annotation may be needed. Candidates: Assert.IsTrue(condition: Nullable<bool>) : unit, Assert.IsTrue(condition: bool) : unit [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]

We could just disable some of these tests for CoreCLR I suppose, though it would be good to have the coverage.

[dsyme]

@TIHan OK, just pushed another commit which should fix those I think

[dsyme]

Ok, this is all green now, I'm happy to see this merged

[TIHan]

@dsyme Thank you so much and thank you for fixing the build! I'm glad to see this is all good. I apologize for not getting to it this weekend; didn't mean to add work to your plate.

[isaacabraham]

I know that this is really late in the day but here's a question nonetheless. Am I correct that for records we'll use an attribute to mark it as a struct? Would it not make sense to use the struct keyword instead for consistencies' sake with the upcoming struct tuples?

[TIHan]

@isaacabraham Yes, you will use the [<Struct>] attribute to make it a struct. I think part of the reason the attribute exists was in hope for having records and DU's to be able to be structs. I'm open minded to using a struct keyword, but it absolutely must work using the [<Struct>] attribute anyway. I prefer only the [<Struct>] attribute as I enjoy one way of doing things on that front. :)

[vasily-kirichenko]

@isaacabraham you mean this:

type struct Foo { }

or this:

struct Foo { }

?
I understand that F# tries to use minimum number of keywords, but it does use private and internal for example, so, considering how often structs are used, I vote for struct keyword as well.

[forki]

that keyword would need to set the attribute, right? so this PR is needed as basis, right?
Let's get this in and then discuss in RFC. I mean in the meantime we can already use this one using attributes inside the compiler itself!?

[isaacabraham]

I've absolutely zero understanding of what is required in the internals of this i.e. if you need an attribute or not. I just wanted to mention this before this feature was merged just in case it's something that would need changing before merging.

I'm really just thinking from a consumer point of view i.e. F# developer looking to create a struct record. @vasily-kirichenko I was thinking of your first example, with struct acting as a modifier of sorts (albeit with tuples it's at the point of value creation, here it's at the point of type definition).

I just think having a keyword would be more consistent, that's all. Maybe this should go back into the RFC as @forki suggests.

[isaacabraham]

@TIHan would you expect there to be the ability to add [<Struct>] on the definition of tuples as well e.g.

[<Struct>]
type Name = string * string

[dsyme]

@isaacabraham Yes, the attribute is needed, and @TIHan is correct that this is one reason it's always been there. Using the attribute is the preferred way if indicating structness for nominal type definitions.

There's also a basic principle here that F# only uses "let", "module" and "type" as top level declarations (and "exception" and "extern" but they are rarely used). We don't use new keyword-led declarations for the panoply of different kinds of nominal types.

Unlike tuples, for record types the structness is an aspect of the nominal type definition, and we've long preferred attributes for those.

For struct tuples, the structness is part of the syntax of anonymous types, not nominal type definitions

We wouldn't expect the example above to work. You can't put a struct attribute on a type abbreviation.

[isaacabraham]

@dsyme Makes sense. I was thinking along the lines of @vasily-kirichenko 's idea - using struct as a modifier on type e.g. type struct Foo { }

[dsyme]

@isaacabraham Among other things, type class C() = ... might confuse some Haskell people :) Likewise and type interface IA = ... :)

[forki]

Ah so you are reserving the syntax for type classes. Smart move ;-)
On Jun 3, 2016 14:50, "Don Syme" notifications@github.com wrote:

@isaacabraham https://github.com/isaacabraham Among other things, type
class C() = ... might confuse some Haskell people :) Likewise and type
interface IA = ... :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#620 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNLGg0WTn0Ray4FJQW3VTA7VMTYBDks5qICL4gaJpZM4F5LhS
.

[dsyme]

@forki The funny thing is that type interface ... (or type constraint depending on if it's structural) would probably be the more logically natural syntax to describe expected functionality on a type.

[isaacabraham]

@dsyme hmmm :-) I was expecting everything else to stay as is i.e. interface and class are not needed. Only for struct would there have been the extra keyword.

[forki]

Gratulations! Thanks everyone. ❤️

[vasily-kirichenko]

Thank you guys!

[panesofglass]

💯‼️🎆🏆💃🖖👏

[cartermp]

👍 This is excellent.

[brodyberg]

A fantastic example to the community. Thanks everyone.

On Sat, Jun 4, 2016 at 5:34 PM, Phillip Carter notifications@github.com
wrote:

👍 This is excellent.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#620 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAJRk4Z1KOopKG8o0dj-iWNKba-L1dNXks5qIhmmgaJpZM4F5LhS
.