Skip to content

Return errors instead of panics #638

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Return errors instead of panics #638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions internal/ls/definition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
package ls

import (
"fmt"
"github.com/microsoft/typescript-go/internal/ast"
"github.com/microsoft/typescript-go/internal/astnav"
"github.com/microsoft/typescript-go/internal/core"
"github.com/microsoft/typescript-go/internal/scanner"
)

func (l *LanguageService) ProvideDefinitions(fileName string, position int) []Location {
program, file := l.getProgramAndFile(fileName)
func (l *LanguageService) ProvideDefinitions(fileName string, position int) ([]Location, error) {
program, file, err := l.getProgramAndFile(fileName)
if err != nil {
return nil, fmt.Errorf("failed to get program and file: %w", err)
}

node := astnav.GetTouchingPropertyName(file, position)
if node.Kind == ast.KindSourceFile {
return nil
return nil, nil
}

checker := program.GetTypeChecker()
Expand All @@ -33,7 +38,7 @@ func (l *LanguageService) ProvideDefinitions(fileName string, position int) []Lo
Range: core.NewTextRange(pos, loc.End()),
})
}
return locations
return locations, nil
}
return nil
return nil, nil
}
10 changes: 7 additions & 3 deletions internal/ls/diagnostics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
package ls

import (
"fmt"
"slices"

"github.com/microsoft/typescript-go/internal/ast"
)

