internal/lsp: pass options by reference instead of by value

We were previously behaving as though the slice/map values in the
options struct could be modified directly. The options should be cloned
before modification. Also, convert any usage of source.Options to
*source.Options.

Fixes golang/go#39592

Change-Id: Ib39f668bca0fa1038162206bd7793fd2049af576
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254558
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This commit is contained in:
Rebecca Stambler 2020-09-13 00:26:21 -04:00
parent c9a70fc28c
commit c537a342dd
18 changed files with 183 additions and 121 deletions

View File

@ -34,10 +34,6 @@ func updateAnalyzers(options *source.Options) {
// Always add hooks for all available analyzers, but disable them if the
// user does not have staticcheck enabled (they may enable it later on).
for _, a := range analyzers {
addStaticcheckAnalyzer(options, a)
options.AddStaticcheckAnalyzer(a)
}
}
func addStaticcheckAnalyzer(options *source.Options, a *analysis.Analyzer) {
options.StaticcheckAnalyzers[a.Name] = source.Analyzer{Analyzer: a, Enabled: true}
}

View File

@ -115,10 +115,14 @@ func readFile(ctx context.Context, uri span.URI, modTime time.Time) *fileHandle
func (c *Cache) NewSession(ctx context.Context) *Session {
index := atomic.AddInt64(&sessionIndex, 1)
options := source.DefaultOptions().Clone()
if c.options != nil {
c.options(options)
}
s := &Session{
cache: c,
id: strconv.FormatInt(index, 10),
options: source.DefaultOptions(),
options: options,
overlays: make(map[span.URI]*overlay),
gocmdRunner: &gocommand.Runner{},
}

View File

@ -27,7 +27,7 @@ type Session struct {
cache *Cache
id string
options source.Options
options *source.Options
viewMu sync.Mutex
views []*View
@ -117,11 +117,11 @@ func (c *closedFile) Version() float64 {
func (s *Session) ID() string { return s.id }
func (s *Session) String() string { return s.id }
func (s *Session) Options() source.Options {
func (s *Session) Options() *source.Options {
return s.options
}
func (s *Session) SetOptions(options source.Options) {
func (s *Session) SetOptions(options *source.Options) {
s.options = options
}
@ -140,7 +140,7 @@ func (s *Session) Cache() interface{} {
return s.cache
}
func (s *Session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, source.Snapshot, func(), error) {
func (s *Session) NewView(ctx context.Context, name string, folder span.URI, options *source.Options) (source.View, source.Snapshot, func(), error) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
view, snapshot, release, err := s.createView(ctx, name, folder, options, 0)
@ -153,7 +153,7 @@ func (s *Session) NewView(ctx context.Context, name string, folder span.URI, opt
return view, snapshot, release, nil
}
func (s *Session) createView(ctx context.Context, name string, folder span.URI, options source.Options, snapshotID uint64) (*View, *snapshot, func(), error) {
func (s *Session) createView(ctx context.Context, name string, folder span.URI, options *source.Options, snapshotID uint64) (*View, *snapshot, func(), error) {
index := atomic.AddInt64(&viewIndex, 1)
// We want a true background context and not a detached context here
// the spans need to be unrelated and no tag values should pollute it.
@ -197,7 +197,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
}
if v.session.cache.options != nil {
v.session.cache.options(&v.options)
v.session.cache.options(v.options)
}
// Set the module-specific information.
@ -253,7 +253,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
//
// It assumes that the caller has not yet created the view, and therefore does
// not lock any of the internal data structures before accessing them.
func (v *View) findWorkspaceModules(ctx context.Context, options source.Options) error {
func (v *View) findWorkspaceModules(ctx context.Context, options *source.Options) error {
// If the user is intentionally limiting their workspace scope, add their
// folder to the roots and return early.
if !options.ExpandWorkspaceToModule {
@ -431,7 +431,7 @@ func (s *Session) removeView(ctx context.Context, view *View) error {
return nil
}
func (s *Session) updateView(ctx context.Context, view *View, options source.Options) (*View, error) {
func (s *Session) updateView(ctx context.Context, view *View, options *source.Options) (*View, error) {
s.viewMu.Lock()
defer s.viewMu.Unlock()
i, err := s.dropView(ctx, view)

View File

@ -40,7 +40,7 @@ type View struct {
id string
optionsMu sync.Mutex
options source.Options
options *source.Options
// mu protects most mutable state of the view.
mu sync.Mutex
@ -278,13 +278,13 @@ func (v *View) Folder() span.URI {
return v.folder
}
func (v *View) Options() source.Options {
func (v *View) Options() *source.Options {
v.optionsMu.Lock()
defer v.optionsMu.Unlock()
return v.options
}
func minorOptionsChange(a, b source.Options) bool {
func minorOptionsChange(a, b *source.Options) bool {
// Check if any of the settings that modify our understanding of files have been changed
mapEnv := func(env []string) map[string]string {
m := make(map[string]string, len(env))
@ -315,7 +315,7 @@ func minorOptionsChange(a, b source.Options) bool {
return true
}
func (v *View) SetOptions(ctx context.Context, options source.Options) (source.View, error) {
func (v *View) SetOptions(ctx context.Context, options *source.Options) (source.View, error) {
// no need to rebuild the view if the options were not materially changed
v.optionsMu.Lock()
if minorOptionsChange(v.options, options) {
@ -804,7 +804,7 @@ func (v *View) maybeReinitialize() {
v.initializeOnce = &once
}
func (v *View) setBuildInformation(ctx context.Context, options source.Options) error {
func (v *View) setBuildInformation(ctx context.Context, options *source.Options) error {
if err := checkPathCase(v.Folder().Filename()); err != nil {
return errors.Errorf("invalid workspace configuration: %w", err)
}

View File

@ -211,9 +211,9 @@ func (app *Application) connect(ctx context.Context) (*connection, error) {
case strings.HasPrefix(app.Remote, "internal@"):
internalMu.Lock()
defer internalMu.Unlock()
opts := source.DefaultOptions()
opts := source.DefaultOptions().Clone()
if app.options != nil {
app.options(&opts)
app.options(opts)
}
key := fmt.Sprintf("%s %v", app.wd, opts)
if c := internalConnections[key]; c != nil {
@ -271,9 +271,9 @@ func (c *connection) initialize(ctx context.Context, options func(*source.Option
params.Capabilities.Workspace.Configuration = true
// Make sure to respect configured options when sending initialize request.
opts := source.DefaultOptions()
opts := source.DefaultOptions().Clone()
if options != nil {
options(&opts)
options(opts)
}
params.Capabilities.TextDocument.Hover = protocol.HoverClientCapabilities{
ContentFormat: []protocol.MarkupKind{opts.PreferredContentFormat},

View File

@ -104,7 +104,7 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara
}, nil
}
func toProtocolCompletionItems(candidates []completion.CompletionItem, rng protocol.Range, options source.Options) []protocol.CompletionItem {
func toProtocolCompletionItems(candidates []completion.CompletionItem, rng protocol.Range, options *source.Options) []protocol.CompletionItem {
var (
items = make([]protocol.CompletionItem, 0, len(candidates))
numDeepCompletionsSeen int

View File

@ -123,8 +123,8 @@ func (r *runner) callCompletion(t *testing.T, src span.Span, options func(*sourc
t.Fatal(err)
}
original := view.Options()
modified := original
options(&modified)
modified := view.Options().Clone()
options(modified)
view, err = view.SetOptions(r.ctx, modified)
if err != nil {
t.Error(err)

View File

@ -39,7 +39,7 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitializ
options := s.session.Options()
defer func() { s.session.SetOptions(options) }()
if err := s.handleOptionResults(ctx, source.SetOptions(&options, params.InitializationOptions)); err != nil {
if err := s.handleOptionResults(ctx, source.SetOptions(options, params.InitializationOptions)); err != nil {
return nil, err
}
options.ForClientCapabilities(params.Capabilities)

View File

@ -50,8 +50,8 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
cache := cache.New(ctx, nil)
session := cache.NewSession(ctx)
options := source.DefaultOptions()
tests.DefaultOptions(&options)
options := source.DefaultOptions().Clone()
tests.DefaultOptions(options)
session.SetOptions(options)
options.Env = datum.Config.Env
view, _, release, err := session.NewView(ctx, datum.Config.Dir, span.URIFromPath(datum.Config.Dir), options)
@ -64,7 +64,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
tests.EnableAllAnalyzers(view, &options)
tests.EnableAllAnalyzers(view, options)
view.SetOptions(ctx, options)
// Only run the -modfile specific tests in module mode with Go 1.14 or above.

View File

@ -104,7 +104,7 @@ func Hover(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle,
}, nil
}
func formatExplanation(text string, req *modfile.Require, options source.Options, isPrivate bool) string {
func formatExplanation(text string, req *modfile.Require, options *source.Options, isPrivate bool) string {
text = strings.TrimSuffix(text, "\n")
splt := strings.Split(text, "\n")
length := len(splt)

View File

@ -28,8 +28,8 @@ func TestModfileRemainsUnchanged(t *testing.T) {
ctx := tests.Context(t)
cache := cache.New(ctx, nil)
session := cache.NewSession(ctx)
options := source.DefaultOptions()
tests.DefaultOptions(&options)
options := source.DefaultOptions().Clone()
tests.DefaultOptions(options)
options.TempModfile = true
options.Env = []string{"GOPACKAGESDRIVER=off", "GOROOT="}

View File

@ -221,7 +221,7 @@ func analyses(ctx context.Context, snapshot Snapshot, reports map[VersionedFileI
// meant to provide diagnostics, but rather only suggested fixes.
// Skip these types of errors in diagnostics; we will use their
// suggested fixes when providing code actions.
if isConvenienceAnalyzer(e.Category) {
if isConvenienceAnalyzer(snapshot.View().Options(), e.Category) {
continue
}
// This is a bit of a hack, but clients > 3.15 will be able to grey out unnecessary code.
@ -312,7 +312,7 @@ func hasUndeclaredErrors(pkg Package) bool {
return false
}
func isConvenienceAnalyzer(category string) bool {
func isConvenienceAnalyzer(o *Options, category string) bool {
for _, a := range DefaultOptions().ConvenienceAnalyzers {
if category == a.Analyzer.Name {
return true

View File

@ -57,7 +57,7 @@ func GCOptimizationDetails(ctx context.Context, snapshot Snapshot, pkgDir span.U
if x == nil {
continue
}
v = filterDiagnostics(v, &opts)
v = filterDiagnostics(v, opts)
reports[x.VersionedFileIdentity()] = v
}
return reports, parseError

View File

@ -373,7 +373,7 @@ func formatVar(node ast.Spec, obj types.Object, decl *ast.GenDecl) *HoverInforma
return &HoverInformation{source: obj, comment: decl.Doc}
}
func FormatHover(h *HoverInformation, options Options) (string, error) {
func FormatHover(h *HoverInformation, options *Options) (string, error) {
signature := h.Signature
if signature != "" && options.PreferredContentFormat == protocol.Markdown {
signature = fmt.Sprintf("```go\n%s\n```", signature)
@ -403,7 +403,7 @@ func FormatHover(h *HoverInformation, options Options) (string, error) {
return "", errors.Errorf("no hover for %v", h.source)
}
func formatLink(h *HoverInformation, options Options) string {
func formatLink(h *HoverInformation, options *Options) string {
if !options.LinksInHover || options.LinkTarget == "" || h.Link == "" {
return ""
}
@ -418,14 +418,14 @@ func formatLink(h *HoverInformation, options Options) string {
}
}
func formatDoc(doc string, options Options) string {
func formatDoc(doc string, options *Options) string {
if options.PreferredContentFormat == protocol.Markdown {
return CommentToMarkdown(doc)
}
return doc
}
func formatHover(options Options, x ...string) string {
func formatHover(options *Options, x ...string) string {
var b strings.Builder
for i, el := range x {
if el != "" {

View File

@ -9,8 +9,10 @@ import (
"fmt"
"regexp"
"strings"
"sync"
"time"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/asmdecl"
"golang.org/x/tools/go/analysis/passes/assign"
"golang.org/x/tools/go/analysis/passes/atomic"
@ -52,79 +54,87 @@ import (
errors "golang.org/x/xerrors"
)
var (
optionsOnce sync.Once
defaultOptions *Options
)
//go:generate go run golang.org/x/tools/cmd/stringer -output enums_string.go -type ImportShortcut,HoverKind,Matcher,SymbolMatcher,SymbolStyle
//go:generate go run golang.org/x/tools/internal/lsp/source/genopts -output options_json.go
// DefaultOptions is the options that are used for Gopls execution independent
// of any externally provided configuration (LSP initialization, command
// invokation, etc.).
func DefaultOptions() Options {
var commands []string
for _, c := range Commands {
commands = append(commands, c.Name)
}
return Options{
ClientOptions: ClientOptions{
InsertTextFormat: protocol.PlainTextTextFormat,
PreferredContentFormat: protocol.Markdown,
ConfigurationSupported: true,
DynamicConfigurationSupported: true,
DynamicWatchedFilesSupported: true,
LineFoldingOnly: false,
HierarchicalDocumentSymbolSupport: true,
},
ServerOptions: ServerOptions{
SupportedCodeActions: map[FileKind]map[protocol.CodeActionKind]bool{
Go: {
protocol.SourceFixAll: true,
protocol.SourceOrganizeImports: true,
protocol.QuickFix: true,
protocol.RefactorRewrite: true,
protocol.RefactorExtract: true,
},
Mod: {
protocol.SourceOrganizeImports: true,
},
Sum: {},
func DefaultOptions() *Options {
optionsOnce.Do(func() {
var commands []string
for _, c := range Commands {
commands = append(commands, c.Name)
}
defaultOptions = &Options{
ClientOptions: ClientOptions{
InsertTextFormat: protocol.PlainTextTextFormat,
PreferredContentFormat: protocol.Markdown,
ConfigurationSupported: true,
DynamicConfigurationSupported: true,
DynamicWatchedFilesSupported: true,
LineFoldingOnly: false,
HierarchicalDocumentSymbolSupport: true,
},
SupportedCommands: commands,
},
UserOptions: UserOptions{
HoverKind: FullDocumentation,
LinkTarget: "pkg.go.dev",
},
DebuggingOptions: DebuggingOptions{
CompletionBudget: 100 * time.Millisecond,
LiteralCompletions: true,
},
ExperimentalOptions: ExperimentalOptions{
TempModfile: true,
ExpandWorkspaceToModule: true,
Codelens: map[string]bool{
CommandGenerate.Name: true,
CommandRegenerateCgo.Name: true,
CommandTidy.Name: true,
CommandToggleDetails.Name: false,
CommandUpgradeDependency.Name: true,
CommandVendor.Name: true,
ServerOptions: ServerOptions{
SupportedCodeActions: map[FileKind]map[protocol.CodeActionKind]bool{
Go: {
protocol.SourceFixAll: true,
protocol.SourceOrganizeImports: true,
protocol.QuickFix: true,
protocol.RefactorRewrite: true,
protocol.RefactorExtract: true,
},
Mod: {
protocol.SourceOrganizeImports: true,
},
Sum: {},
},
SupportedCommands: commands,
},
LinksInHover: true,
CompleteUnimported: true,
CompletionDocumentation: true,
DeepCompletion: true,
Matcher: Fuzzy,
SymbolMatcher: SymbolFuzzy,
},
Hooks: Hooks{
ComputeEdits: myers.ComputeEdits,
URLRegexp: urlRegexp(),
DefaultAnalyzers: defaultAnalyzers(),
TypeErrorAnalyzers: typeErrorAnalyzers(),
ConvenienceAnalyzers: convenienceAnalyzers(),
StaticcheckAnalyzers: map[string]Analyzer{},
GoDiff: true,
},
}
UserOptions: UserOptions{
HoverKind: FullDocumentation,
LinkTarget: "pkg.go.dev",
},
DebuggingOptions: DebuggingOptions{
CompletionBudget: 100 * time.Millisecond,
LiteralCompletions: true,
},
ExperimentalOptions: ExperimentalOptions{
TempModfile: true,
ExpandWorkspaceToModule: true,
Codelens: map[string]bool{
CommandGenerate.Name: true,
CommandRegenerateCgo.Name: true,
CommandTidy.Name: true,
CommandToggleDetails.Name: false,
CommandUpgradeDependency.Name: true,
CommandVendor.Name: true,
},
LinksInHover: true,
CompleteUnimported: true,
CompletionDocumentation: true,
DeepCompletion: true,
Matcher: Fuzzy,
SymbolMatcher: SymbolFuzzy,
},
Hooks: Hooks{
ComputeEdits: myers.ComputeEdits,
URLRegexp: urlRegexp(),
DefaultAnalyzers: defaultAnalyzers(),
TypeErrorAnalyzers: typeErrorAnalyzers(),
ConvenienceAnalyzers: convenienceAnalyzers(),
StaticcheckAnalyzers: map[string]Analyzer{},
GoDiff: true,
},
}
})
return defaultOptions
}
// Options holds various configuration that affects Gopls execution, organized
@ -206,11 +216,11 @@ type Hooks struct {
GoDiff bool
ComputeEdits diff.ComputeEdits
URLRegexp *regexp.Regexp
GofumptFormat func(ctx context.Context, src []byte) ([]byte, error)
DefaultAnalyzers map[string]Analyzer
TypeErrorAnalyzers map[string]Analyzer
ConvenienceAnalyzers map[string]Analyzer
StaticcheckAnalyzers map[string]Analyzer
GofumptFormat func(ctx context.Context, src []byte) ([]byte, error)
}
// ExperimentalOptions defines configuration for features under active
@ -475,6 +485,59 @@ func (o *Options) ForClientCapabilities(caps protocol.ClientCapabilities) {
o.HierarchicalDocumentSymbolSupport = caps.TextDocument.DocumentSymbol.HierarchicalDocumentSymbolSupport
}
func (o *Options) Clone() *Options {
result := &Options{
ClientOptions: o.ClientOptions,
DebuggingOptions: o.DebuggingOptions,
ExperimentalOptions: o.ExperimentalOptions,
Hooks: Hooks{
GoDiff: o.Hooks.GoDiff,
ComputeEdits: o.Hooks.ComputeEdits,
GofumptFormat: o.GofumptFormat,
URLRegexp: o.URLRegexp,
},
ServerOptions: o.ServerOptions,
UserOptions: o.UserOptions,
}
// Fully clone any slice or map fields. Only Hooks, ExperimentalOptions,
// and UserOptions can be modified.
copyStringMap := func(src map[string]bool) map[string]bool {
dst := make(map[string]bool)
for k, v := range src {
dst[k] = v
}
return dst
}
result.Analyses = copyStringMap(o.Analyses)
result.Annotations = copyStringMap(o.Annotations)
result.Codelens = copyStringMap(o.Codelens)
copySlice := func(src []string) []string {
dst := make([]string, len(src))
copy(dst, src)
return dst
}
result.Env = copySlice(o.Env)
result.BuildFlags = copySlice(o.BuildFlags)
copyAnalyzerMap := func(src map[string]Analyzer) map[string]Analyzer {
dst := make(map[string]Analyzer)
for k, v := range src {
dst[k] = v
}
return dst
}
result.DefaultAnalyzers = copyAnalyzerMap(o.DefaultAnalyzers)
result.TypeErrorAnalyzers = copyAnalyzerMap(o.TypeErrorAnalyzers)
result.ConvenienceAnalyzers = copyAnalyzerMap(o.ConvenienceAnalyzers)
result.StaticcheckAnalyzers = copyAnalyzerMap(o.StaticcheckAnalyzers)
return result
}
func (options *Options) AddStaticcheckAnalyzer(a *analysis.Analyzer) {
options.StaticcheckAnalyzers[a.Name] = Analyzer{Analyzer: a, Enabled: true}
}
func (o *Options) set(name string, value interface{}) OptionResult {
result := OptionResult{Name: name, Value: value}
switch name {

View File

@ -52,8 +52,8 @@ func testSource(t *testing.T, exporter packagestest.Exporter) {
cache := cache.New(ctx, nil)
session := cache.NewSession(ctx)
options := source.DefaultOptions()
tests.DefaultOptions(&options)
options := source.DefaultOptions().Clone()
tests.DefaultOptions(options)
options.Env = datum.Config.Env
view, _, release, err := session.NewView(ctx, "source_test", span.URIFromPath(datum.Config.Dir), options)
release()
@ -64,7 +64,7 @@ func testSource(t *testing.T, exporter packagestest.Exporter) {
// Enable type error analyses for tests.
// TODO(golang/go#38212): Delete this once they are enabled by default.
tests.EnableAllAnalyzers(view, &options)
tests.EnableAllAnalyzers(view, options)
view.SetOptions(ctx, options)
var modifications []source.FileModification
for filename, content := range datum.Config.Overlay {
@ -295,8 +295,8 @@ func (r *runner) callCompletion(t *testing.T, src span.Span, options func(*sourc
t.Fatal(err)
}
original := r.view.Options()
modified := original
options(&modified)
modified := original.Clone()
options(modified)
newView, err := r.view.SetOptions(r.ctx, modified)
if newView != r.view {
t.Fatalf("options change unexpectedly created new view")

View File

@ -153,13 +153,13 @@ type View interface {
RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) error) error
// Options returns a copy of the Options for this view.
Options() Options
Options() *Options
// SetOptions sets the options of this view to new values.
// Calling this may cause the view to be invalidated and a replacement view
// added to the session. If so the new view will be returned, otherwise the
// original one will be.
SetOptions(context.Context, Options) (View, error)
SetOptions(context.Context, *Options) (View, error)
// Snapshot returns the current snapshot for the view.
Snapshot(ctx context.Context) (Snapshot, func())
@ -222,7 +222,7 @@ type TidiedModule struct {
// A session may have many active views at any given time.
type Session interface {
// NewView creates a new View, returning it and its first snapshot.
NewView(ctx context.Context, name string, folder span.URI, options Options) (View, Snapshot, func(), error)
NewView(ctx context.Context, name string, folder span.URI, options *Options) (View, Snapshot, func(), error)
// Cache returns the cache that created this session, for debugging only.
Cache() interface{}
@ -250,10 +250,10 @@ type Session interface {
Overlays() []Overlay
// Options returns a copy of the SessionOptions for this session.
Options() Options
Options() *Options
// SetOptions sets the options of this session to new values.
SetOptions(Options)
SetOptions(*Options)
}
// Overlay is the type for a file held in memory on a session.

View File

@ -33,9 +33,8 @@ func (s *Server) addView(ctx context.Context, name string, uri span.URI) (source
if state < serverInitialized {
return nil, nil, func() {}, errors.Errorf("addView called before server initialized")
}
options := s.session.Options()
if err := s.fetchConfig(ctx, name, uri, &options); err != nil {
options := s.session.Options().Clone()
if err := s.fetchConfig(ctx, name, uri, options); err != nil {
return nil, nil, func() {}, err
}
return s.session.NewView(ctx, name, uri, options)
@ -44,8 +43,8 @@ func (s *Server) addView(ctx context.Context, name string, uri span.URI) (source
func (s *Server) didChangeConfiguration(ctx context.Context, changed interface{}) error {
// go through all the views getting the config
for _, view := range s.session.Views() {
options := view.Options()
if err := s.fetchConfig(ctx, view.Name(), view.Folder(), &options); err != nil {
options := s.session.Options().Clone()
if err := s.fetchConfig(ctx, view.Name(), view.Folder(), options); err != nil {
return err
}
view, err := view.SetOptions(ctx, options)