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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 43 additions & 25 deletions src/fsharp/NicePrint.fs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ open Microsoft.FSharp.Compiler.Layout
open Microsoft.FSharp.Compiler.PrettyNaming

[<AutoOpen>]
module private PrintUtilities =
module internal PrintUtilities =
let bracketIfL x lyt = if x then bracketL lyt else lyt
let squareAngleL x = leftL "[<" ^^ x ^^ rightL ">]"
let angleL x = sepL "<" ^^ x ^^ rightL ">"
Expand Down Expand Up @@ -70,6 +70,37 @@ module private PrintUtilities =
| [x] -> [resultFunction x (layoutFunction x)]
| (x:: rest) -> [ resultFunction x (layoutFunction x -- leftL (match rest.Length with 1 -> FSComp.SR.nicePrintOtherOverloads1() | n -> FSComp.SR.nicePrintOtherOverloadsN(n))) ]
| _ -> []

let layoutTyconRefImpl isAttribute (denv: DisplayEnv) (tcref:TyconRef) =
let demangled =
let name =
if denv.includeStaticParametersInTypeNames then
tcref.DisplayNameWithStaticParameters
elif tcref.DisplayName = tcref.DisplayNameWithStaticParameters then
tcref.DisplayName // has no static params
else
tcref.DisplayName+"<...>" // shorten
if isAttribute then
defaultArg (String.tryDropSuffix name "Attribute") name
else name
let tyconTextL = wordL demangled
if denv.shortTypeNames then
tyconTextL
else
let path = demangledPathOfCompPath tcref.CompilationPath
let path =
Copy link
Contributor

Choose a reason for hiding this comment

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

used same bound name path, use another instead of replace path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are subjective opinions on value shadowing. Since the code is moved from another place in the file, I will keep it as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's fine. FWIW there is plenty of shadowing in the compiler in any case

Copy link
Contributor

Choose a reason for hiding this comment

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

@dungpa I have not seen that code was moved only.
btw @dsyme about shadowing (asking, like for <| on code review ) is it usually ok? or we should avoit it for new code ( not moved code )

if denv.includeStaticParametersInTypeNames then
path
else
path |> List.map (fun s -> let i = s.IndexOf(',')
if i <> -1 then s.Substring(0,i)+"<...>" // apparently has static params, shorten
else s)
let pathText = trimPathByDisplayEnv denv path
if pathText = "" then tyconTextL else leftL pathText ^^ tyconTextL

let layoutBuiltinAttribute (denv: DisplayEnv) (attrib: BuiltinAttribInfo) =
let tcref = attrib.TyconRef
squareAngleL (layoutTyconRefImpl true denv tcref)

module private PrintIL =

Expand Down Expand Up @@ -185,7 +216,8 @@ module private PrintIL =
// Layout an unnamed argument
| _, None, _ -> leftL ":"
// Layout a named argument
| true, Some nm,_ -> wordL "params" ^^ leftL (nm+":")
| true, Some nm,_ ->
layoutBuiltinAttribute denv denv.g.attrib_ParamArrayAttribute ^^ leftL (nm + ":")
| false, Some nm,_ -> leftL (nm+":")
preL ^^ (layoutILType denv ilTyparSubst p.Type)

Expand Down Expand Up @@ -550,27 +582,7 @@ module private PrintTypes =
| _ -> itemL

/// Layout a reference to a type
let layoutTyconRef (denv: DisplayEnv) (tcref:TyconRef) =
let demangled = if denv.includeStaticParametersInTypeNames then
tcref.DisplayNameWithStaticParameters
elif tcref.DisplayName = tcref.DisplayNameWithStaticParameters then
tcref.DisplayName // has no static params
else
tcref.DisplayName+"<...>" // shorten
let tyconTextL = wordL demangled
if denv.shortTypeNames then
tyconTextL
else
let path = demangledPathOfCompPath tcref.CompilationPath
let path =
if denv.includeStaticParametersInTypeNames then
path
else
path |> List.map (fun s -> let i = s.IndexOf(',')
if i <> -1 then s.Substring(0,i)+"<...>" // apparently has static params, shorten
else s)
let pathText = trimPathByDisplayEnv denv path
if pathText = "" then tyconTextL else leftL pathText ^^ tyconTextL
let layoutTyconRef denv tycon = layoutTyconRefImpl false denv tycon

/// Layout the flags of a member
let layoutMemberFlags memFlags =
Expand Down Expand Up @@ -918,7 +930,12 @@ module private PrintTypes =
layoutTypeWithInfoAndPrec denv env 2 ty
// Layout a named argument
| Some id,_,isParamArray,_ ->
leftL ((if isParamArray then "params " else "") + id.idText) ^^ sepL ":" ^^ layoutTypeWithInfoAndPrec denv env 2 ty
let prefix =
if isParamArray then
layoutBuiltinAttribute denv denv.g.attrib_ParamArrayAttribute ^^ leftL id.idText
else
leftL id.idText
prefix ^^ sepL ":" ^^ layoutTypeWithInfoAndPrec denv env 2 ty

