Skip to content

Commit

Permalink
gopls/internal/cache: refine bug reports
Browse files Browse the repository at this point in the history
Refine a few bug reports related to incorrectly positioned type checker
errors, by differentiating the 'fixed file' case from the case with no
fixed files.

Since we haven't made progress on these bugs, we need more information
to guide our debugging.

If the bugs only occur in the presence of AST fixes, we can probably
downgrade them to not be a bug: if we're fixing the AST, we won't
display type checker errors anyway. Nevertheless, leave the bug reports
for now, so that we can collect data.

For golang/go#65960
For golang/go#66765
For golang/go#66766

Change-Id: I2060c897d249cdd5cc3e7bb183d3f563987bec57
Reviewed-on: https://go-review.googlesource.com/c/tools/+/621876
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
  • Loading branch information
findleyr authored and gopherbot committed Oct 22, 2024
1 parent 63e4449 commit 6381f0b
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 8 deletions.
36 changes: 28 additions & 8 deletions gopls/internal/cache/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -1877,7 +1877,11 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, inputs *typeCheckInputs, errs [
// over fixed syntax, which overflowed its file. So it's definitely
// possible that we get here (it's hard to reason about fixing up the
// AST). Nevertheless, it's a bug.
bug.Reportf("internal error: type checker error %q outside its Fset", e)
if pkg.hasFixedFiles() {
bug.Reportf("internal error: type checker error %q outside its Fset (fixed files)", e)
} else {
bug.Reportf("internal error: type checker error %q outside its Fset", e)
}
continue
}
pgf, err := pkg.File(protocol.URIFromPath(posn.Filename))
Expand All @@ -1889,12 +1893,16 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, inputs *typeCheckInputs, errs [
// package (the message would be rather confusing), but we do want to
// report an error in the current package (golang/go#59005).
if i == 0 {
bug.Reportf("internal error: could not locate file for primary type checker error %v: %v", e, err)
if pkg.hasFixedFiles() {
bug.Reportf("internal error: could not locate file for primary type checker error %v: %v (fixed files)", e, err)
} else {
bug.Reportf("internal error: could not locate file for primary type checker error %v: %v", e, err)
}
}
continue
}

// debugging #65960
// debugging golang/go#65960
//
// At this point, we know 'start' IsValid, and
// StartPosition(start) worked (with e.Fset).
Expand All @@ -1903,21 +1911,33 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, inputs *typeCheckInputs, errs [
// is also in range for pgf.Tok, which means
// the PosRange failure must be caused by 'end'.
if pgf.Tok != e.Fset.File(start) {
bug.Reportf("internal error: inconsistent token.Files for pos")
if pkg.hasFixedFiles() {
bug.Reportf("internal error: inconsistent token.Files for pos (fixed files)")
} else {
bug.Reportf("internal error: inconsistent token.Files for pos")
}
}

if end == start {
// Expand the end position to a more meaningful span.
end = analysisinternal.TypeErrorEndPos(e.Fset, pgf.Src, start)

// debugging #65960
// debugging golang/go#65960
if _, err := safetoken.Offset(pgf.Tok, end); err != nil {
bug.Reportf("TypeErrorEndPos returned invalid end: %v", err)
if pkg.hasFixedFiles() {
bug.Reportf("TypeErrorEndPos returned invalid end: %v (fixed files)", err)
} else {
bug.Reportf("TypeErrorEndPos returned invalid end: %v", err)
}
}
} else {
// debugging #65960
// debugging golang/go#65960
if _, err := safetoken.Offset(pgf.Tok, end); err != nil {
bug.Reportf("ReadGo116ErrorData returned invalid end: %v", err)
if pkg.hasFixedFiles() {
bug.Reportf("ReadGo116ErrorData returned invalid end: %v (fixed files)", err)
} else {
bug.Reportf("ReadGo116ErrorData returned invalid end: %v", err)
}
}
}

Expand Down
9 changes: 9 additions & 0 deletions gopls/internal/cache/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"go/scanner"
"go/token"
"go/types"
"slices"
"sync"

"golang.org/x/tools/gopls/internal/cache/metadata"
Expand Down Expand Up @@ -87,6 +88,14 @@ func (p *syntaxPackage) tests() *testfuncs.Index {
return p._tests
}

// hasFixedFiles reports whether there are any 'fixed' compiled go files in the
// package.
//
// Intended to be used to refine bug reports.
func (p *syntaxPackage) hasFixedFiles() bool {
return slices.ContainsFunc(p.compiledGoFiles, (*parsego.File).Fixed)
}

func (p *Package) String() string { return string(p.metadata.ID) }

func (p *Package) Metadata() *metadata.Package { return p.metadata }
Expand Down

0 comments on commit 6381f0b

Please sign in to comment.