From 88e652f7a869b23605d43d25f672ec0ec5d03fc6 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Sun, 26 Jan 2020 19:00:02 -0800 Subject: [PATCH] internal/lsp/cache: improve completion after dangling selector Dangling selectors such as: func _() { x. } var x struct { i int } tend to wreak havoc on the AST. In the above example you didn't used to get completions because the declaration of "x" was missing from the AST. We now work around this issue by inserting a "_" into the source code before parsing to make the selector valid: func _() { x._ // <-- insert "_" here } var x struct { i int } This makes completion work as expected because the declaration of "x" is present in the AST. I had to change fixAST() to be called before fixSrc() because otherwise this new workaround in fixSrc() breaks the "accidental keyword" countermeasures in fixAST(). Fixes golang/go#31973. Updates golang/go#31687. Change-Id: Ia7ef6c045a9c71502d1b8b36f187ac9b8a85fe21 Reviewed-on: https://go-review.googlesource.com/c/tools/+/216484 Run-TryBot: Muir Manders TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/parse.go | 93 +++++++++++++++++-- .../lsp/testdata/lsp/primarymod/baz/baz.go.in | 6 +- .../danglingstmt/dangling_selector_1.go | 7 ++ .../danglingstmt/dangling_selector_2.go | 8 ++ internal/lsp/testdata/lsp/summary.txt.golden | 2 +- 5 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_1.go create mode 100644 internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_2.go diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index 1cc3a931825..816b89abedb 100644 --- a/internal/lsp/cache/parse.go +++ b/internal/lsp/cache/parse.go @@ -18,7 +18,6 @@ import ( "golang.org/x/tools/internal/lsp/telemetry" "golang.org/x/tools/internal/memoize" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/telemetry/log" "golang.org/x/tools/internal/telemetry/trace" errors "golang.org/x/xerrors" ) @@ -134,6 +133,9 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod return &parseGoData{err: errors.Errorf("successfully parsed but no token.File for %s (%v)", fh.Identity().URI, parseError)} } + // Fix any badly parsed parts of the AST. + _ = fixAST(ctx, file, tok, buf) + // Fix certain syntax errors that render the file unparseable. newSrc := fixSrc(file, tok, buf) if newSrc != nil { @@ -143,17 +145,14 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod file = newFile buf = newSrc tok = fset.File(file.Pos()) + + _ = fixAST(ctx, file, tok, buf) } } if mode == source.ParseExported { trimAST(file) } - - // Fix any badly parsed parts of the AST. - if err := fixAST(ctx, file, tok, buf); err != nil { - log.Error(ctx, "failed to fix AST", err) - } } if file == nil { @@ -301,6 +300,8 @@ func fixSrc(f *ast.File, tok *token.File, src []byte) (newSrc []byte) { switch n := n.(type) { case *ast.BlockStmt: newSrc = fixMissingCurlies(f, n, parent, tok, src) + case *ast.SelectorExpr: + newSrc = fixDanglingSelector(f, n, parent, tok, src) } return newSrc == nil @@ -390,6 +391,76 @@ func fixMissingCurlies(f *ast.File, b *ast.BlockStmt, parent ast.Node, tok *toke return buf.Bytes() } +// fixDanglingSelector inserts real "_" selector expressions in place +// of phantom "_" selectors. For example: +// +// func _() { +// x.<> +// } +// var x struct { i int } +// +// To fix completion at "<>", we insert a real "_" after the "." so the +// following declaration of "x" can be parsed and type checked +// normally. +func fixDanglingSelector(f *ast.File, s *ast.SelectorExpr, parent ast.Node, tok *token.File, src []byte) []byte { + if !isPhantomUnderscore(s.Sel, tok, src) { + return nil + } + + if !s.X.End().IsValid() { + return nil + } + + // Insert directly after the selector's ".". + insertOffset := tok.Offset(s.X.End()) + 1 + if src[insertOffset-1] != '.' { + return nil + } + + var buf bytes.Buffer + buf.Grow(len(src) + 1) + buf.Write(src[:insertOffset]) + buf.WriteByte('_') + buf.Write(src[insertOffset:]) + return buf.Bytes() +} + +// fixAccidentalDecl tries to fix "accidental" declarations. For example: +// +// func typeOf() {} +// type<> // want to call typeOf(), not declare a type +// +// If we find an *ast.DeclStmt with only a single phantom "_" spec, we +// replace the decl statement with an expression statement containing +// only the keyword. This allows completion to work to some degree. +func fixAccidentalDecl(decl *ast.DeclStmt, parent ast.Node, tok *token.File, src []byte) { + genDecl, _ := decl.Decl.(*ast.GenDecl) + if genDecl == nil || len(genDecl.Specs) != 1 { + return + } + + switch spec := genDecl.Specs[0].(type) { + case *ast.TypeSpec: + // If the name isn't a phantom "_" identifier inserted by the + // parser then the decl is likely legitimate and we shouldn't mess + // with it. + if !isPhantomUnderscore(spec.Name, tok, src) { + return + } + case *ast.ValueSpec: + if len(spec.Names) != 1 || !isPhantomUnderscore(spec.Names[0], tok, src) { + return + } + } + + replaceNode(parent, decl, &ast.ExprStmt{ + X: &ast.Ident{ + Name: genDecl.Tok.String(), + NamePos: decl.Pos(), + }, + }) +} + // fixPhantomSelector tries to fix selector expressions with phantom // "_" selectors. In particular, we check if the selector is a // keyword, and if so we swap in an *ast.Ident with the keyword text. For example: @@ -402,6 +473,16 @@ func fixPhantomSelector(sel *ast.SelectorExpr, tok *token.File, src []byte) { return } + // Only consider selectors directly abutting the selector ".". This + // avoids false positives in cases like: + // + // foo. // don't think "var" is our selector + // var bar = 123 + // + if sel.Sel.Pos() != sel.X.End()+1 { + return + } + maybeKeyword := readKeyword(sel.Sel.Pos(), tok, src) if maybeKeyword == "" { return diff --git a/internal/lsp/testdata/lsp/primarymod/baz/baz.go.in b/internal/lsp/testdata/lsp/primarymod/baz/baz.go.in index 4652e966edd..3b74ee580c3 100644 --- a/internal/lsp/testdata/lsp/primarymod/baz/baz.go.in +++ b/internal/lsp/testdata/lsp/primarymod/baz/baz.go.in @@ -20,13 +20,13 @@ func Baz() { func _() { bob := f.StructFoo{Value: 5} - if x := bob. //@complete(" //", Value) + if x := bob. //@complete(" //", Value) switch true == false { case true: - if x := bob. //@complete(" //", Value) + if x := bob. //@complete(" //", Value) case false: } - if x := bob.Va //@complete("a", Value) + if x := bob.Va //@complete("a", Value) switch true == true { default: } diff --git a/internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_1.go b/internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_1.go new file mode 100644 index 00000000000..772152f7b4f --- /dev/null +++ b/internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_1.go @@ -0,0 +1,7 @@ +package danglingstmt + +func _() { + x. //@rank(" //", danglingI) +} + +var x struct { i int } //@item(danglingI, "i", "int", "field") diff --git a/internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_2.go b/internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_2.go new file mode 100644 index 00000000000..a9e75e82a57 --- /dev/null +++ b/internal/lsp/testdata/lsp/primarymod/danglingstmt/dangling_selector_2.go @@ -0,0 +1,8 @@ +package danglingstmt + +import "golang.org/x/tools/internal/lsp/foo" + +func _() { + foo. //@rank(" //", Foo) + var _ = []string{foo.} //@rank("}", Foo) +} diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden index 4466d234652..43a4cda3b93 100644 --- a/internal/lsp/testdata/lsp/summary.txt.golden +++ b/internal/lsp/testdata/lsp/summary.txt.golden @@ -4,7 +4,7 @@ CompletionSnippetCount = 67 UnimportedCompletionsCount = 11 DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 -RankedCompletionsCount = 95 +RankedCompletionsCount = 98 CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 38 FoldingRangesCount = 2