Struct records #620

Merged
merged 68 commits into from Jun 4, 2016

Conversation

Projects
None yet
@TIHan
Contributor

TIHan commented Sep 8, 2015

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.

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Sep 6, 2015

Owner

This was an accident. It should be Auto.

This was an accident. It should be Auto.

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Sep 8, 2015

Contributor

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.

Contributor

vasily-kirichenko commented Sep 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@krauthaufen

krauthaufen Sep 8, 2015

Contributor

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?

Contributor

krauthaufen commented Sep 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Sep 8, 2015

Contributor

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

Contributor

vasily-kirichenko commented Sep 8, 2015

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

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Sep 8, 2015

Contributor

@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.

Contributor

TIHan commented Sep 8, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 8, 2015

Contributor

@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". :)

Contributor

dsyme commented Sep 8, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 8, 2015

Contributor

@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

Contributor

dsyme commented Sep 8, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Sep 8, 2015

Contributor

@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.

Contributor

TIHan commented Sep 8, 2015

@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

This comment has been minimized.

Show comment
Hide comment

💯

@latkin

This comment has been minimized.

Show comment
Hide comment
@latkin

latkin Sep 8, 2015

Contributor

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.

Contributor

latkin commented Sep 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 8, 2015

Contributor

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!

Contributor

dsyme commented Sep 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@latkin

latkin Sep 8, 2015

Contributor

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?

Contributor

latkin commented Sep 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 8, 2015

Contributor

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 } 
Contributor

dsyme commented Sep 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 8, 2015

Contributor

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

Contributor

dsyme commented Sep 8, 2015

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

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Sep 8, 2015

Contributor

@dsyme @latkin - Thank you very much for your feedback. I will act accordingly.

My plan so far:

  • Remove changes to tast and pickle - use EntityFlags to replace this behavior.
  • Provide codegen tests.
  • Provide tests against the usage of the following attributes: DefaultValue, CustomComparison, CustomEquality, NoComparison, NoEquality, StructLayout, FieldOffset, MarshalAs. I will try to make sure we test the proper behavior with the record and struct combination in comparison to class records and normal structs.
  • Provide interop tests, may go along with attributes, StructLayout, FieldOffset, MarshalAs.
  • Provide tests for Unchecked.defaultof<_>

The pretty-printer/IDE tools indicate ref type/struct type in tooltips, etc. Need to make sure that works here, too.

I managed to compile the tools, and I was able to get the same kind of tooltips as class records.

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.

There were places where the compiler assumed that record types are reference types. I think I fixed all those places, namely isRefTy. But there could be more, I guess having a ton of tests will give us all confidence.

I agree that this should not be part of the OOB1 release. In fact, this feature should probably be included into a release that also has single case union structs. This isn't an urgent feature, but one that I know some people been wanting for a while. But, there a bigger priorities than this, and I understand that.

Let me know if I am missing anything.

Thanks,
Will

Contributor

TIHan commented Sep 8, 2015

@dsyme @latkin - Thank you very much for your feedback. I will act accordingly.

My plan so far:

  • Remove changes to tast and pickle - use EntityFlags to replace this behavior.
  • Provide codegen tests.
  • Provide tests against the usage of the following attributes: DefaultValue, CustomComparison, CustomEquality, NoComparison, NoEquality, StructLayout, FieldOffset, MarshalAs. I will try to make sure we test the proper behavior with the record and struct combination in comparison to class records and normal structs.
  • Provide interop tests, may go along with attributes, StructLayout, FieldOffset, MarshalAs.
  • Provide tests for Unchecked.defaultof<_>

The pretty-printer/IDE tools indicate ref type/struct type in tooltips, etc. Need to make sure that works here, too.

I managed to compile the tools, and I was able to get the same kind of tooltips as class records.

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.

There were places where the compiler assumed that record types are reference types. I think I fixed all those places, namely isRefTy. But there could be more, I guess having a ton of tests will give us all confidence.

I agree that this should not be part of the OOB1 release. In fact, this feature should probably be included into a release that also has single case union structs. This isn't an urgent feature, but one that I know some people been wanting for a while. But, there a bigger priorities than this, and I understand that.

Let me know if I am missing anything.

Thanks,
Will

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 9, 2015

Contributor

In fact, this feature should probably be included into a release that also has single case union structs.

Well, that would be awesome :)

I marked both the "records as structs" and "single case unions as structs" proposals as approved, see http://fslang.uservoice.com/forums/245727-f-language/suggestions/6147144-allow-single-case-unions-to-be-compiled-as-structs and http://fslang.uservoice.com/forums/245727-f-language/suggestions/6547517-record-types-can-be-marked-with-the-struct-attribu.

For unions, ideally it should also be possible to mark multi-case unions as structs, with a field-flattened+integer-tagged representation being used.

Contributor

dsyme commented Sep 9, 2015

In fact, this feature should probably be included into a release that also has single case union structs.

Well, that would be awesome :)

I marked both the "records as structs" and "single case unions as structs" proposals as approved, see http://fslang.uservoice.com/forums/245727-f-language/suggestions/6147144-allow-single-case-unions-to-be-compiled-as-structs and http://fslang.uservoice.com/forums/245727-f-language/suggestions/6547517-record-types-can-be-marked-with-the-struct-attribu.

For unions, ideally it should also be possible to mark multi-case unions as structs, with a field-flattened+integer-tagged representation being used.

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Sep 9, 2015

Contributor

I marked both the "records as structs" and "single case unions as structs" proposals as approved

Yay! Cool!

For unions, ideally it should also be possible to mark multi-case unions as structs, with a field-flattened+integer-tagged representation being used.

I didn't think we could do that! : O - I know right now multi-case uses abstract classes which contains some of the equality/comparison stuff. How would we approach this if they could be structs?

Contributor

TIHan commented Sep 9, 2015

I marked both the "records as structs" and "single case unions as structs" proposals as approved

Yay! Cool!

For unions, ideally it should also be possible to mark multi-case unions as structs, with a field-flattened+integer-tagged representation being used.

I didn't think we could do that! : O - I know right now multi-case uses abstract classes which contains some of the equality/comparison stuff. How would we approach this if they could be structs?

@latkin

This comment has been minimized.

Show comment
Hide comment
@latkin

latkin Sep 9, 2015

Contributor

@TIHan I think the suggestion is to flatten into a single concrete struct type, instead of an abstract base + concrete subtypes (current impl). Something along the lines of

type Foo = 
    | Bar of int
    | Baz of string * int

--->

struct Foo
{
    int Tag;

    int Bar_Item1;
    string Baz_Item1;
    int Baz_Item2;

    bool Equals(Foo other)
    {
       if(other.Tag != this.Tag) return false;
       if(this.Tag = 0)
            return this.Bar_Item1 == other.Bar_Item1;
       else if(this.Tag == 1)
            return this.Baz_Item1 == other.Baz_Item1 && this.Baz_Item2 == other.Baz_Item2;
    }
    ...
}

Though that means all fields are carried around by all instances, which would reduce perf benefits due to additional copying required. Better than no value-type option, though?

Contributor

latkin commented Sep 9, 2015

@TIHan I think the suggestion is to flatten into a single concrete struct type, instead of an abstract base + concrete subtypes (current impl). Something along the lines of

type Foo = 
    | Bar of int
    | Baz of string * int

--->

struct Foo
{
    int Tag;

    int Bar_Item1;
    string Baz_Item1;
    int Baz_Item2;

    bool Equals(Foo other)
    {
       if(other.Tag != this.Tag) return false;
       if(this.Tag = 0)
            return this.Bar_Item1 == other.Bar_Item1;
       else if(this.Tag == 1)
            return this.Baz_Item1 == other.Baz_Item1 && this.Baz_Item2 == other.Baz_Item2;
    }
    ...
}

