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

Breaking change in tuples after VS 15.4 update #3729

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
@forki
Contributor

forki commented Oct 11, 2017

With VS Update 15.3.5 we get:

image

With VS Update 15.4 we see:

image

/cc @mexx @drvink

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Oct 11, 2017

Contributor

OK, so this is a regression. We fixed an issue to translate Tuple<int,int> into F#'s view of tuple types int * int. However the F# view of tuple types doesn't support .Item1 etc. properties.

I'm not sure what to do about this, we will need to think about it.

Contributor

dsyme commented Oct 11, 2017

OK, so this is a regression. We fixed an issue to translate Tuple<int,int> into F#'s view of tuple types int * int. However the F# view of tuple types doesn't support .Item1 etc. properties.

I'm not sure what to do about this, we will need to think about it.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 11, 2017

Contributor

Tbh I think it should compile in any case since we only use system.tuple everywhere

Contributor

forki commented Oct 11, 2017

Tbh I think it should compile in any case since we only use system.tuple everywhere

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Oct 11, 2017

Contributor

I think whenever the type System.Tuple is specified explicitly the compiler would allow ItemX otherwise it probably shouldn't?

Besides that System.Tuple and * should behave exactly the same (and we should be able to change one to the other without semantic changes - with the exception of ItemX).

The easier solution would be to allow ItemX everywhere, but that might have other side-effects.

Contributor

matthid commented Oct 11, 2017

I think whenever the type System.Tuple is specified explicitly the compiler would allow ItemX otherwise it probably shouldn't?

Besides that System.Tuple and * should behave exactly the same (and we should be able to change one to the other without semantic changes - with the exception of ItemX).

The easier solution would be to allow ItemX everywhere, but that might have other side-effects.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 11, 2017

Contributor

Workaround:

image

Contributor

forki commented Oct 11, 2017

Workaround:

image

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 11, 2017

Contributor

ok this fails in ci_part1 for unrelated reason. I guess master is broken

Contributor

forki commented Oct 11, 2017

ok this fails in ci_part1 for unrelated reason. I guess master is broken

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Oct 11, 2017

Contributor

@forki Yes, there's something fishy with the latest commit "Merge branch 'vs2017-rtm' into master".

Contributor

dsyme commented Oct 11, 2017

@forki Yes, there's something fishy with the latest commit "Merge branch 'vs2017-rtm' into master".

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Oct 11, 2017

Contributor

I think whenever the type System.Tuple is specified explicitly the compiler would allow ItemX otherwise it probably shouldn't? Besides that System.Tuple and * should behave exactly the same (and we should be able to change one to the other without semantic changes - with the exception of ItemX). The easier solution would be to allow ItemX everywhere, but that might have other side-effects.

See #3016, #3283

Contributor

dsyme commented Oct 11, 2017

I think whenever the type System.Tuple is specified explicitly the compiler would allow ItemX otherwise it probably shouldn't? Besides that System.Tuple and * should behave exactly the same (and we should be able to change one to the other without semantic changes - with the exception of ItemX). The easier solution would be to allow ItemX everywhere, but that might have other side-effects.

See #3016, #3283

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Oct 11, 2017

Contributor

See #3016, #3283

Ok with that in mind another solution would in fact be to not allow users to use System.Tuple by either emitting a warning or even an error (because apparently it is hard to write code when you have that anywhere)

Contributor

matthid commented Oct 11, 2017

See #3016, #3283

Ok with that in mind another solution would in fact be to not allow users to use System.Tuple by either emitting a warning or even an error (because apparently it is hard to write code when you have that anywhere)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 11, 2017

Contributor

@matthid this code comes from C# interop in real life app. We broke that.

one could argue that's bad design to use tuples in public APIs, but this is still a breaking change

Contributor

forki commented Oct 11, 2017

@matthid this code comes from C# interop in real life app. We broke that.

one could argue that's bad design to use tuples in public APIs, but this is still a breaking change

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Oct 11, 2017

Contributor

@forki Yes non-breaking is only my suggestion above. Problem is as far as I can understand the point of @dsyme is to strictly separate language-tuples from System.Tuple. That they are in fact the same is an implementation detail. This is a hard decision to make with what is already released.

Maybe the compiler needs to be less eager in converting to F# tuples as it is right now?

Contributor

matthid commented Oct 11, 2017

@forki Yes non-breaking is only my suggestion above. Problem is as far as I can understand the point of @dsyme is to strictly separate language-tuples from System.Tuple. That they are in fact the same is an implementation detail. This is a hard decision to make with what is already released.

Maybe the compiler needs to be less eager in converting to F# tuples as it is right now?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 12, 2017

Contributor

ok so the test is capturing the bug.

Contributor

forki commented Oct 12, 2017

ok so the test is capturing the bug.