func (l *LanguageService) GetDocumentDiagnostics(fileName string) []*ast.Diagnostic {
program, file := l.getProgramAndFile(fileName)
func (l *LanguageService) GetDocumentDiagnostics(fileName string) ([]*ast.Diagnostic, error) {
program, file, err := l.getProgramAndFile(fileName)
if err != nil {
return nil, fmt.Errorf("failed to get program and file for diagnostics: %w", err)
}
syntaxDiagnostics := program.GetSyntacticDiagnostics(file)
semanticDiagnostics := program.GetSemanticDiagnostics(file)
return slices.Concat(syntaxDiagnostics, semanticDiagnostics)
return slices.Concat(syntaxDiagnostics, semanticDiagnostics), nil
}
15 changes: 10 additions & 5 deletions internal/ls/hover.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,28 @@
package ls

import (
"fmt"

"github.com/microsoft/typescript-go/internal/ast"
"github.com/microsoft/typescript-go/internal/astnav"
)

func (l *LanguageService) ProvideHover(fileName string, position int) string {
program, file := l.getProgramAndFile(fileName)
func (l *LanguageService) ProvideHover(fileName string, position int) (string, error) {
program, file, err := l.getProgramAndFile(fileName)
if err != nil {
return "", fmt.Errorf("failed to get program and file for hover: %w", err)
}
node := astnav.GetTouchingPropertyName(file, position)
if node.Kind == ast.KindSourceFile {
// Avoid giving quickInfo for the sourceFile as a whole.
return ""
return "", nil
}

checker := program.GetTypeChecker()
if symbol := checker.GetSymbolAtLocation(node); symbol != nil {
if t := checker.GetTypeOfSymbolAtLocation(symbol, node); t != nil {
return checker.TypeToString(t)
return checker.TypeToString(t), nil
}
}
return ""
return "", nil
}
8 changes: 5 additions & 3 deletions internal/ls/languageservice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package ls

import (
"fmt"

"github.com/microsoft/typescript-go/internal/ast"
"github.com/microsoft/typescript-go/internal/compiler"
"github.com/microsoft/typescript-go/internal/core"
Expand Down Expand Up @@ -55,11 +57,11 @@ func (l *LanguageService) GetProgram() *compiler.Program {
return l.host.GetProgram()
}

func (l *LanguageService) getProgramAndFile(fileName string) (*compiler.Program, *ast.SourceFile) {
func (l *LanguageService) getProgramAndFile(fileName string) (*compiler.Program, *ast.SourceFile, error) {
program := l.GetProgram()
file := program.GetSourceFile(fileName)
if file == nil {
panic("file not found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that these are mainly invariants that shouldn't be broken, such that we'd want to know about them. But, maybe we need to surface these differently (like gopls' bug.Report), or as a general panic handler (sort of like old TS with exceptions, which we also did not use except in tragic cases).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but it happened in the mentioned issue, and I'm not sure it should crash the LSP server.

I don't have any strong opinion, because it's always up to the team and the project. But in general, panics are for unexpected errors, this one (even if it should never happen in real life) is kinda expected.
And I'm not a big fan of global recovery exactly because if some panic happens, we'd want to know about it.

I think the same problem would be reported in issue even without panic anyway, since it's a bug (at least looks like the one).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, lsp servers shouldn't crash for file not found cases. At least the ts lsp , go lsp doesn't crash. I think we need an error channel to manage errors async. The Current version #645 seems to be halting at every error.

return nil, nil, fmt.Errorf("file not found")
}
return program, file
return program, file, nil
}
27 changes: 18 additions & 9 deletions internal/lsp/converters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,18 @@ func (c *converters) fromLspRange(textRange lsproto.Range, fileName string) (cor
if scriptInfo == nil {
return core.TextRange{}, fmt.Errorf("no script info found for %s", fileName)
}
return core.NewTextRange(
lineAndCharacterToPosition(textRange.Start, scriptInfo.LineMap()),
lineAndCharacterToPosition(textRange.End, scriptInfo.LineMap()),
), nil

startPos, err := lineAndCharacterToPosition(textRange.Start, scriptInfo.LineMap())
if err != nil {
return core.TextRange{}, fmt.Errorf("error converting start position: %w", err)
}

endPos, err := lineAndCharacterToPosition(textRange.End, scriptInfo.LineMap())
if err != nil {
return core.TextRange{}, fmt.Errorf("error converting end position: %w", err)
}

return core.NewTextRange(startPos, endPos), nil
}

func (c *converters) fromLspTextChange(change *lsproto.TextDocumentContentChangePartial, fileName string) (ls.TextChange, error) {
Expand Down Expand Up @@ -124,7 +132,7 @@ func (c *converters) lineAndCharacterToPosition(lineAndCharacter lsproto.Positio
if scriptInfo == nil {
return 0, fmt.Errorf("no script info found for %s", fileName)
}
return lineAndCharacterToPosition(lineAndCharacter, scriptInfo.LineMap()), nil
return lineAndCharacterToPosition(lineAndCharacter, scriptInfo.LineMap())
}

func languageKindToScriptKind(languageID lsproto.LanguageKind) core.ScriptKind {
Expand Down Expand Up @@ -188,19 +196,20 @@ func fileNameToDocumentUri(fileName string) lsproto.DocumentUri {
return lsproto.DocumentUri("file://" + fileName)
}

func lineAndCharacterToPosition(lineAndCharacter lsproto.Position, lineMap []core.TextPos) int {
func lineAndCharacterToPosition(lineAndCharacter lsproto.Position, lineMap []core.TextPos) (int, error) {
line := int(lineAndCharacter.Line)
offset := int(lineAndCharacter.Character)

if line < 0 || line >= len(lineMap) {
panic(fmt.Sprintf("bad line number. Line: %d, lineMap length: %d", line, len(lineMap)))
return 0, fmt.Errorf("bad line number. Line: %d, lineMap length: %d", line, len(lineMap))
}

res := int(lineMap[line]) + offset
if line < len(lineMap)-1 && res >= int(lineMap[line+1]) {
panic("resulting position is out of bounds")
return 0, fmt.Errorf("resulting position is out of bounds")
}
return res

return res, nil
}

func positionToLineAndCharacter(position int, lineMap []core.TextPos) lsproto.Position {
Expand Down
18 changes: 15 additions & 3 deletions internal/lsp/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,11 @@ func (s *Server) handleDidClose(req *lsproto.RequestMessage) error {
func (s *Server) handleDocumentDiagnostic(req *lsproto.RequestMessage) error {
params := req.Params.(*lsproto.DocumentDiagnosticParams)
file, project := s.getFileAndProject(params.TextDocument.Uri)
diagnostics := project.LanguageService().GetDocumentDiagnostics(file.FileName())
diagnostics, err := project.LanguageService().GetDocumentDiagnostics(file.FileName())
if err != nil {
return s.sendError(req.ID, err)
}

lspDiagnostics := make([]lsproto.Diagnostic, len(diagnostics))
for i, diag := range diagnostics {
if lspDiagnostic, err := s.converters.toLspDiagnostic(diag); err != nil {
Expand All @@ -330,7 +334,11 @@ func (s *Server) handleHover(req *lsproto.RequestMessage) error {
return s.sendError(req.ID, err)
}

hoverText := project.LanguageService().ProvideHover(file.FileName(), pos)
hoverText, err := project.LanguageService().ProvideHover(file.FileName(), pos)
if err != nil {
return s.sendError(req.ID, err)
}

return s.sendResult(req.ID, &lsproto.Hover{
Contents: lsproto.MarkupContentOrMarkedStringOrMarkedStrings{
MarkupContent: &lsproto.MarkupContent{
Expand All @@ -349,7 +357,11 @@ func (s *Server) handleDefinition(req *lsproto.RequestMessage) error {
return s.sendError(req.ID, err)
}

locations := project.LanguageService().ProvideDefinitions(file.FileName(), pos)
locations, err := project.LanguageService().ProvideDefinitions(file.FileName(), pos)
if err != nil {
return s.sendError(req.ID, err)
}

lspLocations := make([]lsproto.Location, len(locations))
for i, loc := range locations {
if lspLocation, err := s.converters.toLspLocation(loc); err != nil {
Expand Down