Though that means all fields are carried around by all instances, which would reduce perf benefits due to additional copying required. Better than no value-type option, though?

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Sep 9, 2015

Contributor

Though that means all fields are carried around by all instances, which would reduce perf benefits due to additional copying required. Better than no value-type option, though?

Yea, I can imagine the perf starting to drop off. You're right that it's probably better than nothing. Interfaces wouldn't help as you would have boxing, so this approach seems fair. I guess the dev would need to use it wisely.

Contributor

TIHan commented Sep 9, 2015

Though that means all fields are carried around by all instances, which would reduce perf benefits due to additional copying required. Better than no value-type option, though?

Yea, I can imagine the perf starting to drop off. You're right that it's probably better than nothing. Interfaces wouldn't help as you would have boxing, so this approach seems fair. I guess the dev would need to use it wisely.

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Sep 9, 2015

Contributor

That is how enums implemented in Rust.

Contributor

vasily-kirichenko commented Sep 9, 2015

That is how enums implemented in Rust.

@latkin

This comment has been minimized.

Show comment
Hide comment
@latkin

latkin Sep 9, 2015

Contributor

@TIHan and in particular you lose the benefits of the current static singleton instance for DU cases with no fields. All instances will be unique and they will all carry around every field!

Contributor

latkin commented Sep 9, 2015

@TIHan and in particular you lose the benefits of the current static singleton instance for DU cases with no fields. All instances will be unique and they will all carry around every field!

@dsyme dsyme closed this May 9, 2016

@dsyme dsyme reopened this May 9, 2016

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan May 26, 2016

Contributor

Building tests seem to fail. I'll try to look at all of this again this weekend. If I can get everything passing, I deem the PR complete.

Contributor

TIHan commented May 26, 2016

Building tests seem to fail. I'll try to look at all of this again this weekend. If I can get everything passing, I deem the PR complete.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme May 27, 2016

Contributor

