internal/lsp/source/completion: suggest only valid package names
Before this change directory names were used "as is" for package completion.
It could lead to invalid suggestions (for example, 'package 1abc' or package 'ab-cd').
This change adds a check whether a directory name can be used in a package path.
If the directory name is invalid, only 'package main' will be suggested.
Otherwise, the directory name will be normalized and will be used as a package name.
Note: normalized directory names contain only lower case letters and digits.
Fixes golang/go#44680
Change-Id: I4b710f90d1723c512e29dc3c248a1e681f1cd37f
GitHub-Last-Rev: 8ae69f1c6fdf80831e5773bdb3507a8d51a4a0cf
GitHub-Pull-Request: golang/tools#310
Reviewed-on: https://go-review.googlesource.com/c/tools/+/313092
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Peter Weinberger <pjw@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go
index 7984095..a88e861 100644
--- a/gopls/internal/regtest/completion/completion_test.go
+++ b/gopls/internal/regtest/completion/completion_test.go
@@ -70,6 +70,11 @@
-- fruits/testfile3.go --
pac
+-- 123f_r.u~its-123/testfile.go --
+package
+
+-- .invalid-dir@-name/testfile.go --
+package
`
var (
testfile4 = ""
@@ -147,6 +152,21 @@
want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"},
editRegexp: `\*\/\n()`,
},
+ // Issue golang/go#44680
+ {
+ name: "package completion for dir name with punctuation",
+ filename: "123f_r.u~its-123/testfile.go",
+ triggerRegexp: "package()",
+ want: []string{"package fruits123", "package fruits123_test", "package main"},
+ editRegexp: "package\n",
+ },
+ {
+ name: "package completion for invalid dir name",
+ filename: ".invalid-dir@-name/testfile.go",
+ triggerRegexp: "package()",
+ want: []string{"package main"},
+ editRegexp: "package\n",
+ },
} {
t.Run(tc.name, func(t *testing.T) {
Run(t, files, func(t *testing.T, env *Env) {
diff --git a/internal/lsp/source/completion/package.go b/internal/lsp/source/completion/package.go
index 483223a..aea414e 100644
--- a/internal/lsp/source/completion/package.go
+++ b/internal/lsp/source/completion/package.go
@@ -5,6 +5,7 @@
package completion
import (
+ "bytes"
"context"
"fmt"
"go/ast"
@@ -14,6 +15,7 @@
"go/types"
"path/filepath"
"strings"
+ "unicode"
"golang.org/x/tools/internal/lsp/fuzzy"
"golang.org/x/tools/internal/lsp/protocol"
@@ -207,17 +209,12 @@
// have the given prefix and are used in the same directory as the given
// file. This also includes test packages for these packages (<pkg>_test) and
// the directory name itself.
-func packageSuggestions(ctx context.Context, snapshot source.Snapshot, fileURI span.URI, prefix string) ([]candidate, error) {
+func packageSuggestions(ctx context.Context, snapshot source.Snapshot, fileURI span.URI, prefix string) (packages []candidate, err error) {
workspacePackages, err := snapshot.WorkspacePackages(ctx)
if err != nil {
return nil, err
}
- dirPath := filepath.Dir(string(fileURI))
- dirName := filepath.Base(dirPath)
-
- seenPkgs := make(map[string]struct{})
-
toCandidate := func(name string, score float64) candidate {
obj := types.NewPkgName(0, nil, name, types.NewPackage("", name))
return candidate{obj: obj, name: name, detail: name, score: score}
@@ -225,9 +222,24 @@
matcher := fuzzy.NewMatcher(prefix)
+ // Always try to suggest a main package
+ defer func() {
+ if score := float64(matcher.Score("main")); score > 0 {
+ packages = append(packages, toCandidate("main", score*lowScore))
+ }
+ }()
+
+ dirPath := filepath.Dir(fileURI.Filename())
+ dirName := filepath.Base(dirPath)
+ if !isValidDirName(dirName) {
+ return packages, nil
+ }
+ pkgName := convertDirNameToPkgName(dirName)
+
+ seenPkgs := make(map[string]struct{})
+
// The `go` command by default only allows one package per directory but we
// support multiple package suggestions since gopls is build system agnostic.
- var packages []candidate
for _, pkg := range workspacePackages {
if pkg.Name() == "main" || pkg.Name() == "" {
continue
@@ -239,7 +251,7 @@
// Only add packages that are previously used in the current directory.
var relevantPkg bool
for _, pgf := range pkg.CompiledGoFiles() {
- if filepath.Dir(string(pgf.URI)) == dirPath {
+ if filepath.Dir(pgf.URI.Filename()) == dirPath {
relevantPkg = true
break
}
@@ -267,20 +279,80 @@
}
// Add current directory name as a low relevance suggestion.
- if _, ok := seenPkgs[dirName]; !ok {
- if score := float64(matcher.Score(dirName)); score > 0 {
- packages = append(packages, toCandidate(dirName, score*lowScore))
+ if _, ok := seenPkgs[pkgName]; !ok {
+ if score := float64(matcher.Score(pkgName)); score > 0 {
+ packages = append(packages, toCandidate(pkgName, score*lowScore))
}
- testDirName := dirName + "_test"
- if score := float64(matcher.Score(testDirName)); score > 0 {
- packages = append(packages, toCandidate(testDirName, score*lowScore))
+ testPkgName := pkgName + "_test"
+ if score := float64(matcher.Score(testPkgName)); score > 0 {
+ packages = append(packages, toCandidate(testPkgName, score*lowScore))
}
}
- if score := float64(matcher.Score("main")); score > 0 {
- packages = append(packages, toCandidate("main", score*lowScore))
- }
-
return packages, nil
}
+
+// isValidDirName checks whether the passed directory name can be used in
+// a package path. Requirements for a package path can be found here:
+// https://golang.org/ref/mod#go-mod-file-ident.
+func isValidDirName(dirName string) bool {
+ if dirName == "" {
+ return false
+ }
+
+ for i, ch := range dirName {
+ if isLetter(ch) || isDigit(ch) {
+ continue
+ }
+ if i == 0 {
+ // Directory name can start only with '_'. '.' is not allowed in module paths.
+ // '-' and '~' are not allowed because elements of package paths must be
+ // safe command-line arguments.
+ if ch == '_' {
+ continue
+ }
+ } else {
+ // Modules path elements can't end with '.'
+ if isAllowedPunctuation(ch) && (i != len(dirName)-1 || ch != '.') {
+ continue
+ }
+ }
+
+ return false
+ }
+ return true
+}
+
+// convertDirNameToPkgName converts a valid directory name to a valid package name.
+// It leaves only letters and digits. All letters are mapped to lower case.
+func convertDirNameToPkgName(dirName string) string {
+ var buf bytes.Buffer
+ for _, ch := range dirName {
+ switch {
+ case isLetter(ch):
+ buf.WriteRune(unicode.ToLower(ch))
+
+ case buf.Len() != 0 && isDigit(ch):
+ buf.WriteRune(ch)
+ }
+ }
+ return buf.String()
+}
+
+// isLetter and isDigit allow only ASCII characters because
+// "Each path element is a non-empty string made of up ASCII letters,
+// ASCII digits, and limited ASCII punctuation"
+// (see https://golang.org/ref/mod#go-mod-file-ident).
+
+func isLetter(ch rune) bool {
+ return 'a' <= ch && ch <= 'z' || 'A' <= ch && ch <= 'Z'
+}
+
+func isDigit(ch rune) bool {
+ return '0' <= ch && ch <= '9'
+}
+
+func isAllowedPunctuation(ch rune) bool {
+ return ch == '_' || ch == '-' || ch == '~' || ch == '.'
+}
diff --git a/internal/lsp/source/completion/package_test.go b/internal/lsp/source/completion/package_test.go
new file mode 100644
index 0000000..6436984
--- /dev/null
+++ b/internal/lsp/source/completion/package_test.go
@@ -0,0 +1,77 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package completion
+
+import "testing"
+
+func TestIsValidDirName(t *testing.T) {
+ tests := []struct {
+ dirName string
+ valid bool
+ }{
+ {dirName: "", valid: false},
+ //
+ {dirName: "a", valid: true},
+ {dirName: "abcdef", valid: true},
+ {dirName: "AbCdEf", valid: true},
+ //
+ {dirName: "1a35", valid: true},
+ {dirName: "a16", valid: true},
+ //
+ {dirName: "_a", valid: true},
+ {dirName: "a_", valid: true},
+ //
+ {dirName: "~a", valid: false},
+ {dirName: "a~", valid: true},
+ //
+ {dirName: "-a", valid: false},
+ {dirName: "a-", valid: true},
+ //
+ {dirName: ".a", valid: false},
+ {dirName: "a.", valid: false},
+ //
+ {dirName: "a~_b--c.-e", valid: true},
+ {dirName: "~a~_b--c.-e", valid: false},
+ {dirName: "a~_b--c.-e--~", valid: true},
+ {dirName: "a~_b--2134dc42.-e6--~", valid: true},
+ {dirName: "abc`def", valid: false},
+ {dirName: "тест", valid: false},
+ {dirName: "你好", valid: false},
+ }
+ for _, tt := range tests {
+ valid := isValidDirName(tt.dirName)
+ if tt.valid != valid {
+ t.Errorf("%s: expected %v, got %v", tt.dirName, tt.valid, valid)
+ }
+ }
+}
+
+func TestConvertDirNameToPkgName(t *testing.T) {
+ tests := []struct {
+ dirName string
+ pkgName string
+ }{
+ {dirName: "a", pkgName: "a"},
+ {dirName: "abcdef", pkgName: "abcdef"},
+ {dirName: "AbCdEf", pkgName: "abcdef"},
+ {dirName: "1a35", pkgName: "a35"},
+ {dirName: "14a35", pkgName: "a35"},
+ {dirName: "a16", pkgName: "a16"},
+ {dirName: "_a", pkgName: "a"},
+ {dirName: "a_", pkgName: "a"},
+ {dirName: "a~", pkgName: "a"},
+ {dirName: "a-", pkgName: "a"},
+ {dirName: "a~_b--c.-e", pkgName: "abce"},
+ {dirName: "a~_b--c.-e--~", pkgName: "abce"},
+ {dirName: "a~_b--2134dc42.-e6--~", pkgName: "ab2134dc42e6"},
+ }
+ for _, tt := range tests {
+ pkgName := convertDirNameToPkgName(tt.dirName)
+ if tt.pkgName != pkgName {
+ t.Errorf("%s: expected %v, got %v", tt.dirName, tt.pkgName, pkgName)
+ continue
+ }
+ }
+}