@cartermp cartermp added this to the VS 2017 Updates milestone Oct 12, 2017

@curtnichols

This comment has been minimized.

Show comment
Hide comment
@curtnichols

curtnichols Oct 13, 2017

Found the same issue in our code base, C# calling an F# function that explicitly specifies System.Tuple with types. Repro:

[<CompiledName("F")>]
let f (p : System.Tuple<bool, int>) =
    printfn "%A %A" p.Item1 p.Item2

I'm finding it rather troublesome that one can specify a concrete type and the compiler supersedes the request.

curtnichols commented Oct 13, 2017

Found the same issue in our code base, C# calling an F# function that explicitly specifies System.Tuple with types. Repro:

[<CompiledName("F")>]
let f (p : System.Tuple<bool, int>) =
    printfn "%A %A" p.Item1 p.Item2

I'm finding it rather troublesome that one can specify a concrete type and the compiler supersedes the request.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Oct 13, 2017

Contributor

curtnichols Thank you for reporting this. Firs,t I want to apologize that this change has come into F# 4 .1 midway instead of being done as a language addition/change in F# 4.2. The rising use of System.Tuple meant that the #3016 was becoming a more serious issue that would have meant very inconsistent views of the use of the type when encountered via F# source and C# binaries, which is why I prioritized fixing it. However I hadn't appreciated fully the side effects that addressing the issue would have. Certainly, an RFC should have been written with full analysis, and it is something I will endeavor to do next week - better late than never.

The change to fix #3016 has effectively made (int * int) and Tuple<int*int> equivalent and regular. The case you're seeing above is actually a long standing nit in the F# compiler - which we decided long ago not to change - that there is an element of type-directedness in determining the compiled form of a function that accepts tuples as arguments. Basically a function like this:

let f1 (p : (bool * int))) = ()

or

let f2 ((a,b,c) as d) = ()

will in each case have a compiled form that accepts multiple arguments, rather than one tupled argument. As you're encountering, it now doesn't matter whether you write (bool * int) or Tuple<bool,int> - in either case you now get two arguments. It's at least now consistent in that the two ways of writing tuple types are equivalent

The original rational for this approach was that the following is quite a common coding pattern:

let f3 (a,b,c) = ...
let f4 args = f3 args

In this case, looking only at let f4 args = f3 args, it is reasonable for the programmer to expect that f3 and f4 will have the same compiled form. [ As an aside, we can't "have our cake and eat it" here: it's intrinsic in the fact there is a choice about the compiled form that we have to "pay the cost" at some point in the language design. In this case we chose "implicit type-directed de-tupling happens" over "innocuous rebindings change your compiled form". The F# design has a number of such choices. ]

The only way I know to stop the compiler making this choice is to add a signature file to a module. Previously you could use an explicit System.Tuple as in your code, but that route has now been blocked off by fixing #3016..

To use a signature file, add MyFile.fsi before in the compilation order and write out the contents of the public signature of the file, e.g.

val f1 : p:(int * bool)  -> unit
val f2 : p:(int * int * int)  -> unit

then you will get one argument in the compiled form. If instead you use

val f1 : a:int * b:bool  -> unit
val f2 : a:int * b:int * c:int  -> unit

then you get multiple arguments. AFAIK there is no way to get this behavior any more without a signature file. I'm still thinking about what we should do here, and I will try to document the choices in an RFC next week. We may add such a way, or we could in theory roll back the change that fixed #3016 (though that seems unlikely)

In short the workaround at the moment appears to be to add a signature file. I understand that's not a great workaround though if your implementation file is large.

Contributor

dsyme commented Oct 13, 2017

curtnichols Thank you for reporting this. Firs,t I want to apologize that this change has come into F# 4 .1 midway instead of being done as a language addition/change in F# 4.2. The rising use of System.Tuple meant that the #3016 was becoming a more serious issue that would have meant very inconsistent views of the use of the type when encountered via F# source and C# binaries, which is why I prioritized fixing it. However I hadn't appreciated fully the side effects that addressing the issue would have. Certainly, an RFC should have been written with full analysis, and it is something I will endeavor to do next week - better late than never.

The change to fix #3016 has effectively made (int * int) and Tuple<int*int> equivalent and regular. The case you're seeing above is actually a long standing nit in the F# compiler - which we decided long ago not to change - that there is an element of type-directedness in determining the compiled form of a function that accepts tuples as arguments. Basically a function like this:

let f1 (p : (bool * int))) = ()

or

let f2 ((a,b,c) as d) = ()

will in each case have a compiled form that accepts multiple arguments, rather than one tupled argument. As you're encountering, it now doesn't matter whether you write (bool * int) or Tuple<bool,int> - in either case you now get two arguments. It's at least now consistent in that the two ways of writing tuple types are equivalent

