Skip to content

Breaking change in tuples after VS 15.4 update #3729

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

Closed
wants to merge 1 commit into from

Conversation

forki
Copy link
Contributor

@forki 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
Copy link
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.

@dsyme dsyme added Regression Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. Area-Compiler labels Oct 11, 2017
@forki
Copy link
Contributor Author

forki commented Oct 11, 2017

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

@matthid
Copy link
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
Copy link
Contributor Author

forki commented Oct 11, 2017

Workaround:

image

@forki
Copy link
Contributor Author

forki commented Oct 11, 2017

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

@dsyme
Copy link
Contributor

dsyme commented Oct 11, 2017

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

@dsyme
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link

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
Copy link
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
Copy link
Contributor

dsyme commented Oct 13, 2017

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

@forki
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

forki commented Oct 14, 2017

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

@mexx
Copy link
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
Copy link
Contributor Author

forki commented Oct 15, 2017 via email

@curtnichols
Copy link

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
Copy link
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.

@eiriktsarpalis
Copy link
Member

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

@gusty
Copy link
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
Copy link
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
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

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

@dsyme
Copy link
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
Copy link
Contributor

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

@eiriktsarpalis
Copy link
Member

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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
Contributor

dsyme commented Dec 9, 2017

The fix for this issue is at #4034

@dsyme
Copy link
Contributor

dsyme commented Dec 9, 2017

The RFC issue for this whole tuple malarkey is at https://github.com/fsharp/fslang-design/blob/master/FSharp-4.1b/FS-1046-consistent-tuple-types.md

@KevinRansom
Copy link
Contributor

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
Labels
Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. Regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.