I think the problem is building the FSharp.Core.Unittests on .NET Core (or possible a portable profile - it's hard to tell which)

         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(225,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(242,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(269,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(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(278,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(298,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(305,64): error FS0039: The field, constructor or member 'MakeRecord' 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(307,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(307,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]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(309,58): error FS0039: The field, constructor or member 'GetRecordFields' 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(311,14): error FS0752: The operator 'expr.[idx]' has been used on an object of indeterminate type based on information prior to this program point. Consider adding further type constraints [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(311,14): error FS0008: This runtime coercion or type test from type�    'a    � to �    int    �involves an indeterminate type based on information prior to this program point. Runtime type tests are not allowed on some types. Further type annotations are needed. [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(314,14): error FS0752: The operator 'expr.[idx]' has been used on an object of indeterminate type based on information prior to this program point. Consider adding further type constraints [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(314,14): error FS0008: This runtime coercion or type test from type�    'a    � to �    int    �involves an indeterminate type based on information prior to this program point. Runtime type tests are not allowed on some types. Further type annotations are needed. [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
Contributor

dsyme commented May 27, 2016

I think the problem is building the FSharp.Core.Unittests on .NET Core (or possible a portable profile - it's hard to tell which)

         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(225,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(242,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(269,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(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(278,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(298,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(305,64): error FS0039: The field, constructor or member 'MakeRecord' 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(307,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(307,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]
         D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core\RecordTypes.fs(309,58): error FS0039: The field, constructor or member 'GetRecordFields' 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(311,14): error FS0752: The operator 'expr.[idx]' has been used on an object of indeterminate type based on information prior to this program point. Consider adding further type constraints [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(311,14): error FS0008: This runtime coercion or type test from type�    'a    � to �    int    �involves an indeterminate type based on information prior to this program point. Runtime type tests are not allowed on some types. Further type annotations are needed. [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(314,14): error FS0752: The operator 'expr.[idx]' has been used on an object of indeterminate type based on information prior to this program point. Consider adding further type constraints [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(314,14): error FS0008: This runtime coercion or type test from type�    'a    � to �    int    �involves an indeterminate type based on information prior to this program point. Runtime type tests are not allowed on some types. Further type annotations are needed. [D:\j\workspace\release_ci_pa---866fd2c3\src\fsharp\FSharp.Core.Unittests\FSharp.Core.Unittests.fsproj]
@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme May 27, 2016

Contributor

See https://github.com/fsharp/FSharpLangDesign/blob/master/RFCs/FS-1013-enable-reflection-functionality-on-portable-profiles.md

@TIHan For now I suppose you should make these new tests conditional on

#if !FX_RESHAPED_REFLECTION
   ...
#endif

We really need to get this reflection functionality sorted out on .NET Core.

Contributor

dsyme commented May 27, 2016

See https://github.com/fsharp/FSharpLangDesign/blob/master/RFCs/FS-1013-enable-reflection-functionality-on-portable-profiles.md

@TIHan For now I suppose you should make these new tests conditional on

#if !FX_RESHAPED_REFLECTION
   ...
#endif

We really need to get this reflection functionality sorted out on .NET Core.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme May 27, 2016

Contributor

@TIHan Actually I think all you need to do is

open FSharp.Reflection.FSharpReflectionExtensions 

It looks like the necessary extension methods are already available.

Contributor

dsyme commented May 27, 2016

@TIHan Actually I think all you need to do is

open FSharp.Reflection.FSharpReflectionExtensions 

It looks like the necessary extension methods are already available.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 2, 2016

Contributor

@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.

Contributor

dsyme commented Jun 2, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 2, 2016

Contributor

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

Contributor

dsyme commented Jun 2, 2016

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

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 2, 2016

Contributor

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

Contributor

dsyme commented Jun 2, 2016

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

@TIHan

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Jun 2, 2016

Contributor

@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.

Contributor

TIHan commented Jun 2, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Jun 3, 2016

Contributor

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?

Contributor

isaacabraham commented Jun 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@TIHan

TIHan Jun 3, 2016

Contributor

@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. :)

Contributor

TIHan commented Jun 3, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jun 3, 2016

Contributor

@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.

Contributor

vasily-kirichenko commented Jun 3, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 3, 2016

Contributor

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!?

Contributor

forki commented Jun 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Jun 3, 2016

Contributor

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.

Contributor

isaacabraham commented Jun 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Jun 3, 2016

Contributor

@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
Contributor

isaacabraham commented Jun 3, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 3, 2016

Contributor

@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.

Contributor

dsyme commented Jun 3, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Jun 3, 2016

Contributor

@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 { }

Contributor

isaacabraham commented Jun 3, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 3, 2016

Contributor

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

Contributor

dsyme commented Jun 3, 2016

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

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 3, 2016

Contributor

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
.

Contributor

forki commented Jun 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 3, 2016

Contributor

@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.

Contributor

dsyme commented Jun 3, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Jun 3, 2016

Contributor

@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.

Contributor

isaacabraham commented Jun 3, 2016

@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.

@KevinRansom KevinRansom merged commit eb54394 into Microsoft:master Jun 4, 2016

4 checks passed

Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 4, 2016

Contributor

Gratulations! Thanks everyone. ❤️

Contributor

forki commented Jun 4, 2016

Gratulations! Thanks everyone. ❤️

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jun 4, 2016

Contributor

Thank you guys!

Contributor

vasily-kirichenko commented Jun 4, 2016

Thank you guys!

@panesofglass

This comment has been minimized.

Show comment
Hide comment
@panesofglass

panesofglass Jun 4, 2016

💯‼️🎆🏆💃🖖👏

💯‼️🎆🏆💃🖖👏

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Jun 5, 2016

Contributor

👍 This is excellent.

Contributor

cartermp commented Jun 5, 2016

👍 This is excellent.

@brodyberg

This comment has been minimized.

Show comment
Hide comment
@brodyberg

brodyberg Jun 9, 2016

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
.

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
.

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