"System.Func overload-is-better": checking if anything breaks, or not (in which case, we'd like a test) #11538
Conversation
…running (well, at least compile...).
|
Mmh, unless I miss something, this being green may indicate we basically have a hole in the tests in terms of asserting overloading with func-is-better holds. The thing I could be missing is that all the tests are run with |
|
So, it seems in F# compiler codebase and tests, beside the test I've added in this PR, there is nothing which checks for "func-is-better". This doesn't make me super confident in the test suite having nearly enough checks on the nitty gritty of overload resolution. I'll try to hack a bit on having the warning, if that proves simple enough, it could turn this PR into fixing #11534 and prepare the ground for removing the support for "func-is-better" in a later major version. @dsyme, would you mind sharing a bit here or in #11534 some of the historical reasons for "func-is-better" in overload resolution? and if you are favorable with a warning wave and eventual deprecation of that special casing? On my end, for sake of explicitness and principle of least surprise, treating all delegate types the same, I'd favor taking this out in next major release (could even do this under preview guards soon), and later, work on addressing issues such as highlighted in #2503 (comment). |
… is a lambda literal. In such case, when we hit the func-is-better, we display a warning.
|
This is green and ready for a first round of review. |
| @@ -508,6 +508,29 @@ type CalledMeth<'T> | |||
| /// The argument analysis for each set of curried arguments | |||
| member x.ArgSets = argSets | |||
|
|
|||
| member x.GetCallerArgAt index = | |||
| // Arrays are better... | |||
| let mutable currentIndex = 0 | |||
There was a problem hiding this comment.
I'll be looking at reimplementing this in terms of List.mapi2, this has been first try to make things work (it passes the test, but the test likely doesn't covers all possible cases).
There was a problem hiding this comment.
This code is problematic, I wrote it before surveying some more my assumptions (in #11544, which shows that my assumptions are just wrong).
Looking for guidance on how we want that to work to get this PR considered for merge.
Also eager to hear if there is a point or no, to move from list list representation of SynValInfo, I know @baronfel was facing slightly related question recently:
anyone have any insight into why a named tuple-parameter is erased in the TAST? eg
let myFun (f: (int * int * int)) = ()
in the TAST is represented as an FSharpMemberOrFunctionOrValue, whose CurriedParameterArgs is a list< list< FSharpParameter > > of length 1, whose inner list is of length 3. The three inner parameters are unnamed, but as a result of this form I lose information (as an IDE user) about the name/documentation of the parameter.
which shows that the representation as we have it, from the parser AST down to the typed AST is a bit arcane to the people not used to it.
| @@ -0,0 +1,16 @@ | |||
|
|
|||
| neg_at_somepoint_in_the_future_func-is-better-or-not.fsx(14,1,14,17): typecheck error FS0193: The method 'M' has overload with 'System.Func' delegate in the signature, to fix the warning, you need to specify the type explicitly. | |||
There was a problem hiding this comment.
wrt to the warning message itself:
- there may be potential to show the
CallerArg.Exprexpression, if there are existing facilities to prettify it - it might be worth to mention which argument (by the name of it from
CalledArg) is causing the warning
There was a problem hiding this comment.
Specifying the argument name may be problematic in circumstances similar to what @baronfel was dealing with, patterns and "anonymous" parameters.
If there are places we already prettify Expr to display to the end user, please let me know and I'll try to experiment with it.
Probably several error messages could benefit from having this kind of "expanding the expression to explain the error in actual case".
|
Bump @cartermp |
|
@smoothdeveloper Could you include the XLF file updates as well please (see DEVGUIDE.md) |
|
There are merge conflicts and test failures that needs to be resolved. |
|
@smoothdeveloper Please feel free to re-open if you wish to continue to work on this PR. Kevin |
|
@KevinRansom, no issue, we could scope it for #15901, I think the implementation for it didn't give me too much trouble, so I could give it on top of the then-to-be-current implementation. |
Related to #11534 and prompted by excellent suggestion from @baronfel to apply the warning wave protocol (as was done with XML docs warnings) to tackle issues around "System.Func overload is better".
This PR is, for now, just checking if we have tests that would fail due to this feature of the compiler, and potentially adding one if not.