-
Notifications
You must be signed in to change notification settings - Fork 773
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
[CompilerPerf] make tuples support Item* with warning #4034
Conversation
This is definitely the better route to go! |
@cartermp Here are the known issues:
|
@dotnet-bot Test Windows_NT Release_ci_part1 Build please |
CI is broken, we need to re-test this |
I've tested this with some code I had working since F# 2.0, using overload resolution and now it breaks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be great if this commit is split in 3:
- Add support for Item* and Rest
- Add warning
- Add tests
I wouldn't separate the tests |
@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. |
@dotnet-bot Test this please |
@gusty I think we'll need a repro for this - could you make one for us? thanks |
@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:
|
Tagging this for the 15.6 milestone, which is targeted to release early next year. |
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 |
@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 |
Thanks @dsyme
Yes, that's what I thought. But hopefully it can be fixed here.
But then how's possible that the |
It is a mistake in this PR, I will fix it for the sake of consistency |
@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:
|
Actually is the 8-tuple constructor which is not converting, try this:
|
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 |
@dotnet-bot test Windows_NT Release_ci_part1 Build please |
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
|
@KevinRansom @cartermp I won't be at a computer where I can properly look at those failures until Sunday evening GMT |
No worries |
@cartermp I think I've finally got this sorted. Tracking down the last couple of test failures now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good ... a few cosmetic items, if you want to take care of them.
|
||
//let isUnseenByBeingTupleMethod () = isAnyTupleTy g typ | ||
|
||
isUnseenByObsoleteAttrib () || isUnseenByHidingAttribute () //|| isUnseenByBeingTupleMethod () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented out code.
src/fsharp/FSComp.txt
Outdated
@@ -1421,3 +1421,4 @@ notAFunctionButMaybeIndexer,"This expression is not a function and cannot be app | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This method or property is not normally used from F# code, use an explicit tuple pattern for deconstruction."
delete instead add full stop.
src/fsharp/MethodOverrides.fs
Outdated
//| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more commented out code
@@ -845,7 +866,7 @@ type ILMethInfo = | |||
/// Describes an F# use of a method | |||
[<NoComparison; NoEquality>] | |||
type MethInfo = | |||
/// FSMeth(tcGlobals, declaringType, valRef, extensionMethodPriority). | |||
/// FSMeth(tcGlobals, enclosingType, valRef, extensionMethodPriority). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is part of the comment showing the use of the union case
src/fsharp/infos.fs
Outdated
/// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: assocaited
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 |
@dsyme, targeting for 15.6 preview 4. |
Just to confirm what was communicated via email:
|
@dotnet-bot test Ubuntu14.04 Release_fcs Build please |
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 |
@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. |
@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
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