Skip to content
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

Jump to bottom

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

Open
golopot opened this issue Mar 13, 2024 · 1 comment
Open

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

golopot opened this issue Mar 13, 2024 · 1 comment
Labels
gopls Issues related to the Go language server, gopls. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.16.0

Comments

@golopot
Copy link

golopot commented Mar 13, 2024

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
@golopot golopot added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Mar 13, 2024
@gopherbot gopherbot added this to the Unreleased milestone Mar 13, 2024
@ansaba ansaba added the Refactoring Issues related to refactoring tools label Mar 13, 2024
@findleyr
Copy link
Contributor

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

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.16.0 Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.16.0
Development

No branches or pull requests
4 participants
@ansaba @gopherbot @golopot @findleyr