The original rational for this approach was that the following is quite a common coding pattern:

let f3 (a,b,c) = ...
let f4 args = f3 args

In this case, looking only at let f4 args = f3 args, it is reasonable for the programmer to expect that f3 and f4 will have the same compiled form. [ As an aside, we can't "have our cake and eat it" here: it's intrinsic in the fact there is a choice about the compiled form that we have to "pay the cost" at some point in the language design. In this case we chose "implicit type-directed de-tupling happens" over "innocuous rebindings change your compiled form". The F# design has a number of such choices. ]

The only way I know to stop the compiler making this choice is to add a signature file to a module. Previously you could use an explicit System.Tuple as in your code, but that route has now been blocked off by fixing #3016..

To use a signature file, add MyFile.fsi before in the compilation order and write out the contents of the public signature of the file, e.g.

val f1 : p:(int * bool)  -> unit
val f2 : p:(int * int * int)  -> unit

then you will get one argument in the compiled form. If instead you use

val f1 : a:int * b:bool  -> unit
val f2 : a:int * b:int * c:int  -> unit

then you get multiple arguments. AFAIK there is no way to get this behavior any more without a signature file. I'm still thinking about what we should do here, and I will try to document the choices in an RFC next week. We may add such a way, or we could in theory roll back the change that fixed #3016 (though that seems unlikely)

In short the workaround at the moment appears to be to add a signature file. I understand that's not a great workaround though if your implementation file is large.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Oct 13, 2017

Contributor

@forki How serious is the Item1, Item2... thing for you (or others)?

Contributor

dsyme commented Oct 13, 2017

@forki How serious is the Item1, Item2... thing for you (or others)?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 14, 2017

Contributor

For me personally it's no issue at all. I only reported it for @mexx. So he should comment on that.

Contributor

forki commented Oct 14, 2017

For me personally it's no issue at all. I only reported it for @mexx. So he should comment on that.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 14, 2017

Contributor

I mean since destructuring with (i1, i2) is working it's only an annoying step to go over all the item1 thingies and change the code.

Contributor

forki commented Oct 14, 2017

I mean since destructuring with (i1, i2) is working it's only an annoying step to go over all the item1 thingies and change the code.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 14, 2017

Contributor

But then we need to make sure we don't break it ever again.

Contributor

forki commented Oct 14, 2017

But then we need to make sure we don't break it ever again.

@mexx

This comment has been minimized.

Show comment
Hide comment
@mexx

mexx Oct 15, 2017

Contributor

In my case the code base which was broken is not big, I was able to change the code to function again.
For me it's not critical, but I can imagine someone could have more code to change.

Contributor

mexx commented Oct 15, 2017

In my case the code base which was broken is not big, I was able to change the code to function again.
For me it's not critical, but I can imagine someone could have more code to change.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 15, 2017

Contributor
Contributor

forki commented Oct 15, 2017

@curtnichols

This comment has been minimized.

Show comment
Hide comment
@curtnichols

curtnichols Oct 15, 2017

Here is a more sinister (but real) repro, simplified. I've attempted the signature file, but as you can see the Sytem.Tuple is not part of a function signature, so my optimism wanes.

The F# side essentially looks like this:

module Update =

    open System
    open System.IO

    type FileMapper = Func<string, Tuple<bool, Stream>>

    type UpdateArguments = {
        MapFilePath : FileMapper
    }

    [<CompiledName("F")>]
    let f (args : UpdateArguments) =

        let success, file =     let result = args.MapFilePath.Invoke("name")
                                result.Item1, result.Item2
        printfn "%A %A" success file.CanRead

@dsyme

The case you're seeing above is actually a long standing nit in the F# compiler....

I think I grok most of what you're saying, but this is obviously an interop issue--which I think you mention as the reason for prioritizing this change. So this was meant to be a breaking change sooner or later? As in, our understanding of how this interop works needs to change?

I write primarily F#, C#, and C++, with interop between the various factions. Few things push my buttons more than compilers not doing what I ask of them. :)

curtnichols commented Oct 15, 2017

Here is a more sinister (but real) repro, simplified. I've attempted the signature file, but as you can see the Sytem.Tuple is not part of a function signature, so my optimism wanes.

The F# side essentially looks like this:

module Update =

    open System
    open System.IO

    type FileMapper = Func<string, Tuple<bool, Stream>>

    type UpdateArguments = {
        MapFilePath : FileMapper
    }

    [<CompiledName("F")>]
    let f (args : UpdateArguments) =

        let success, file =     let result = args.MapFilePath.Invoke("name")
                                result.Item1, result.Item2
        printfn "%A %A" success file.CanRead

@dsyme

The case you're seeing above is actually a long standing nit in the F# compiler....

I think I grok most of what you're saying, but this is obviously an interop issue--which I think you mention as the reason for prioritizing this change. So this was meant to be a breaking change sooner or later? As in, our understanding of how this interop works needs to change?

I write primarily F#, C#, and C++, with interop between the various factions. Few things push my buttons more than compilers not doing what I ask of them. :)

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Oct 15, 2017

Contributor

@dsyme

I made a small modification to @curtnichols' example, to make the surprise of this behavior more clear.

    [<CompiledName("F")>]
    let f (args : UpdateArguments) =
        let success, file =     let result:Tuple<bool, Stream> = args.MapFilePath.Invoke("name")
                                result.Item1, result.Item2
        printfn "%A %A" success file.CanRead

And it yielded a compile error.

>------ Rebuild All started: Project: Library1, Configuration: Debug Any CPU ------
1>C:\Users\kevinr\source\repos\ConsoleApp1\Library1\Library1.fs(17,40): error FS0039: The field, constructor or member 'Item1' is not defined.
1>C:\Users\kevinr\source\repos\ConsoleApp1\Library1\Library1.fs(17,54): error FS0039: The field, constructor or member 'Item2' is not defined.
1>C:\Users\kevinr\source\repos\ConsoleApp1\Library1\Library1.fs(18,33): error FS0072: Lookup on object of indeterminate type based on information prior to this program point. A type annotation may be needed prior to this program point to constrain the type of the object. This may allow the lookup to be resolved.
1>Done building project "Library1.fsproj" -- FAILED.

I think this error message would surprise most F# developers. and be pretty hard to explain.

Contributor

KevinRansom commented Oct 15, 2017

@dsyme

I made a small modification to @curtnichols' example, to make the surprise of this behavior more clear.

    [<CompiledName("F")>]
    let f (args : UpdateArguments) =
        let success, file =     let result:Tuple<bool, Stream> = args.MapFilePath.Invoke("name")
                                result.Item1, result.Item2
        printfn "%A %A" success file.CanRead

And it yielded a compile error.

>------ Rebuild All started: Project: Library1, Configuration: Debug Any CPU ------
1>C:\Users\kevinr\source\repos\ConsoleApp1\Library1\Library1.fs(17,40): error FS0039: The field, constructor or member 'Item1' is not defined.
1>C:\Users\kevinr\source\repos\ConsoleApp1\Library1\Library1.fs(17,54): error FS0039: The field, constructor or member 'Item2' is not defined.
1>C:\Users\kevinr\source\repos\ConsoleApp1\Library1\Library1.fs(18,33): error FS0072: Lookup on object of indeterminate type based on information prior to this program point. A type annotation may be needed prior to this program point to constrain the type of the object. This may allow the lookup to be resolved.
1>Done building project "Library1.fsproj" -- FAILED.

I think this error message would surprise most F# developers. and be pretty hard to explain.

@varon

This comment has been minimized.

Show comment
Hide comment
@varon

varon Oct 16, 2017

I could easily be missing something, but wouldn't it be a whole lot simpler and avoid tons of garbage if we could just use the new struct tuples everywhere and get rid of the old class-based tuples?

varon commented Oct 16, 2017

I could easily be missing something, but wouldn't it be a whole lot simpler and avoid tons of garbage if we could just use the new struct tuples everywhere and get rid of the old class-based tuples?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Oct 16, 2017

Contributor

@varon

I think this error message would surprise most F# developers. and be pretty hard to explain.

F# developers don't use Tuple or Item1 explicitly - they use (int * int) and pattern matching.

We could allow the use of ItemN properties. However these properties are defined in the compiled form of the types, which is relevant for large tuples. So, for large tuples, would (int * ...... * int) 10-tuple have a Item10 property? Or only up to Item7 or Item8, with Item8 revealing the awkward encoding of large tuples? Either we "fake" the properties, or reveal the compiled form, neither of which I'm rushing to do.

In any case the correct thing to do in F# is to adjust the code above to this:

let f (args : UpdateArguments) =
    let success, file = args.MapFilePath.Invoke("name")
    printfn "%A %A" success file.CanRead

As you can see, the code was just a kludge to workaround the fact that F# wasn't understanding that Tuple<X,Y> is the same as (X * Y)

Contributor

dsyme commented Oct 16, 2017

@varon

I think this error message would surprise most F# developers. and be pretty hard to explain.

F# developers don't use Tuple or Item1 explicitly - they use (int * int) and pattern matching.

We could allow the use of ItemN properties. However these properties are defined in the compiled form of the types, which is relevant for large tuples. So, for large tuples, would (int * ...... * int) 10-tuple have a Item10 property? Or only up to Item7 or Item8, with Item8 revealing the awkward encoding of large tuples? Either we "fake" the properties, or reveal the compiled form, neither of which I'm rushing to do.

In any case the correct thing to do in F# is to adjust the code above to this:

let f (args : UpdateArguments) =
    let success, file = args.MapFilePath.Invoke("name")
    printfn "%A %A" success file.CanRead

As you can see, the code was just a kludge to workaround the fact that F# wasn't understanding that Tuple<X,Y> is the same as (X * Y)

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Oct 16, 2017

Contributor

@dsyme I think in an ideal fix for this a variable/symbol declared as (X * Y) would not have ItemN properties.
But if the developer explicitly declares a variable/symbol as Tuple<...> I feel like the ItemN properties should become available (while otherwise still behave exactly the same).

  1. This would solve this bug as we would gain backwards compatibility
  2. It removes the surprise of people going to the docs and expecting the ItemN properties.

Besides that I feel like X*Y should behave like Tuple<X,Y>. Is there a problem with this suggestion? Is it too hard to implement? Basically the compiler has to "remember" if the type was declared as F# tuple or System.Tuple and unify them later (if at all).

I think what I'm missing in the above solution is how we can differentiate F# tuples from System.Tuple<...> in external references. Is it even feasible to detect if a function type was declared as F# or System.Tuple after compilation? Probably not. But then I'd favor @dsyme's argument of not enabling ItemN everywhere.

Contributor

matthid commented Oct 16, 2017

@dsyme I think in an ideal fix for this a variable/symbol declared as (X * Y) would not have ItemN properties.
But if the developer explicitly declares a variable/symbol as Tuple<...> I feel like the ItemN properties should become available (while otherwise still behave exactly the same).

  1. This would solve this bug as we would gain backwards compatibility
  2. It removes the surprise of people going to the docs and expecting the ItemN properties.

Besides that I feel like X*Y should behave like Tuple<X,Y>. Is there a problem with this suggestion? Is it too hard to implement? Basically the compiler has to "remember" if the type was declared as F# tuple or System.Tuple and unify them later (if at all).

I think what I'm missing in the above solution is how we can differentiate F# tuples from System.Tuple<...> in external references. Is it even feasible to detect if a function type was declared as F# or System.Tuple after compilation? Probably not. But then I'd favor @dsyme's argument of not enabling ItemN everywhere.

@eiriktsarpalis eiriktsarpalis referenced this pull request Nov 27, 2017

Closed

.netcore support? #83

@gusty

This comment has been minimized.

Show comment
Hide comment
@gusty

gusty Nov 27, 2017

Contributor

@ingted @dsyme +1, this regression makes it impossible to build FsPickler without resorting to reflection.

... and it worth clarifying that .Rest is also being used in FsPickler.

Contributor

gusty commented Nov 27, 2017

@ingted @dsyme +1, this regression makes it impossible to build FsPickler without resorting to reflection.

... and it worth clarifying that .Rest is also being used in FsPickler.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 27, 2017

Contributor

... and it worth clarifying that .Rest is also being used in FsPickler.

Yeah, I saw that.

@cartermp One approach to this breaking change would simply be to revert #3283 and try again with that change in F# 4.2.

Contributor

dsyme commented Nov 27, 2017

... and it worth clarifying that .Rest is also being used in FsPickler.

Yeah, I saw that.

@cartermp One approach to this breaking change would simply be to revert #3283 and try again with that change in F# 4.2.

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Nov 27, 2017

Collaborator

@dsyme I agree that reverting #3283 is a good way to go, but such a change will not be until at least the VS 2017 15.6 release. We're currently in Ask Mode for the first Preview (group engineering manager approval required for changes), which means reverting would have to happen ASAP so that we can get it into as many previews as possible and have ample time to react to any breaking changes it causes, should people now be relying on the new behavior. But the final release is not likely to be for a few months. There is a chunk of time allocated for addressing issues with the large forthcoming 15.5 release. And we're far, far beyond the time where we could get a compiler change into 15.5.

We could also look into shipping a VSIX through the gallery, but this will not have the same reach as VSIX distributed in-box, plus it's another thing we'll need to account for until 15.6 RTW. So I think we should look for a way to unblock FsPickler in the interim.

I'm a bit confused about the issue with FsPickler. Looking at the code, it should work with the new compiler with small tweaks. @dsyme says this:

In F# A * B and Tuple<A,B> are now two syntaxes for exactly the same thing. They are identical.

However, in a PR intended to unblock FsPicker, @eiriktsarpalis says this:

Thanks, but Tuple`8 types are not exactly the same as a tuple of 8 elements. In order to get this to work, the F# compiler issue must be fixed.

Does the FsPickler build contain the same compiler as #3283? If not, can we try to update it and see what happens?

Collaborator

cartermp commented Nov 27, 2017

@dsyme I agree that reverting #3283 is a good way to go, but such a change will not be until at least the VS 2017 15.6 release. We're currently in Ask Mode for the first Preview (group engineering manager approval required for changes), which means reverting would have to happen ASAP so that we can get it into as many previews as possible and have ample time to react to any breaking changes it causes, should people now be relying on the new behavior. But the final release is not likely to be for a few months. There is a chunk of time allocated for addressing issues with the large forthcoming 15.5 release. And we're far, far beyond the time where we could get a compiler change into 15.5.

We could also look into shipping a VSIX through the gallery, but this will not have the same reach as VSIX distributed in-box, plus it's another thing we'll need to account for until 15.6 RTW. So I think we should look for a way to unblock FsPickler in the interim.

I'm a bit confused about the issue with FsPickler. Looking at the code, it should work with the new compiler with small tweaks. @dsyme says this:

In F# A * B and Tuple<A,B> are now two syntaxes for exactly the same thing. They are identical.

However, in a PR intended to unblock FsPicker, @eiriktsarpalis says this:

Thanks, but Tuple`8 types are not exactly the same as a tuple of 8 elements. In order to get this to work, the F# compiler issue must be fixed.

Does the FsPickler build contain the same compiler as #3283? If not, can we try to update it and see what happens?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 27, 2017

Contributor

Yes, I understand it would be for 15.6 at the earliest.

I'm a bit confused about the issue with FsPickler. Looking at the code, it should work with the new compiler with small tweaks.

My understanding is that FsPickler programs over Tuple`8 (i.e. Tuple<t1,...,t8>) with explicit access to Rest in order to deal with arbitrarily sized tuples without using reflection. That programming technique is not possible after #3283 (partly because accessing Rest is not possible, and particular because an F# 8-tuple (int * .... * int) corresponds only tuples of logical size 8, where :? Tuple`8 matches tuples of logical size 8 and greater).

