Skip to content

Commit

Permalink
Fix nil checks and improve error message
Browse files Browse the repository at this point in the history
The new mg.F function is the root of the nil-pointer error that can happen
when someone accidentally calls mg.Deps(TargetA()). I have fixed the check
for whether a function was passed so that it won't panic and included
the usage for mg.F in the error message.

I then added a similar check, is the target a function, to
Deps/SerialDeps/CxtSerialDeps functions so that we can give a better
error message when it's passed a non-function. The error message
otherwise will reference mg.F which I don't think will help point people
to the real problem, which is how they called mg.Deps/SerialDeps.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
  • Loading branch information
carolynvs committed Jan 10, 2022
1 parent d9c0439 commit 5cd247a
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 6 deletions.
10 changes: 7 additions & 3 deletions mg/deps.go
Expand Up @@ -130,13 +130,17 @@ func runDeps(ctx context.Context, fns []Fn) {
func checkFns(fns []interface{}) []Fn {
funcs := make([]Fn, len(fns))
for i, f := range fns {
if f == nil {
panic("A dependency of the current target was defined improperly, with parenthesis. Dependencies should be defined as mg.Deps(MyDep), not mg.Deps(MyDep())")
}
if fn, ok := f.(Fn); ok {
funcs[i] = fn
continue
}

// Check if the target provided is a not function so we can give a clear warning
t := reflect.TypeOf(f)
if t == nil || t.Kind() != reflect.Func {
panic(fmt.Errorf("non-function used as a target dependency: %T. The mg.Deps, mg.SerialDeps and mg.CtxDeps functions accept function names, such as mg.Deps(TargetA, TargetB)", f))
}

funcs[i] = F(f)
}
return funcs
Expand Down
2 changes: 1 addition & 1 deletion mg/deps_internal_test.go
Expand Up @@ -45,7 +45,7 @@ func TestDepWasNotInvoked(t *testing.T) {
t.Fatal("expected panic, but didn't get one")
}
gotErr := fmt.Sprint(err)
wantErr := "A dependency of the current target was defined improperly, with parenthesis"
wantErr := "non-function used as a target dependency: <nil>. The mg.Deps, mg.SerialDeps and mg.CtxDeps functions accept function names, such as mg.Deps(TargetA, TargetB)"
if !strings.Contains(gotErr, wantErr) {
t.Fatalf(`expected to get "%s" but got "%s"`, wantErr, gotErr)
}
Expand Down
4 changes: 2 additions & 2 deletions mg/fn.go
Expand Up @@ -100,8 +100,8 @@ func (f fn) Run(ctx context.Context) error {

func checkF(target interface{}, args []interface{}) (hasContext, isNamespace bool, _ error) {
t := reflect.TypeOf(target)
if t.Kind() != reflect.Func {
return false, false, fmt.Errorf("non-function passed to mg.F: %T", target)
if t == nil || t.Kind() != reflect.Func {
return false, false, fmt.Errorf("non-function passed to mg.F: %T. The mg.F function accepts function names, such as mg.F(TargetA, \"arg1\", \"arg2\")", target)
}

if t.NumOut() > 1 {
Expand Down
10 changes: 10 additions & 0 deletions mg/fn_test.go
Expand Up @@ -114,6 +114,16 @@ func TestFuncCheck(t *testing.T) {
if err == nil {
t.Error("expected func(*int) error to be invalid")
}

defer func() {
if r := recover(); r !=nil {
t.Error("expected a nil function argument to be handled gracefully")
}
}()
_, _, err = checkF(nil, []interface{}{1,2})
if err == nil {
t.Error("expected a nil function argument to be invalid")
}
}

func TestF(t *testing.T) {
Expand Down

0 comments on commit 5cd247a

Please sign in to comment.