Uh oh!
There was an error while loading. Please reload this page.
[CompilerPerf] make tuples support Item* with warning - #4034
Conversation
cartermp
commented
Nov 29, 2017
This is definitely the better route to go! |
dsyme
commented
Nov 29, 2017
@cartermp Here are the known issues:
|
dsyme
commented
Dec 1, 2017
@dotnet-bot Test Windows_NT Release_ci_part1 Build please |
dsyme
commented
Dec 1, 2017
CI is broken, we need to re-test this |
gusty
commented
Dec 4, 2017
I've tested this with some code I had working since F# 2.0, using overload resolution and now it breaks. |
gusty
left a comment
There was a problem hiding this comment.
It will be great if this commit is split in 3:
- Add support for Item* and Rest
- Add warning
- Add tests
realvictorprm
commented
Dec 5, 2017
I wouldn't separate the tests |
gusty
commented
Dec 5, 2017
@realvictorprm Why not? I think it will be better to have them separated to make it easier to understand what makes each change and so I (or someone else) can eventually contribute to fix it. |
dsyme
commented
Dec 5, 2017
@dotnet-bot Test this please |
dsyme
commented
Dec 5, 2017
@gusty I think we'll need a repro for this - could you make one for us? thanks |
realvictorprm
commented
Dec 5, 2017
@gusty In terms of stability I prefer to add tests with a new feature if testable. |
@dsyme Yes I can, but since the Otherwise, do you know another way to create a System.Tuple ? I tried this: But it get converted to a syntactic tuple. Honestly I still don't understand the new auto-conversion rules. Because not all System.Tuples get automatically converted, for instance you mention that |
Here's the repro: Create a dll with this helpers: Compile it with F# 4.0, then run this script with the FSI produced by this PR This compiled always fine, but now I get all the desired warnings plus: |
cartermp
commented
Dec 5, 2017
Tagging this for the 15.6 milestone, which is targeted to release early next year. |
gusty
commented
Dec 6, 2017
Regarding SRTPs, the following was always working until the breaking change, this is what I get with this PR: so I try like this (note the following warning was always there, is not new and it makes sense): ... still trying to understand what are the new autoconversion rules |
dsyme
commented
Dec 8, 2017
@gusty OK, - just to clarify - the things you mention are not made particularly worse by this PR (except for the
The change implemented by #3283 is this:
Previously this conversion applied only to occurrences of |
gusty
commented
Dec 8, 2017
Thanks @dsyme
Yes, that's what I thought. But hopefully it can be fixed here.
But then how's possible that the |
dsyme
commented
Dec 8, 2017
It is a mistake in this PR, I will fix it for the sake of consistency |
dsyme
commented
Dec 8, 2017
@gusty I've fixed the problems with both SRTP and @cartermp I've created an F# 4. RFC about this whole mess https://github.com/fsharp/fslang-design/blob/master/FSharp-4.1b/FS-1046-consistent-tuple-types.md. We can gradually fill in more detail there to document the issue |
@dsyme I can see it's fixed but I still notice some inconsistencies:
|
gusty
commented
Dec 8, 2017
Actually is the 8-tuple constructor which is not converting, try this: |
dsyme
commented
Dec 9, 2017
This is correct. According to the F#/C# specs, the correct encoding of an 8-tuple is So there are
Is this for any library method or only the 8-tuple one? |
I made some code sanitizations that reduced some indirections and indirect calls, and this seemed to improve things So perhaps a small slow down showing now but nothing like the previous measure (which may have been a mis-measurement in any case) CI part 2 failed due to a virus check failure on |
dsyme
commented
Jan 20, 2018
@dotnet-bot test Windows_NT Release_ci_part1 Build please |
dsyme
commented
Jan 20, 2018
A test failure was causing CI part 1 to hang. Now that's fixed the actual failures from those unit tests have been revealed, and there a whole bunch of them - it looks like there is some common cause to all of these |
dsyme
commented
Jan 20, 2018
@KevinRansom@cartermp I won't be at a computer where I can properly look at those failures until Sunday evening GMT |
cartermp
commented
Jan 20, 2018
No worries |
dsyme
commented
Jan 22, 2018
@cartermp I think I've finally got this sorted. Tracking down the last couple of test failures now. |
| //let isUnseenByBeingTupleMethod () = isAnyTupleTy g typ | ||
| isUnseenByObsoleteAttrib () || isUnseenByHidingAttribute () //|| isUnseenByBeingTupleMethod () |
| notAFunctionButMaybeDeclaration,"This value is not a function and cannot be applied. Did you forget to terminate a declaration?" | ||
| 3218,ArgumentsInSigAndImplMismatch,"The argument names in the signature '%s' and implementation '%s' do not match. The argument name from the signature file will be used. This may cause problems when debugging or profiling." | ||
| 3219,pickleUnexpectedNonZero,"An error occurred while reading the F# metadata of assembly '%s'. A reserved construct was utilized. You may need to upgrade your F# compiler or use an earlier version of the assembly that doesn't make use of a specific construct." | ||
| 3220,tcTupleMemberNotNormallyUsed,"This method or property is not normally used from F# code, use an explicit tuple pattern for deconstruction instead" |
There was a problem hiding this comment.
"This method or property is not normally used from F# code, use an explicit tuple pattern for deconstruction."
delete instead add full stop.
| //| CanImplementSpecificInterfaceSlot parentTy -> isInterfaceTy g dispatchSlot.EnclosingType && typeEquiv g parentTy dispatchSlot.EnclosingType | ||
| | CanImplementAnyInterfaceSlot -> isInterfaceTy g dispatchSlot.EnclosingType) | ||
| | CanImplementAnyClassHierarchySlot -> not (isInterfaceTy g dispatchSlot.LogicalEnclosingType) | ||
| //| CanImplementSpecificInterfaceSlot parentTy -> isInterfaceTy g dispatchSlot.LogicalEnclosingType && typeEquiv g parentTy dispatchSlot.LogicalEnclosingType |
| [<NoComparison; NoEquality>] | ||
| type MethInfo = | ||
| /// FSMeth(tcGlobals, declaringType, valRef, extensionMethodPriority). | ||
| /// FSMeth(tcGlobals, enclosingType, valRef, extensionMethodPriority). |
There was a problem hiding this comment.
This one is part of the comment showing the use of the union case
| /// Get the enclosing ("parent"/"declaring") type of the field. | ||
| member x.LogicalEnclosingType = match x with ILEventInfo(tinfo,_) -> tinfo.ToType | ||
| // Note: events are always assocaited with nominal types |
dsyme
commented
Jan 22, 2018
Yay, it is green at last However, this PR ended up quite big because it broke an invariant that previously held in the compiler. As mentioned above in this PR we continue to normalize all tuple types to use the To get a grip on this, the PR carefully
@cartermp@KevinRansom I assume the PR is much too big to apply in escrow. There's not a lot I can do about this. I knew that making F# tuples support the same metadata members as are declared in .NET tuple type metadata would be an invasive change, which is why I held off making this change at all. Now it is done, I'll leave it up to you about when to integrate it, thanks |
KevinRansom
commented
Jan 22, 2018
@dsyme, targeting for 15.6 preview 4. |
cartermp
commented
Jan 22, 2018
Just to confirm what was communicated via email:
|
KevinRansom
commented
Jan 22, 2018
@dotnet-bot test Ubuntu14.04 Release_fcs Build please |
psantoro
commented
Feb 2, 2018
I’ve actually been working on another totally unrelated, nasty breaking change (haf/DotNetZip.Semverd#113), so I’m very late to this conversation. However, I’d like to offer some advice in the hopes that it may be useful to the F# community in the future. I apologize if what I’m going to say has been said before, but I haven’t had time to read every comment in detail. Breaking changes are never fun, but given that we are all human, they will continue to happen. I’ve probably been developing software longer than most of you have been alive. I’ve been both the receiver and creator of breaking changes – and I have respect for both sides. (Nowadays, most of the breaking changes that I create are on purpose and are related to refactoring.) I do think it’s important to have a proper balance between how the two sides are ultimately asked to handle these unforeseen events. Whenever possible, breaking changes at the F# application source level should be avoided. However, I don’t believe that we should be opposed to all breaking changes – as none of us is so smart that we will always get things right the first time. Sometimes breaking changes at the F# source level may be necessary/wise in order to maintain/enforce/enhance language idioms/consistency and allow the F# compiler writers more latitude to do their work (e.g. more easily support new features, ease ongoing maintenance efforts, take advantage of performance opportunities, etc.). In this particular breaking change, it’s totally understandable why this issue was missed before it was released. I betting that most F# developers use the F# syntax for constructing/deconstructing tuples and let the compiler deal with the underlying representation – in lieu of using Tuple or ItemN. If the work to develop the new warning message for this issue was only a small part of the patch, perhaps it might have been better to have made this an error message instead (this was briefly discussed). Here’s my reasoning behind this:
Other communities, that I’m familiar with, handle breaking changes by doing the following:
This allows the language maintainers to throw away cruft at predetermined times and gives application developers time to make any necessary changes. Perhaps this is an approach the F# community can use in the future, too? Peter Santoro |
abelbraaksma
commented
Oct 26, 2019
@psantoro, this was not an intentional breaking change. It was a regression bug that caused code to break. This PR fixes the regression. Wrt to the rest you're saying: breaking changes are very rarely accepted, if ever. When they are, they are communicated and discussed thoroughly with the users and a graceful path to the new situation is offered. I think that's in line with what you're suggesting. If you do find that you're still having issues, you can of course write a bug report or a feature request. |
gusty
commented
Jan 20, 2020
@dsyme for tuples greater than 7, is there a way (strong type way) to 'force' a conversion to F# syntactic tuple? I know that if at compile-time the expected type is known I can "retype" the .net tuple (using inlined IL) and it will work, but what if I only know that the type is a tuple of n elements and not the individual type of each element? |
* make tuples support Item* with warning * add deactivated tests for struct tuple * proper return types for Rest, prototype ctors * fix SRTP * fix tests * fix tests * add more protection * improve code for decompiling types * fix unit tests * don't rebuild * make infos systematic to reduce use of tcrefOfAppTy * update test cases * fix build * update test cases * bump FCS version consistently * use consistent names * code review
* make tuples support Item* with warning * add deactivated tests for struct tuple * proper return types for Rest, prototype ctors * fix SRTP * fix tests * fix tests * add more protection * improve code for decompiling types * fix unit tests * don't rebuild * make infos systematic to reduce use of tcrefOfAppTy * update test cases * fix build * update test cases * bump FCS version consistently * use consistent names * code review
The RFC issue for this is at https://github.com/fsharp/fslang-design/blob/master/FSharp-4.1b/FS-1046-consistent-tuple-types.md
This is an alternative to #4029 to deal with the gradual emergence of more serious cases of regressions in #3729 because of change #3283
Item1-7+Rest+new System.Tuple...This does not add Item* and Rest* access for struct tuples, since they only became available in F# 4.1 and it is not really necessary to add those to give good warnings for the regression