- 
                Notifications
    You must be signed in to change notification settings 
- Fork 56
Fully support argument matching #120
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
Now supports no args, curried args, single args, new expressions, attribute constructors and more
# Conflicts: # ReSharper.FSharp/src/FSharp.Psi.Features/src/Util/FSharpMethodInvocationUtil.fs
Named arg error recovery is not yet working, hence the missing gold files
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 looks gorgeous!
I've skimmed through it and left some comments, will do a closer review later.
        
          
                ReSharper.FSharp/src/FSharp.Psi.Features/src/Util/FSharpMethodInvocationUtil.fs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ReSharper.FSharp/test/src/FSharp.Tests/Parsing/ArgumentsOwnerTest.fs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ReSharper.FSharp/test/src/FSharp.Tests/Parsing/ArgumentsOwnerTest.fs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | [<FSharpTest>] | ||
| [<TestPackages("FSharp.Core")>] | ||
| [<TestReferences("System.Drawing", "System", "System.Core")>] | ||
| [<TestProjectOutputType(ProjectOutputType.CONSOLE_EXE)>] | 
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.
I think it'd be great to add ITestProjectOutputTypeProviderAttribute to the framework and to make FSharpTestAttribute implement it, defaulting to ProjectOutputType.CONSOLE_EXE. It'll allow override it on per-test basis but having Exe default will save us from having to add module Module everywhere. I'll look into it.
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.
I'll do that when I split the test infra changes to be their own PR 👍
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.
And a few comments for tests.
        
          
                ReSharper.FSharp/test/data/parsing/arguments/Multiple 06 - Tupled - too many.gold
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ReSharper.FSharp/test/data/parsing/arguments/Attribute 05 - No args - parens.gold
          
            Show resolved
            Hide resolved
        
              
          
                ReSharper.FSharp/test/data/parsing/arguments/Extension CSharp 03 - Tupled args.gold
              
                Outdated
          
            Show resolved
            Hide resolved
        
      |  | ||
| dispose { new IDisposable with override __.Dispose () = () } | ||
|  | ||
| --------------------------------------------------------- | 
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.
We should make NewExpr implement IArgument too (not necessarily as a part of this PR but still).
|  | ||
| --------------------------------------------------------- | ||
| (arg #0) => <no matching param> | ||
| (arg #1) => <no matching param> | 
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.
I wonder if we could somehow make parameter groups for curried functions and match a here later.
This reverts commit 3cba158.
| type T() = | ||
| static member M(u: unit) = () | ||
|  | ||
| {selstart}T.M(u = ()){selend} | 
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.
Thanks for adding the test case! If we're to change it later, we'll have to update the recorded behavior explicitly, which is good.
Non-user-facing change which will enable parameter name hints. I think I've covered most of the ways you could call a function in F#.
Looking for feedback on how I've implemented it. See the 'todo's mentioned in the code. There are definite caching improvements (e.g. caching arguments and caching mapping from argument => parameter) that can be added but I don't know the idiomatic way to implement them.
Todo:
todos in codeExpectsErrorsattribute/test framework feature into its own PR - see Add FSharpTestCheckerService to supportExpectErrorsAttributein tests #121