So @eiriktsarpalis is correct that it's not possible after #3283. I hadn't appreciated that that approach to dealing with arbitrary-sized tuples was possible. Perhaps we should add a snapshot of FsPickler to our tests... :)

Contributor

dsyme commented Nov 27, 2017

Yes, I understand it would be for 15.6 at the earliest.

I'm a bit confused about the issue with FsPickler. Looking at the code, it should work with the new compiler with small tweaks.

My understanding is that FsPickler programs over Tuple`8 (i.e. Tuple<t1,...,t8>) with explicit access to Rest in order to deal with arbitrarily sized tuples without using reflection. That programming technique is not possible after #3283 (partly because accessing Rest is not possible, and particular because an F# 8-tuple (int * .... * int) corresponds only tuples of logical size 8, where :? Tuple`8 matches tuples of logical size 8 and greater).

So @eiriktsarpalis is correct that it's not possible after #3283. I hadn't appreciated that that approach to dealing with arbitrary-sized tuples was possible. Perhaps we should add a snapshot of FsPickler to our tests... :)

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Nov 27, 2017

Collaborator

So that means that this code basically won't work? Suppose I could just pull it down and try it out.

Collaborator

cartermp commented Nov 27, 2017

So that means that this code basically won't work? Suppose I could just pull it down and try it out.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 27, 2017

