x/tools/gopls: extract function on if err != nil is unidiomatic

[golopot]

gopls version

golang.org/x/tools/gopls v0.15.2

go env

.

What did you do?

Perform extract function code action on the block enclosed by // selection start and // selection end

func F() error {
	// selection start
	a, err := json.Marshal(0)
	if err != nil {
		return fmt.Errorf("1: %w", err)
	}
	b, err := json.Marshal(0)
	if err != nil {
		return fmt.Errorf("2: %w", err)
	}
	// selection end
	fmt.Println(a, b)
	return nil
}

What did you see happen?

func F() error {
	// selection start
	a, b, shouldReturn, returnValue := newFunction()
	if shouldReturn {
		return returnValue
	}
	// selection end
	fmt.Println(a, b)
	return nil
}

func newFunction() ([]byte, []byte, bool, error) {
	a, err := json.Marshal(0)
	if err != nil {
		return nil, nil, true, fmt.Errorf("2: %w", err)
	}
	b, err := json.Marshal(0)
	if err != nil {
		return nil, nil, true, fmt.Errorf("2: %w", err)
	}
	return a, b, false, nil
}

What did you expect to see?

func F() error {
	// selection start
	a, b, err := newFunction()
	if err != nil {
		return err
	}
	// selection end
	fmt.Println(a, b)
	return nil
}

func newFunction() ([]byte, []byte, error) {
	a, err := json.Marshal(0)
	if err != nil {
		return nil, nil, fmt.Errorf("2: %w", err)
	}
	b, err := json.Marshal(0)
	if err != nil {
		return nil, nil, fmt.Errorf("2: %w", err)
	}
	return a, b, nil
}

The heuristic is to special case when every return statement in selection is an error handling return.

The detailed heuristic:

Let f be the function in scope which has an error return value as the last return value. special case if every return statement is of the form return zeros..., expr, where expr is known to be non-nil. In this case the modified call site is values..., err := newFunction(...); if err != nil { return zeros..., err }, and the extracted function should add a return values..., nil at the end.

This refactor would return the same value as before, because when it does return, the value is correct, and when it does not return, the if err != nil would ensure that return return statement is not executed.

Editor and settings

No response

Logs

No response

[findleyr]

Agreed. We are working on improving refactoring, and will fix this.

[gopherbot]

Change https://go.dev/cl/627537 mentions this issue: gopls/internal/golang: fix bad slice append in function extraction

[gopherbot]

Change https://go.dev/cl/627775 mentions this issue: gopls/internal/golang: more idiomatic result naming in extract