Skip to content
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

Print ParamArray attribute instead of 'params' #192

Closed
wants to merge 6 commits into from
Closed

Print ParamArray attribute instead of 'params' #192

wants to merge 6 commits into from

Conversation

dungpa
Copy link
Contributor

@dungpa dungpa commented Jan 31, 2015

Fixed #109.

Print System.ParamArray attribute to ensure that generated signature is valid F# code.

A few questions:

  • Is there any better approach than using attrib_ParamArrayAttribute? The attribute doesn't seem to be present in symbol metadata (there is only isParamArray flag in most cases).
  • Should intellisense be updated to use ParamArray attribute for consistency?

//<Expects status=success> end</Expects>

type Heterogeneous =
static member Echo([<System.ParamArray>] args: obj[]) = args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no newline at eof

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dsyme
Copy link
Contributor

dsyme commented Feb 3, 2015

The code looks good. Have you checked in the language service unit tests continue to pass?

@dungpa
Copy link
Contributor Author

dungpa commented Feb 4, 2015

There is no change in language service (yet). Tooltips still use the old style "params " i.e. https://github.com/Microsoft/visualfsharp/blob/468487fa52fce555928ccd1ba60f81b1cb56e93b/src/fsharp/vs/service.fs#L113. Do you think we should change display of tooltips as well?

@dsyme
Copy link
Contributor

dsyme commented Feb 4, 2015

No, it's ok, don't change the tooltips.

@dsyme
Copy link
Contributor

dsyme commented Feb 4, 2015

Could you point me to the part that means this change is not used when formatting types in the language service? (Just for me to understand)

thanks
don

@dungpa
Copy link
Contributor Author

dungpa commented Feb 4, 2015

Sorry, my mistake. The changes indeed have influence on language service. I corrected failing language service tests in the last commit.

@@ -919,7 +919,7 @@ type QuickInfoTests() =
type A() =
static member Foo([<System.ParamArrayAttribute>] a : int[]) = ()
let r = A.Foo(42)""" ,
"type A","params a:" )
"type A","[<ParamArray>] a:" )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the logic that determines whether System. is included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On types, it's always printed in the short form https://github.com/Microsoft/visualfsharp/blob/742c13304ce98a0af84bfcd5bf20afdbe3a74cdb/src/fsharp/vs/ServiceDeclarations.fs#L809. The check is at https://github.com/dungpa/visualfsharp/blob/93bd0997069713d8f0fed10d3af794bf66928e1c/src/fsharp/NicePrint.fs#L87-L88. It seems ok because this is displayed by language service, not FSI response or signature generation.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. In member intellisense, the module or namespace qualification is intelligently modified based on the current environment.

image

But yes, for hover tip on types, it seems that existing established behavior is to naively shorten, no matter what.

image

So I guess the inconsistency is not worth worrying about.

@latkin
Copy link
Contributor

latkin commented Feb 5, 2015

This test needs to be updated: fsharpqa\source -> Conformance\InferenceProcedures\TypeInference (E_LeftToRightOverloadResolution01.fs)

@@ -0,0 +1,11 @@
// #Regression #NoMT #Printing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I would rename this to .fsx since it's being run via FSI
  • As is, piping this into FSI actually results in FSI exit code of 1, which causes the test to fail.
    • You need to add #q at the end to get a 0 exit code

@dungpa
Copy link
Contributor Author

dungpa commented Feb 5, 2015

@latkin Thanks, done.

@latkin latkin closed this in ea11a9b Feb 6, 2015
//<Expects status="error" span="(7,23-7,51)" id="FS0041">A unique overload for method 'WriteLine' could not be determined based on type information prior to this program point\. A type annotation may be needed\. Candidates: System\.Console\.WriteLine\(buffer: char \[\]\) : unit, System\.Console\.WriteLine\(format: string, [<System.ParamArray>] arg: obj \[\]\) : unit, System\.Console\.WriteLine\(value: bool\) : unit, System\.Console\.WriteLine\(value: char\) : unit, System\.Console\.WriteLine\(value: decimal\) : unit, System\.Console\.WriteLine\(value: float\) : unit, System\.Console\.WriteLine\(value: float32\) : unit, System\.Console\.WriteLine\(value: int\) : unit, System\.Console\.WriteLine\(value: int64\) : unit, System\.Console\.WriteLine\(value: obj\) : unit, System\.Console\.WriteLine\(value: string\) : unit, System\.Console\.WriteLine\(value: uint32\) : unit, System\.Console\.WriteLine\(value: uint64\) : unit$</Expects>
//<Expects status="error" span="(9,23-9,51)" id="FS0041">A unique overload for method 'WriteLine' could not be determined based on type information prior to this program point\. A type annotation may be needed\. Candidates: System\.Console\.WriteLine\(buffer: char \[\]\) : unit, System\.Console\.WriteLine\(format: string, [<System.ParamArray>] arg: obj \[\]\) : unit, System\.Console\.WriteLine\(value: bool\) : unit, System\.Console\.WriteLine\(value: char\) : unit, System\.Console\.WriteLine\(value: decimal\) : unit, System\.Console\.WriteLine\(value: float\) : unit, System\.Console\.WriteLine\(value: float32\) : unit, System\.Console\.WriteLine\(value: int\) : unit, System\.Console\.WriteLine\(value: int64\) : unit, System\.Console\.WriteLine\(value: obj\) : unit, System\.Console\.WriteLine\(value: string\) : unit, System\.Console\.WriteLine\(value: uint32\) : unit, System\.Console\.WriteLine\(value: uint64\) : unit$</Expects>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected strings in these kinds of tests are actually regex. In this case I took care of escaping [<System.ParamArray>] to \[<System\.ParamArray>\]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help.

@latkin latkin added the fixed label Feb 6, 2015
@dungpa dungpa deleted the signature-printing branch September 7, 2015 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants