Skip to content

Commit

Permalink
gopls/internal/cache: remove overzealous bug.Report
Browse files Browse the repository at this point in the history
defineView's doc comment confidently states that the only
possible failure is a canceled context...  yet the very
first thing it does is return an error if the folder path
is invalid---and it was ever thus since the comment was
added in CL 552315. D'oh!

Downgrading to a non-bug error.

Fixes golang/go#70909

Change-Id: Ic58199786816ad7de9f77d18a7d46795440bdf01
Reviewed-on: https://go-review.googlesource.com/c/tools/+/639435
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
  • Loading branch information
adonovan committed Jan 2, 2025
1 parent 960f0f4 commit b93274b
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 10 deletions.
11 changes: 5 additions & 6 deletions gopls/internal/cache/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ func (s *Session) View(id string) (*View, error) {
// SnapshotOf returns a Snapshot corresponding to the given URI.
//
// In the case where the file can be can be associated with a View by
// bestViewForURI (based on directory information alone, without package
// [RelevantViews] (based on directory information alone, without package
// metadata), SnapshotOf returns the current Snapshot for that View. Otherwise,
// it awaits loading package metadata and returns a Snapshot for the first View
// containing a real (=not command-line-arguments) package for the file.
Expand Down Expand Up @@ -551,13 +551,12 @@ checkFiles:
}
def, err = defineView(ctx, fs, folder, fh)
if err != nil {
// We should never call selectViewDefs with a cancellable context, so
// this should never fail.
return nil, bug.Errorf("failed to define view for open file: %v", err)
// e.g. folder path is invalid?
return nil, fmt.Errorf("failed to define view for open file: %v", err)
}
// It need not strictly be the case that the best view for a file is
// distinct from other views, as the logic of getViewDefinition and
// bestViewForURI does not align perfectly. This is not necessarily a bug:
// [RelevantViews] does not align perfectly. This is not necessarily a bug:
// there may be files for which we can't construct a valid view.
//
// Nevertheless, we should not create redundant views.
Expand All @@ -572,7 +571,7 @@ checkFiles:
return defs, nil
}

// The viewDefiner interface allows the bestView algorithm to operate on both
// The viewDefiner interface allows the [RelevantViews] algorithm to operate on both
// Views and viewDefinitions.
type viewDefiner interface{ definition() *viewDefinition }

Expand Down
9 changes: 5 additions & 4 deletions gopls/internal/cache/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -809,9 +809,10 @@ func (s *Session) invalidateViewLocked(ctx context.Context, v *View, changed Sta
// If forURI is non-empty, this view should be the best view including forURI.
// Otherwise, it is the default view for the folder.
//
// defineView only returns an error in the event of context cancellation.
// defineView may return an error if the context is cancelled, or the
// workspace folder path is invalid.
//
// Note: keep this function in sync with bestView.
// Note: keep this function in sync with [RelevantViews].
//
// TODO(rfindley): we should be able to remove the error return, as
// findModules is going away, and all other I/O is memoized.
Expand All @@ -838,11 +839,11 @@ func defineView(ctx context.Context, fs file.Source, folder *Folder, forFile fil
// add those constraints to the viewDefinition's environment.

// Content trimming is nontrivial, so do this outside of the loop below.
// Keep this in sync with bestView.
// Keep this in sync with [RelevantViews].
path := forFile.URI().Path()
if content, err := forFile.Content(); err == nil {
// Note the err == nil condition above: by convention a non-existent file
// does not have any constraints. See the related note in bestView: this
// does not have any constraints. See the related note in [RelevantViews]: this
// choice of behavior shouldn't actually matter. In this case, we should
// only call defineView with Overlays, which always have content.
content = trimContentForPortMatch(content)
Expand Down

0 comments on commit b93274b

Please sign in to comment.