Contributor

So that means that this code basically won't work? Suppose I could just pull it down and try it out.

Correct - specifically it won't work on a 9+ tuple

Contributor

dsyme commented Nov 27, 2017

So that means that this code basically won't work? Suppose I could just pull it down and try it out.

Correct - specifically it won't work on a 9+ tuple

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Nov 27, 2017

Collaborator

Okay, in that case let's revert and try to get it into the 15.6 preview

Collaborator

cartermp commented Nov 27, 2017

Okay, in that case let's revert and try to get it into the 15.6 preview

@eiriktsarpalis

This comment has been minimized.

Show comment
Hide comment
@eiriktsarpalis

eiriktsarpalis Nov 28, 2017

Contributor

FWIW here's a small snippet that illustrates how tuples of large arities are encoded:

open FSharp.Reflection

let mkTupleTy arity = 
    Array.init arity (fun _ -> typeof<int>)
    |> FSharpType.MakeTupleType
    |> string

Array.init 20 ((+) 1 >> mkTupleTy)

8-tuples cannot work either, because they also rely on nesting a 1-tuple in the Rest parameter. In general it is not possible to pattern match on a Rest argument using regular tuple expressions.

Contributor

eiriktsarpalis commented Nov 28, 2017

FWIW here's a small snippet that illustrates how tuples of large arities are encoded:

