Skip to content

Better completion #3940

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Nov 19, 2017
Merged

Better completion #3940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
29cff48
suggests types and modules only at pattern type position (x: ...)
vasily-kirichenko Nov 13, 2017
c22dcf3
handle all places where type can be used
vasily-kirichenko Nov 14, 2017
c465d73
Merge remote-tracking branch 'origin/master' into better-completion
Nov 15, 2017
8251a31
fix
vasily-kirichenko Nov 15, 2017
28b9357
union cases should not appear in completion at type hint position
vasily-kirichenko Nov 15, 2017
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
62 changes: 57 additions & 5 deletions src/fsharp/vs/ServiceParseTreeWalk.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,15 @@ module internal AstTraversal =
/// VisitLetOrUse allows overriding behavior when visiting module or local let or use bindings
abstract VisitLetOrUse : SynBinding list * range -> 'T option
default this.VisitLetOrUse (_, _) = None

/// VisitType allows overriding behavior when visiting simple pats
abstract VisitSimplePats : SynSimplePat list -> 'T option
default this.VisitSimplePats (_) = None
/// VisitPat allows overriding behavior when visiting patterns
abstract VisitPat : (SynPat -> 'T option) * SynPat -> 'T option
default this.VisitPat (defaultTraverse, pat) = defaultTraverse pat
/// VisitType allows overriding behavior when visiting type hints (x: ..., etc.)
abstract VisitType : (SynType -> 'T option) * SynType -> 'T option
default this.VisitType (defaultTraverse, ty) = defaultTraverse ty

let dive node range project =
range,(fun() -> project node)
Expand Down Expand Up @@ -189,12 +195,12 @@ module internal AstTraversal =
dive synExpr2 synExpr2.Range traverseSynExpr]
|> pick expr
| SynExpr.Const(_synConst, _range) -> None
| SynExpr.Typed(synExpr, _synType, _range) -> traverseSynExpr synExpr
| SynExpr.Typed(synExpr, synType, _range) -> [ traverseSynExpr synExpr; traverseSynType synType ] |> List.tryPick id
| SynExpr.Tuple(synExprList, _, _range)
| SynExpr.StructTuple(synExprList, _, _range) -> synExprList |> List.map (fun x -> dive x x.Range traverseSynExpr) |> pick expr
| SynExpr.ArrayOrList(_, synExprList, _range) -> synExprList |> List.map (fun x -> dive x x.Range traverseSynExpr) |> pick expr
| SynExpr.Record(inheritOpt,copyOpt,fields, _range) ->
[
[
let diveIntoSeparator offsideColumn scPosOpt copyOpt =
match scPosOpt with
| Some scPos ->
Expand Down Expand Up @@ -448,6 +454,50 @@ module internal AstTraversal =

visitor.VisitExpr(path, traverseSynExpr path, defaultTraverse, expr)

and traversePat (pat: SynPat) =
let defaultTraverse p =
match p with
| SynPat.Paren (p, _) -> traversePat p
| SynPat.Or (p1, p2, _) -> [ p1; p2] |> List.tryPick traversePat
| SynPat.Ands (ps, _)
| SynPat.Tuple (ps, _)
| SynPat.StructTuple (ps, _)
| SynPat.ArrayOrList (_, ps, _) -> ps |> List.tryPick traversePat
| SynPat.Attrib (p, _, _) -> traversePat p
| SynPat.LongIdent(_, _, _, args, _, _) ->
match args with
| SynConstructorArgs.Pats ps -> ps |> List.tryPick traversePat
| SynConstructorArgs.NamePatPairs (ps, _) ->
ps |> List.map snd |> List.tryPick traversePat
| SynPat.Typed (p, ty, _) ->
[ traversePat p; traverseSynType ty ] |> List.tryPick id
| _ -> None

visitor.VisitPat (defaultTraverse, pat)

and traverseSynType (ty: SynType) =
let defaultTraverse ty =
match ty with
| SynType.App (typeName, _, typeArgs, _, _, _, _)
| SynType.LongIdentApp (typeName, _, _, typeArgs, _, _, _) ->
[ yield typeName
yield! typeArgs ]
|> List.tryPick traverseSynType
| SynType.Fun (ty1, ty2, _) -> [ty1; ty2] |> List.tryPick traverseSynType
| SynType.MeasurePower (ty, _, _)
| SynType.HashConstraint (ty, _)
| SynType.WithGlobalConstraints (ty, _, _)
| SynType.Array (_, ty, _) -> traverseSynType ty
| SynType.StaticConstantNamed (ty1, ty2, _)
| SynType.MeasureDivide (ty1, ty2, _) -> [ty1; ty2] |> List.tryPick traverseSynType
| SynType.Tuple (tys, _)
| SynType.StructTuple (tys, _) -> tys |> List.map snd |> List.tryPick traverseSynType
| SynType.StaticConstantExpr (expr, _) -> traverseSynExpr [] expr
| SynType.Anon _ -> None
| _ -> None

visitor.VisitType (defaultTraverse, ty)

and normalizeMembersToDealWithPeculiaritiesOfGettersAndSetters path traverseInherit (synMemberDefns:SynMemberDefns) =
synMemberDefns
// property getters are setters are two members that can have the same range, so do some somersaults to deal with this
Expand Down Expand Up @@ -566,8 +616,10 @@ module internal AstTraversal =
let defaultTraverse b =
let path = TraverseStep.Binding b :: path
match b with
| (SynBinding.Binding(_synAccessOption, _synBindingKind, _, _, _synAttributes, _preXmlDoc, _synValData, _synPat, _synBindingReturnInfoOption, synExpr, _range, _sequencePointInfoForBinding)) ->
traverseSynExpr path synExpr
| (SynBinding.Binding(_synAccessOption, _synBindingKind, _, _, _synAttributes, _preXmlDoc, _synValData, synPat, _synBindingReturnInfoOption, synExpr, _range, _sequencePointInfoForBinding)) ->
[ traversePat synPat
traverseSynExpr path synExpr ]
|> List.tryPick id
visitor.VisitBinding(defaultTraverse,b)

match parseTree with
Expand Down
8 changes: 8 additions & 0 deletions src/fsharp/vs/ServiceUntypedParse.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ type CompletionContext =
| ParameterList of pos * HashSet<string>
| AttributeApplication
| OpenDeclaration
/// completing pattern type (e.g. foo (x: |))
| PatternType
Copy link
Contributor

@cartermp cartermp Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should this be named ParameterTypeAnnotation or TypeAnnotation?

Copy link
Contributor

@cartermp cartermp Nov 16, 2017

Choose a reason for hiding this comment

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

Looks like TypeAnnotation would be the better candidate of my two suggestions since it is also hit here:

let foo (s: string): ^

Copy link
Contributor Author

@vasily-kirichenko vasily-kirichenko Nov 16, 2017

Choose a reason for hiding this comment

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

No, this is for xx: type at any place, not just for parameters. And this thing is called pattern I think.

Copy link
Contributor

@cartermp cartermp Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I think what you're referring to is the patterns that have type annotations, so PatternWithTypeAnnotation would be the name I'd think of here. That may have been what you meant, but when I read PatternType I thought it was referring to a completion context which constituted a pattern of any kind.

Copy link
Contributor

@smoothdeveloper smoothdeveloper Nov 16, 2017

Choose a reason for hiding this comment

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

I believe Pattern refers to the bound pattern (when deconstructing a DU, a tuple, a record, etc.) and Type to the type annotation.

let foo ((Some s): string option) = ...

parameter can be a simple symbol but also can be a pattern.


//----------------------------------------------------------------------------
// FSharpParseFileResults
Expand Down Expand Up @@ -1272,6 +1274,12 @@ module UntypedParseImpl =
else
None
| _ -> defaultTraverse decl

member __.VisitType(defaultTraverse, ty) =
match ty with
| SynType.LongIdent _ when rangeContainsPos ty.Range pos ->
Some CompletionContext.PatternType
| _ -> defaultTraverse ty
}

AstTraversal.Traverse(pos, pt, walker)
Expand Down
2 changes: 2 additions & 0 deletions src/fsharp/vs/ServiceUntypedParse.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ type internal CompletionContext =
| ParameterList of pos * HashSet<string>
| AttributeApplication
| OpenDeclaration
/// completing pattern type (e.g. foo (x: |))
| PatternType

#if COMPILER_PUBLIC_API
type ModuleKind = { IsAutoOpen: bool; HasModuleSuffix: bool }
Expand Down
13 changes: 13 additions & 0 deletions src/fsharp/vs/service.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,19 @@ type TypeCheckInfo
|> Option.map (fun (items, denv, m) ->
items |> List.filter (fun x -> match x.Item with Item.ModuleOrNamespaces _ -> true | _ -> false), denv, m)

// Completion at '(x: ...)"
| Some (CompletionContext.PatternType) ->
GetDeclaredItems (parseResultsOpt, lineStr, origLongIdentOpt, colAtEndOfNamesAndResidue, residueOpt, lastDotPos, line, loc, filterCtors, resolveOverloads, hasTextChangedSinceLastTypecheck, false, getAllSymbols)
|> Option.map (fun (items, denv, m) ->
items
|> List.filter (fun cItem ->
match cItem.Item with
| Item.ModuleOrNamespaces _
| Item.Types _
| Item.UnqualifiedType _
| Item.ExnCase _ -> true
| _ -> false), denv, m)

// Other completions
| cc ->
match residueOpt |> Option.bind Seq.tryHead with
Expand Down
2 changes: 1 addition & 1 deletion vsintegration/tests/unittests/Tests.LanguageService.Completion.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6064,7 +6064,7 @@ let rec f l =
let f (x:MyNamespace1.MyModule(*Maftervariable4*)) = 10
let y = int System.IO(*Maftervariable5*)""",
marker = "(*Maftervariable4*)",
list = ["DuType";"Tag"])
list = ["DuType"])

[<Test>]
member this.``VariableIdentifier.SystemNamespace``() =
Expand Down