Skip to content

Commit

Permalink
fix: improve error position information (#1181)
Browse files Browse the repository at this point in the history
  • Loading branch information
wesbillman authored Apr 5, 2024
1 parent f0006fa commit 01091ce
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 31 deletions.
12 changes: 6 additions & 6 deletions backend/schema/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,12 @@ func TestParsing(t *testing.T) {
{name: "InvalidRequestRef",
input: `module test { verb test(InvalidRequest) InvalidResponse}`,
errors: []string{
"1:25: unknown reference \"InvalidRequest\"",
"1:41: unknown reference \"InvalidResponse\""}},
"1:25-25: unknown reference \"InvalidRequest\"",
"1:41-41: unknown reference \"InvalidResponse\""}},
{name: "InvalidRef",
input: `module test { data Data { user user.User }}`,
errors: []string{
"1:32: unknown reference \"user.User\""}},
"1:32-32: unknown reference \"user.User\""}},
{name: "InvalidMetadataSyntax",
input: `module test { data Data {} calls }`,
errors: []string{
Expand All @@ -214,12 +214,12 @@ func TestParsing(t *testing.T) {
{name: "InvalidDataMetadata",
input: `module test { data Data {} +calls verb }`,
errors: []string{
"1:28: metadata \"+calls verb\" is not valid on data structures",
"1:35: unknown reference \"verb\"",
"1:28-28: metadata \"+calls verb\" is not valid on data structures",
"1:35-35: unknown reference \"verb\"",
}},
{name: "KeywordAsName",
input: `module int { data String { name String } verb verb(String) String }`,
errors: []string{"1:14: data structure name \"String\" is a reserved word"}},
errors: []string{"1:14-14: data structure name \"String\" is a reserved word"}},
{name: "BuiltinRef",
input: `module test { verb myIngress(HttpRequest<String>) HttpResponse<String, String> }`,
expected: &Schema{
Expand Down
3 changes: 1 addition & 2 deletions backend/schema/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"strings"

"github.com/alecthomas/participle/v2"
"github.com/alecthomas/participle/v2/lexer"
"github.com/alecthomas/types/optional"
xreflect "golang.design/x/reflect"
"golang.org/x/exp/maps"
Expand Down Expand Up @@ -438,7 +437,7 @@ func dfsForDependencyCycle(imports map[string][]string, vertexStates map[depende
}

func errorf(pos interface{ Position() Position }, format string, args ...interface{}) error {
return participle.Errorf(lexer.Position(pos.Position()), format, args...)
return Errorf(pos.Position(), pos.Position().Column, format, args...)
}

func validateVerbMetadata(scopes Scopes, n *Verb) (merr []error) {
Expand Down
16 changes: 8 additions & 8 deletions backend/schema/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ func TestValidate(t *testing.T) {
}
`,
errs: []string{
"3:13: ingress verb a: request type Empty must be builtin.HttpRequest",
"3:20: ingress verb a: response type Empty must be builtin.HttpRequest",
"3:13-13: ingress verb a: request type Empty must be builtin.HttpRequest",
"3:20-20: ingress verb a: response type Empty must be builtin.HttpRequest",
}},
{name: "IngressBodyTypes",
schema: `
Expand Down Expand Up @@ -130,12 +130,12 @@ func TestValidate(t *testing.T) {
}
`,
errs: []string{
"11:15: ingress verb any: request type HttpRequest<Any> must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any",
"11:33: ingress verb any: response type HttpResponse<Any, Any> must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any",
"14:31: ingress verb path: cannot use path parameter \"invalid\" with request type String, expected Data type",
"16:7: duplicate http ingress GET /path/{} for 17:6:\"pathFound\" and 15:6:\"pathMissing\"",
"16:31: ingress verb pathMissing: request type one.Path does not contain a field corresponding to the parameter \"missing\"",
"18:7: duplicate http ingress GET /path/{} for 13:6:\"path\" and 17:6:\"pathFound\"",
"11:15-15: ingress verb any: request type HttpRequest<Any> must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any",
"11:33-33: ingress verb any: response type HttpResponse<Any, Any> must have a body of bytes, string, data structure, unit, float, int, bool, map, or array not Any",
"14:31-31: ingress verb path: cannot use path parameter \"invalid\" with request type String, expected Data type",
"16:31-31: ingress verb pathMissing: request type one.Path does not contain a field corresponding to the parameter \"missing\"",
"16:7-7: duplicate http ingress GET /path/{} for 17:6:\"pathFound\" and 15:6:\"pathMissing\"",
"18:7-7: duplicate http ingress GET /path/{} for 13:6:\"path\" and 17:6:\"pathFound\"",
}},
{name: "Array",
schema: `
Expand Down
4 changes: 2 additions & 2 deletions go-runtime/compile/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ func parseDirectives(fset *token.FileSet, docs *ast.CommentGroup) ([]directive,
// Adjust the Participle-reported position relative to the AST node.
var perr participle.Error
if errors.As(err, &perr) {
ppos := perr.Position()
ppos := schema.Position{}
ppos.Filename = pos.Filename
ppos.Column += pos.Column + 2
ppos.Line = pos.Line
err = participle.Errorf(ppos, "%s", perr.Message())
err = schema.Errorf(ppos, ppos.Column, "%s", perr.Message())
} else {
err = fmt.Errorf("%s: %w", pos, err)
}
Expand Down
19 changes: 14 additions & 5 deletions go-runtime/compile/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ func errorf(node ast.Node, format string, args ...interface{}) schema.Error {
return schema.Errorf(pos, endCol, format, args...)
}

func tokenWrapf(pos token.Pos, tokenText string, err error, format string, args ...interface{}) schema.Error {
goPos := goPosToSchemaPos(pos)
endColumn := goPos.Column
if len(tokenText) > 0 {
endColumn += utf8.RuneCountInString(tokenText)
}
return schema.Wrapf(goPos, endColumn, err, format, args...)
}

func wrapf(node ast.Node, err error, format string, args ...interface{}) schema.Error {
pos, endCol := goNodePosToSchemaPos(node)
return schema.Wrapf(pos, endCol, err, format, args...)
Expand Down Expand Up @@ -554,7 +563,7 @@ func visitStruct(pctx *parseContext, pos token.Pos, tnode types.Type) (*schema.R
arg := named.TypeArgs().At(i)
typeArg, err := visitType(pctx, pos, arg)
if err != nil {
return nil, tokenErrorf(pos, arg.String(), "type parameter %s: %v", arg.String(), err)
return nil, tokenWrapf(pos, arg.String(), err, "type parameter %s", arg.String())
}

// Fully qualify the Ref if needed
Expand Down Expand Up @@ -587,7 +596,7 @@ func visitStruct(pctx *parseContext, pos token.Pos, tnode types.Type) (*schema.R
})
typeArg, err := visitType(pctx, pos, named.TypeArgs().At(i))
if err != nil {
return nil, tokenErrorf(pos, param.Obj().Name(), "type parameter %s: %v", param.Obj().Name(), err)
return nil, tokenWrapf(pos, param.Obj().Name(), err, "type parameter %s", param.Obj().Name())
}
dataRef.TypeParameters = append(dataRef.TypeParameters, typeArg)
}
Expand Down Expand Up @@ -627,7 +636,7 @@ func visitStruct(pctx *parseContext, pos token.Pos, tnode types.Type) (*schema.R
f := s.Field(i)
ft, err := visitType(pctx, f.Pos(), f.Type())
if err != nil {
return nil, tokenErrorf(f.Pos(), f.Name(), "field %s: %v", f.Name(), err)
return nil, tokenWrapf(f.Pos(), f.Name(), err, "field %s", f.Name())
}

// Check if field is exported
Expand Down Expand Up @@ -771,10 +780,10 @@ func visitType(pctx *parseContext, pos token.Pos, tnode types.Type) (schema.Type
if underlying.String() == "any" {
return &schema.Any{Pos: goPosToSchemaPos(pos)}, nil
}
return nil, tokenErrorf(pos, tnode.String(), "unsupported type %q", tnode)
return nil, tokenErrorf(pos, "", "unsupported type %q", tnode)

default:
return nil, tokenErrorf(pos, tnode.String(), "unsupported type %T", tnode)
return nil, tokenErrorf(pos, "", "unsupported type %q", tnode)
}
}

Expand Down
19 changes: 11 additions & 8 deletions lsp/lsp.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,17 @@ func (s *Server) post(err error) {
errByFilename := make(map[string]errSet)

// Deduplicate and associate by filename.
for _, subErr := range ftlErrors.UnwrapAll(err) {
var ce schema.Error
if errors.As(subErr, &ce) {
filename := ce.Pos.Filename
if _, exists := errByFilename[filename]; !exists {
errByFilename[filename] = make(errSet)
errs := ftlErrors.UnwrapAll(err)
for _, err := range errs {
if ftlErrors.Innermost(err) {
var ce schema.Error
if errors.As(err, &ce) {
filename := ce.Pos.Filename
if _, exists := errByFilename[filename]; !exists {
errByFilename[filename] = make(errSet)
}
errByFilename[filename][strings.TrimSpace(ce.Error())] = ce
}
errByFilename[filename][strings.TrimSpace(ce.Error())] = ce
}
}

Expand All @@ -101,7 +104,7 @@ func publishErrors(errByFilename map[string]errSet, s *Server) {
severity := protocol.DiagnosticSeverityError

// If the end column is not set, set it to the length of the word.
if e.EndColumn-1 == pp.Column {
if e.EndColumn <= pp.Column {
length, err := getLineOrWordLength(filename, pp.Line, pp.Column, false)
if err != nil {
s.logger.Errorf(err, "Failed to get line or word length")
Expand Down

0 comments on commit 01091ce

Please sign in to comment.