open FSharp.Reflection

let mkTupleTy arity = 
    Array.init arity (fun _ -> typeof<int>)
    |> FSharpType.MakeTupleType
    |> string

Array.init 20 ((+) 1 >> mkTupleTy)

8-tuples cannot work either, because they also rely on nesting a 1-tuple in the Rest parameter. In general it is not possible to pattern match on a Rest argument using regular tuple expressions.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 28, 2017

Contributor

Okay, in that case let's revert and try to get it into the 15.6 preview

@cartermp See #4029, though as I note in the PR it makes me feel queasy. The alternative is to implement the proposal above after all:

• For Tuples and ValueTuples allow Items 1-7 + Rest.
• Warn on explicit use of Item* and Rest
• Hide Tuple members from intellisense

I'm deeply annoyed to have placed us in this position. Careless.

Contributor

dsyme commented Nov 28, 2017

Okay, in that case let's revert and try to get it into the 15.6 preview

@cartermp See #4029, though as I note in the PR it makes me feel queasy. The alternative is to implement the proposal above after all:

• For Tuples and ValueTuples allow Items 1-7 + Rest.
• Warn on explicit use of Item* and Rest
• Hide Tuple members from intellisense

I'm deeply annoyed to have placed us in this position. Careless.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Nov 28, 2017