let delimitReturnValue = if denv.useColonForReturnType then ":" else "->"

Expand Down Expand Up @@ -1147,7 +1164,8 @@ module InfoMemberPrinting =
outputTy denv os pty;
// Layout a named argument
| true, Some nm,_,_ ->
bprintf os "params %s: " nm
layoutBuiltinAttribute denv denv.g.attrib_ParamArrayAttribute |> bufferL os
bprintf os " %s: " nm
outputTy denv os pty
| false, Some nm,_,_ ->
bprintf os "%s: " nm
Expand Down
6 changes: 5 additions & 1 deletion src/fsharp/vs/service.fs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,11 @@ module internal Params =
"", "", pty
// Layout a named argument
| Some nm,_,_ ->
let prefix = if isParamArrayArg then sprintf "params %s: " nm else sprintf "%s: " nm
let prefix =
if isParamArrayArg then
sprintf "%s %s: " (NicePrint.PrintUtilities.layoutBuiltinAttribute denv denv.g.attrib_ParamArrayAttribute |> showL) nm
else
sprintf "%s: " nm
nm, prefix,pty)
|> List.unzip3
let paramTypeAndRetLs,_ = NicePrint.layoutPrettifiedTypes denv (paramTypes@[rty])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ Array.iter (fun it -> System.Console.WriteLine(it)) array

array |> Array.iter (fun it -> System.Console.WriteLine(it))

//<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, params 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, params 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="(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.

13 changes: 13 additions & 0 deletions tests/fsharpqa/Source/Printing/ParamArrayInSignatures.fsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// #Regression #NoMT #Printing
// Regression test for https://github.com/Microsoft/visualfsharp/issues/109

// pretty printing signatures with params arguments
//<Expects status=success>type Heterogeneous =</Expects>
//<Expects status=success> class</Expects>
//<Expects status=success> static member Echo : \[<System.ParamArray>\] args:obj \[\] -> obj \[\]</Expects>
//<Expects status=success> end</Expects>

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

#q;;
1 change: 1 addition & 0 deletions tests/fsharpqa/Source/Printing/env.lst
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,4 @@ ReqENU SOURCE=LazyValues03NetFx4.fsx COMPILE_ONLY=1 SCFLAGS="--nologo" FSIMODE=P

SOURCE=WidthForAFormatter.fs # WidthForAFormatter.fs
SOURCE=ToStringOnCollections.fs # ToStringOnCollections.fs
SOURCE=ParamArrayInSignatures.fsx SCFLAGS="--nologo" COMPILE_ONLY=1 FSIMODE=PIPE # ParamArrayInSignatures.fsx
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ let x =
CheckErrorList content <|
fun errors ->
Assert.AreEqual(1, List.length errors)
assertContains errors "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, params arg: obj []) : unit, System.Console.WriteLine(value: obj) : unit, System.Console.WriteLine(value: string) : unit"
assertContains errors "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: obj) : unit, System.Console.WriteLine(value: string) : unit"

[<Test>]
member public this.``InvalidMethodOverload2``() =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1745,7 +1745,7 @@ We really need to rewrite some code paths here to use the real parse tree rather
)
|> Seq.tryFind(fun (i, _) -> i = 2)
match overloadWithTwoParamsOpt with
| Some(_, [_;(_name, display, _description)]) -> Assert.IsTrue(display.Contains("params args"))
| Some(_, [_;(_name, display, _description)]) -> Assert.IsTrue(display.Contains("[<System.ParamArray>] args"))
| x -> Assert.Fail(sprintf "Expected overload not found, current result %A" x)

(* DotNet functions for multi-parameterinfo tests -------------------------------------------------- *)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ type QuickInfoTests() =
let fileContents = """
let t = "a".Split('c')"""

this.AssertQuickInfoContainsAtEndOfMarker (fileContents, "Spl", "params separator")
this.AssertQuickInfoContainsAtEndOfMarker (fileContents, "Spl", "[<System.ParamArray>] separator")

[<Test>]
[<Category("TypeProvider")>]
Expand Down Expand Up @@ -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.


[<Test>]
member public this.``ParamsArrayArgument.OnMethod``() =
Expand All @@ -928,7 +928,7 @@ type QuickInfoTests() =
type A() =
static member Foo([<System.ParamArrayAttribute>] a : int[]) = ()
let r = A.Foo(42)""" ,
"A.Foo","params a:" )
"A.Foo","[<System.ParamArray>] a:" )

[<Test>]
member public this.``Regression.AccessorMutator.Bug4903e``() =
Expand Down