Skip to content

Commit e1c804e

Browse files
raphaelraphael
authored
eval: report validation error locations (#3890)
Add file:line prefixes to validation errors when the failing expression has a DSL function pointer. Refactor DSL error location resolution to skip internal Goa frames reliably and add tests that assert locations for both DSL and validation errors.
1 parent 567be03 commit e1c804e

12 files changed

Lines changed: 538 additions & 29 deletions

‎codegen/generator/generate_union_merge_integration_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ func TestGenerateUnionUserTypeSamePathMerged(t *testing.T) {
2828
})
2929

3030
var MyUnion = d.Type("MyUnion", func() {
31+
d.Meta("struct:pkg:path", "types")
3132
d.OneOf("MyUnion", func() {
3233
d.Attribute("sum", Summary)
3334
})

‎eval/error.go‎

Lines changed: 89 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"os"
66
"path/filepath"
7+
"reflect"
78
"runtime"
89
"strings"
910
)
@@ -66,16 +67,14 @@ func normalizeFileForPackageMatch(file string) string {
6667
// When successful it returns the file name and line number, empty string and
6768
// 0 otherwise.
6869
func computeErrorLocation() (file string, line int) {
69-
skipFunc := func(pc uintptr, file string) bool {
70+
shouldSkip := func(file, name string) bool {
7071
if strings.HasSuffix(file, "_test.go") { // Be nice with tests
7172
return false
7273
}
73-
file = filepath.ToSlash(file)
74-
fn := runtime.FuncForPC(pc)
75-
var name string
76-
if fn != nil {
77-
name = fn.Name()
74+
if isGoaSourceFile(file) {
75+
return true
7876
}
77+
file = filepath.ToSlash(file)
7978
normalized := normalizeFileForPackageMatch(file)
8079
for _, pkg := range Context.dslPackages {
8180
if strings.Contains(file, pkg) || strings.Contains(normalized, pkg) || strings.Contains(name, pkg) {
@@ -84,24 +83,97 @@ func computeErrorLocation() (file string, line int) {
8483
}
8584
return false
8685
}
87-
depth := 3
88-
pc, file, line, _ := runtime.Caller(depth)
89-
for skipFunc(pc, file) {
90-
depth++
91-
pc, file, line, _ = runtime.Caller(depth)
86+
87+
// Start scanning just above computeErrorLocation itself. This is robust to
88+
// inlining and avoids hardcoding assumptions about the exact call depth.
89+
const skip = 2
90+
pcs := make([]uintptr, 32)
91+
n := runtime.Callers(skip, pcs)
92+
frames := runtime.CallersFrames(pcs[:n])
93+
for {
94+
frame, more := frames.Next()
95+
if frame.File == "" || frame.Line == 0 {
96+
if !more {
97+
break
98+
}
99+
continue
100+
}
101+
if !shouldSkip(frame.File, frame.Function) {
102+
return relativeToWorkdir(frame.File), frame.Line
103+
}
104+
if !more {
105+
break
106+
}
107+
}
108+
return "", 0
109+
}
110+
111+
// isGoaSourceFile reports whether file points into the Goa module sources.
112+
//
113+
// This is used to robustly skip internal Goa frames even when the runtime
114+
// reports a call location (file:line) inside an inlined Goa function but the
115+
// corresponding frame Function name does not include an import path.
116+
func isGoaSourceFile(file string) bool {
117+
_, thisFile, _, ok := runtime.Caller(0)
118+
if !ok {
119+
return false
120+
}
121+
root := filepath.Dir(filepath.Dir(thisFile))
122+
rel, err := filepath.Rel(root, file)
123+
if err != nil {
124+
return false
125+
}
126+
return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)))
127+
}
128+
129+
// validationErrorLocation returns the location of the DSL that declared the
130+
// given expression when available.
131+
//
132+
// The location is derived from the expression DSL function pointer and is used
133+
// to annotate validation errors (i.e. errors returned by Validate()).
134+
func validationErrorLocation(expr Expression) (file string, line int, ok bool) {
135+
source, ok := expr.(Source)
136+
if !ok {
137+
return "", 0, false
138+
}
139+
fn := source.DSL()
140+
if fn == nil {
141+
return "", 0, false
92142
}
143+
return dslFuncLocation(fn)
144+
}
145+
146+
// dslFuncLocation returns the file and line where the given DSL function is
147+
// declared.
148+
//
149+
// The returned file is relative to the current working directory when possible.
150+
func dslFuncLocation(fn func()) (file string, line int, ok bool) {
151+
pc := reflect.ValueOf(fn).Pointer()
152+
f := runtime.FuncForPC(pc)
153+
if f == nil {
154+
return "", 0, false
155+
}
156+
file, line = f.FileLine(pc)
157+
if file == "" || line == 0 {
158+
return "", 0, false
159+
}
160+
return relativeToWorkdir(file), line, true
161+
}
162+
163+
// relativeToWorkdir returns file relative to the current working directory when
164+
// possible, otherwise it returns file unchanged.
165+
func relativeToWorkdir(file string) string {
93166
wd, err := os.Getwd()
94167
if err != nil {
95-
return file, line
168+
return file
96169
}
97170
wd, err = filepath.Abs(wd)
98171
if err != nil {
99-
return file, line
172+
return file
100173
}
101-
f, err := filepath.Rel(wd, file)
174+
rel, err := filepath.Rel(wd, file)
102175
if err != nil {
103-
return file, line
176+
return file
104177
}
105-
file = f
106-
return file, line
178+
return rel
107179
}

‎eval/error_location_test.go‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
package eval_test
2+
3+
import (
4+
"errors"
5+
"os"
6+
"path/filepath"
7+
"reflect"
8+
"runtime"
9+
"testing"
10+
11+
"goa.design/goa/v3/eval"
12+
)
13+
14+
func TestReportErrorRecordsLocation(t *testing.T) {
15+
t.Skip("computeErrorLocation intentionally skips eval package frames; this is tested from another package")
16+
}
17+
18+
func TestValidationErrorsIncludeLocation(t *testing.T) {
19+
var verr eval.ValidationErrors
20+
21+
_, file, _, ok := runtime.Caller(0)
22+
if !ok {
23+
t.Fatal("runtime.Caller failed")
24+
}
25+
dsl := func() {}
26+
27+
pc := reflect.ValueOf(dsl).Pointer()
28+
fn := runtime.FuncForPC(pc)
29+
if fn == nil {
30+
t.Fatal("runtime.FuncForPC returned nil")
31+
}
32+
_, expectedLine := fn.FileLine(pc)
33+
expectedFile := relativeToWorkdir(t, file)
34+
35+
expr := &testExpr{name: "expr", dsl: dsl}
36+
verr.AddError(expr, errors.New("bad"))
37+
38+
expected := "[" + expectedFile + ":" + itoa(expectedLine) + "] expr: bad"
39+
if got := verr.Error(); got != expected {
40+
t.Fatalf("unexpected error message:\n got: %q\nwant: %q", got, expected)
41+
}
42+
}
43+
44+
func TestValidationErrorsWithoutLocation(t *testing.T) {
45+
var verr eval.ValidationErrors
46+
verr.AddError(eval.Top, errors.New("bad"))
47+
if got, expected := verr.Error(), "top-level: bad"; got != expected {
48+
t.Fatalf("unexpected error message:\n got: %q\nwant: %q", got, expected)
49+
}
50+
}
51+
52+
func TestValidationErrorsNilDSL(t *testing.T) {
53+
var verr eval.ValidationErrors
54+
expr := &testExpr{name: "expr", dsl: nil}
55+
verr.AddError(expr, errors.New("bad"))
56+
if got, expected := verr.Error(), "expr: bad"; got != expected {
57+
t.Fatalf("unexpected error message:\n got: %q\nwant: %q", got, expected)
58+
}
59+
}
60+
61+
type testExpr struct {
62+
name string
63+
dsl func()
64+
}
65+
66+
func (e *testExpr) EvalName() string { return e.name }
67+
func (e *testExpr) DSL() func() { return e.dsl }
68+
69+
func relativeToWorkdir(t *testing.T, file string) string {
70+
t.Helper()
71+
72+
wd, err := os.Getwd()
73+
if err != nil {
74+
t.Fatalf("os.Getwd failed: %v", err)
75+
}
76+
wd, err = filepath.Abs(wd)
77+
if err != nil {
78+
t.Fatalf("filepath.Abs failed: %v", err)
79+
}
80+
rel, err := filepath.Rel(wd, file)
81+
if err != nil {
82+
t.Fatalf("filepath.Rel failed: %v", err)
83+
}
84+
return rel
85+
}
86+
87+
func itoa(v int) string {
88+
// Avoid pulling fmt/strconv into this focused test.
89+
if v == 0 {
90+
return "0"
91+
}
92+
var buf [32]byte
93+
i := len(buf)
94+
for v > 0 {
95+
i--
96+
buf[i] = byte('0' + v%10)
97+
v /= 10
98+
}
99+
return string(buf[i:])
100+
}

‎eval/eval.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,12 @@ type ValidationErrors struct {
148148
func (verr *ValidationErrors) Error() string {
149149
msg := make([]string, len(verr.Errors))
150150
for i, err := range verr.Errors {
151-
msg[i] = fmt.Sprintf("%s: %s", verr.Expressions[i].EvalName(), err)
151+
expr := verr.Expressions[i]
152+
if file, line, ok := validationErrorLocation(expr); ok {
153+
msg[i] = fmt.Sprintf("[%s:%d] %s: %s", file, line, expr.EvalName(), err)
154+
} else {
155+
msg[i] = fmt.Sprintf("%s: %s", expr.EvalName(), err)
156+
}
152157
}
153158
return strings.Join(msg, "\n")
154159
}

‎eval/run_dsl_error_location_test.go‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
package eval_test
2+
3+
import (
4+
"errors"
5+
"reflect"
6+
"runtime"
7+
"testing"
8+
9+
"goa.design/goa/v3/eval"
10+
)
11+
12+
func TestRunDSL_ReportErrorLocation(t *testing.T) {
13+
eval.Reset()
14+
15+
var expectedFile string
16+
var expectedLine int
17+
expr := &runDSLExpr{
18+
name: "expr",
19+
dsl: func() {
20+
_, file, line, ok := runtime.Caller(0)
21+
eval.ReportError("boom")
22+
if !ok {
23+
t.Fatal("runtime.Caller failed")
24+
}
25+
expectedFile = relativeToWorkdir(t, file)
26+
expectedLine = line + 1
27+
},
28+
}
29+
30+
if err := eval.Register(&runDSLRoot{expr: expr}); err != nil {
31+
t.Fatalf("Register failed: %v", err)
32+
}
33+
34+
err := eval.RunDSL()
35+
if err == nil {
36+
t.Fatal("expected error, got nil")
37+
}
38+
39+
var merr eval.MultiError
40+
if !errors.As(err, &merr) {
41+
t.Fatalf("expected MultiError, got %T", err)
42+
}
43+
if len(merr) != 1 {
44+
t.Fatalf("expected 1 error, got %d", len(merr))
45+
}
46+
47+
got := merr[0]
48+
if got.File != expectedFile {
49+
t.Fatalf("unexpected file: got %q, expected %q", got.File, expectedFile)
50+
}
51+
if got.Line != expectedLine {
52+
t.Fatalf("unexpected line: got %d, expected %d", got.Line, expectedLine)
53+
}
54+
}
55+
56+
func TestRunDSL_ValidationErrorLocation(t *testing.T) {
57+
eval.Reset()
58+
59+
dsl := func() {}
60+
expr := &runDSLExpr{
61+
name: "expr",
62+
dsl: dsl,
63+
validate: errors.New("bad"),
64+
}
65+
66+
if err := eval.Register(&runDSLRoot{expr: expr}); err != nil {
67+
t.Fatalf("Register failed: %v", err)
68+
}
69+
70+
err := eval.RunDSL()
71+
if err == nil {
72+
t.Fatal("expected error, got nil")
73+
}
74+
75+
var merr eval.MultiError
76+
if !errors.As(err, &merr) {
77+
t.Fatalf("expected MultiError, got %T", err)
78+
}
79+
if len(merr) != 1 {
80+
t.Fatalf("expected 1 error, got %d", len(merr))
81+
}
82+
83+
got := merr[0]
84+
if got.File != "" {
85+
t.Fatalf("unexpected file: got %q, expected empty (validation errors are embedded)", got.File)
86+
}
87+
if got.Line != 0 {
88+
t.Fatalf("unexpected line: got %d, expected 0 (validation errors are embedded)", got.Line)
89+
}
90+
91+
expectedFile, expectedLine := dslDeclLocation(t, dsl)
92+
expected := "[" + expectedFile + ":" + itoa(expectedLine) + "] expr: bad"
93+
if got := err.Error(); got != expected {
94+
t.Fatalf("unexpected error message:\n got: %q\nwant: %q", got, expected)
95+
}
96+
}
97+
98+
type runDSLRoot struct {
99+
expr eval.Expression
100+
}
101+
102+
func (*runDSLRoot) EvalName() string { return "test" }
103+
func (*runDSLRoot) DependsOn() []eval.Root {
104+
return nil
105+
}
106+
func (*runDSLRoot) Packages() []string {
107+
return nil
108+
}
109+
func (r *runDSLRoot) WalkSets(walk eval.SetWalker) {
110+
walk(eval.ExpressionSet{r.expr})
111+
}
112+
113+
type runDSLExpr struct {
114+
name string
115+
dsl func()
116+
validate error
117+
}
118+
119+
func (e *runDSLExpr) EvalName() string { return e.name }
120+
func (e *runDSLExpr) DSL() func() { return e.dsl }
121+
func (e *runDSLExpr) Validate() error { return e.validate }
122+
123+
func dslDeclLocation(t *testing.T, fn func()) (file string, line int) {
124+
t.Helper()
125+
126+
pc := reflect.ValueOf(fn).Pointer()
127+
f := runtime.FuncForPC(pc)
128+
if f == nil {
129+
t.Fatal("runtime.FuncForPC returned nil")
130+
}
131+
file, line = f.FileLine(pc)
132+
return relativeToWorkdir(t, file), line
133+
}

0 commit comments

Comments
 (0)