Contributor

Warn on explicit use of Item* and Rest

Would that really be necessary in your opinion?

I find destructuring larger tuples to be annoying (even though large tuples are probably an anti-pattern).

Compare

let (_,__,x,_,_,_) = tpl
let x = tpl.Item3

In the first case it is really not obvious which member is accessed - I need to count the number of ignored items.

This issue is compounded by the fact that it is not possible to write universal accessor functions fst, snd, or itemN which work over all tuple sizes. So I would need to create fstOf4, fstOf5 and so on.

Edit: exposing ItemX and Rest would allow to create these universal accessors using SRTP for all tuple-like types.

Contributor

0x53A commented Nov 28, 2017

Warn on explicit use of Item* and Rest

Would that really be necessary in your opinion?

I find destructuring larger tuples to be annoying (even though large tuples are probably an anti-pattern).

Compare

let (_,__,x,_,_,_) = tpl
let x = tpl.Item3

In the first case it is really not obvious which member is accessed - I need to count the number of ignored items.

This issue is compounded by the fact that it is not possible to write universal accessor functions fst, snd, or itemN which work over all tuple sizes. So I would need to create fstOf4, fstOf5 and so on.

Edit: exposing ItemX and Rest would allow to create these universal accessors using SRTP for all tuple-like types.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Nov 28, 2017

Contributor

In the current state, it seems also impossible to explicitly call the constructor of System.Tuple:

let tpl3 = new System.Tuple<int,int,int>(8,9,10)

error FS0756: 'new' may only be used with named types

Contributor

0x53A commented Nov 28, 2017

In the current state, it seems also impossible to explicitly call the constructor of System.Tuple:

let tpl3 = new System.Tuple<int,int,int>(8,9,10)

error FS0756: 'new' may only be used with named types

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 29, 2017

Contributor

@0x53A Re Item

I understand the temptation to add these to the F# programming model for all tuple values. However, see my comment above, particularly:

A separate consideration which is weighing on my mind is that using Item properties for tuple elements makes code less readable... When you are forced to use tuple decomposition with a pattern you are forced to give names to the elements of the tuple, which is valuable metadata,.....[using Item properties] is horrible for readability and maintainability. People are lazy and love hitting . ....making Item properties available in intellisense lists will not remotely increase the quality of F# code being written, and indeed will harm it.

Contributor

dsyme commented Nov 29, 2017

@0x53A Re Item

I understand the temptation to add these to the F# programming model for all tuple values. However, see my comment above, particularly:

A separate consideration which is weighing on my mind is that using Item properties for tuple elements makes code less readable... When you are forced to use tuple decomposition with a pattern you are forced to give names to the elements of the tuple, which is valuable metadata,.....[using Item properties] is horrible for readability and maintainability. People are lazy and love hitting . ....making Item properties available in intellisense lists will not remotely increase the quality of F# code being written, and indeed will harm it.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 29, 2017

Contributor

In the current state, it seems also impossible to explicitly call the constructor of System.Tuple:

Yes, that is correct. The proposal here would be not to change this.

Contributor

dsyme commented Nov 29, 2017

In the current state, it seems also impossible to explicitly call the constructor of System.Tuple:

Yes, that is correct. The proposal here would be not to change this.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Nov 29, 2017

Contributor

[...] When you are forced to use tuple decomposition with a pattern you are forced to give names to the elements of the tuple, which is valuable metadata,..... [...]

This is a nice goal, but I don't think it actually holds in practice. In reality I will use

let (_,__,x,_,_,_) = tpl

where x doesn't really tell me anything, and I lose the information which member I access and so need to manually count.


IF you really hate ItemX, please consider fsharp/fslang-suggestions#628. The SRTP method needs ItemX visible, but it could still warn on explicit use, and/or hide them from intellisense, as long as they are visible to the constraint solver.

