-
Notifications
You must be signed in to change notification settings - Fork 826
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
Conversation
OK, so this is a regression. We fixed an issue to translate I'm not sure what to do about this, we will need to think about it. |
Tbh I think it should compile in any case since we only use system.tuple everywhere |
I think whenever the type Besides that The easier solution would be to allow |
ok this fails in ci_part1 for unrelated reason. I guess master is broken |
@forki Yes, there's something fishy with the latest commit "Merge branch 'vs2017-rtm' into master". |
|
@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 |
@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 Maybe the compiler needs to be less eager in converting to F# tuples as it is right now? |
ok so the test is capturing the bug. |
Found the same issue in our code base, C# calling an F# function that explicitly specifies
I'm finding it rather troublesome that one can specify a concrete type and the compiler supersedes the request. |
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
or
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 The original rational for this approach was that the following is quite a common coding pattern:
In this case, looking only at 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 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.
then you will get one argument in the compiled form. If instead you use
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. |
@forki How serious is the |
For me personally it's no issue at all. I only reported it for @mexx. So he should comment on that. |
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. |
But then we need to make sure we don't break it ever again. |
In my case the code base which was broken is not big, I was able to change the code to function again. |
Tbf 8 think it's now pretty usable when you consider that you can basically
destructure all kinds of tuples now with same syntax. Only trouble is that
it is breaking...
Am 15.10.2017 14:52 schrieb "Max Malook" <notifications@github.com>:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3729 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNNFKOwSHGs12MW7TlOXHuoMmIW4qks5ssgAVgaJpZM4P1Qzn>
.
|
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:
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. :) |
I made a small modification to @curtnichols' example, to make the surprise of this behavior more clear.
And it yielded a compile error.
I think this error message would surprise most F# developers. and be pretty hard to explain. |
... and it worth clarifying that |
@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:
However, in a PR intended to unblock FsPicker, @eiriktsarpalis says this:
Does the FsPickler build contain the same compiler as #3283? If not, can we try to update it and see what happens? |
Yes, I understand it would be for 15.6 at the earliest.
My understanding is that FsPickler programs over 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... :) |
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 |
Okay, in that case let's revert and try to get it into the 15.6 preview |
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 |
@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. I'm deeply annoyed to have placed us in this position. Careless. |
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
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 Edit: exposing ItemX and Rest would allow to create these universal accessors using SRTP for all tuple-like types. |
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)
|
@0x53A Re I understand the temptation to add these to the F# programming model for all tuple values. However, see my comment above, particularly:
|
Yes, that is correct. The proposal here would be not to change this. |
This is a nice goal, but I don't think it actually holds in practice. In reality I will use
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
and
... |
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 In contrast offering
|
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. |
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:
|
The fix for this issue is at #4034 |
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 |
Thanks for this, I am closing because I believe this is handled by Don's PR: #4034 |
With VS Update 15.3.5 we get:
With VS Update 15.4 we see:
/cc @mexx @drvink