Though I am not sure if there is a big difference between

let x = tpl.Item2

and

let x = snd tpl

...

Contributor

0x53A commented Nov 29, 2017

[...] When you are forced to use tuple decomposition with a pattern you are forced to give names to the elements of the tuple, which is valuable metadata,..... [...]

This is a nice goal, but I don't think it actually holds in practice. In reality I will use

let (_,__,x,_,_,_) = tpl

where x doesn't really tell me anything, and I lose the information which member I access and so need to manually count.


IF you really hate ItemX, please consider fsharp/fslang-suggestions#628. The SRTP method needs ItemX visible, but it could still warn on explicit use, and/or hide them from intellisense, as long as they are visible to the constraint solver.

Though I am not sure if there is a big difference between

let x = tpl.Item2

and

let x = snd tpl

...

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 29, 2017

Contributor

In reality I will use let (_,__,x,_,_,_) = tpl where x doesn't really tell me anything, and I lose the information which member I access and so need to manually count.

I agree this kind of code is poor and in good, long-lived code there should be accurate transition paths away from it. Using a large tuple or a name x - well that is a programmer's problem. However, there are progressive, accurate steps the programmer can take to rectify the mess, e.g. turn the tuple values into a record type or a single-case discriminated union.

In contrast offering tpl.Item3 in the intellisense list is worse

  1. We actively incentivize the use of a crappy, meaningless name (even x is better than Item3)
  2. There is no incentive to move away from a large tuple
  3. Programming is less accurate under change. For example, consider when an extra item is added or removed from the front of the tuple - you have to manually hunt down and correct all Item accesses that aren't caught by the type checker. With an explicit tuple, you at least manually go and adjust the tuple patterns that consume the tuple by adding an _ at the appropriate point
Contributor

dsyme commented Nov 29, 2017

In reality I will use let (_,__,x,_,_,_) = tpl where x doesn't really tell me anything, and I lose the information which member I access and so need to manually count.

I agree this kind of code is poor and in good, long-lived code there should be accurate transition paths away from it. Using a large tuple or a name x - well that is a programmer's problem. However, there are progressive, accurate steps the programmer can take to rectify the mess, e.g. turn the tuple values into a record type or a single-case discriminated union.

In contrast offering tpl.Item3 in the intellisense list is worse

  1. We actively incentivize the use of a crappy, meaningless name (even x is better than Item3)
  2. There is no incentive to move away from a large tuple
  3. Programming is less accurate under change. For example, consider when an extra item is added or removed from the front of the tuple - you have to manually hunt down and correct all Item accesses that aren't caught by the type checker. With an explicit tuple, you at least manually go and adjust the tuple patterns that consume the tuple by adding an _ at the appropriate point
@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Nov 29, 2017

Contributor

Do you think the same about external accessor functions (fst, snd, thrd, ...)?

The big advantage of them is that they aren't easily discoverable (you can't dot into them from the tuple itself), so you can only use them if you already know they are there ;-)

Otherwise they have the same issues as ItemX properties.

Contributor

0x53A commented Nov 29, 2017

Do you think the same about external accessor functions (fst, snd, thrd, ...)?

The big advantage of them is that they aren't easily discoverable (you can't dot into them from the tuple itself), so you can only use them if you already know they are there ;-)

Otherwise they have the same issues as ItemX properties.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 29, 2017

Contributor

Do you think the same about external accessor functions (fst, snd, thrd, ...)?

I'm not fundamentally opposed to generic constructs over generic data shapes. For tuple projections I think the incentives are set about right for those in F# today:

  • fst and snd are there but you aren't particularly incentivised to use them

  • You can define additional projections (e.g. p13, p14 as we do in the compiler code) but you probably know what you're doing in that case

Contributor

dsyme commented Nov 29, 2017

Do you think the same about external accessor functions (fst, snd, thrd, ...)?

I'm not fundamentally opposed to generic constructs over generic data shapes. For tuple projections I think the incentives are set about right for those in F# today:

  • fst and snd are there but you aren't particularly incentivised to use them

  • You can define additional projections (e.g. p13, p14 as we do in the compiler code) but you probably know what you're doing in that case

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Dec 9, 2017

Contributor

The fix for this issue is at #4034

Contributor

dsyme commented Dec 9, 2017

The fix for this issue is at #4034

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme
Contributor

dsyme commented Dec 9, 2017

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Jan 24, 2018

Contributor

Thanks for this,

I am closing because I believe this is handled by Don's PR: #4034

Contributor

KevinRansom commented Jan 24, 2018

Thanks for this,

I am closing because I believe this is handled by Don's PR: #4034

@cartermp cartermp modified the milestones: VS 2017 Updates, 15.6 Jun 9, 2018

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