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.

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

False positive for exotic error return pattern #84

Open
joonas-fi opened this issue Jan 9, 2025 · 0 comments
Open

False positive for exotic error return pattern #84

joonas-fi opened this issue Jan 9, 2025 · 0 comments

Comments

@joonas-fi
Copy link

joonas-fi commented Jan 9, 2025

Background

(exotic = I "invented" it myself, haven't seen it used in the wild.)

What the pattern is? Suppose I have a function that reads file content and parses it as timestamp:

func readDateFromFile() (time.Time, error) {
	dateStr, err := os.ReadFile("file-that-has-timestamp-as-content.txt")
	if err != nil {
		return time.Time{}, fmt.Errorf("readDateFromFile: %w", err) // <-- repetition: error prefix & time struct instantiation
	}

	date, err := time.Parse(time.RFC3339, string(dateStr))
	if err != nil {
		return time.Time{}, fmt.Errorf("readDateFromFile: %w", err) // <-- repetition: error prefix & time struct instantiation
	}

	return date, nil
}

This gets a lot more frustrating the more error branches we might have and especially if one needs to add/remove result values. Hence I use this pattern to DRY it up:

func readDateFromFile() (time.Time, error) {
	withErr := func(err error) (time.Time, error) {
		return time.Time{}, fmt.Errorf("readDateFromFile: %w", err)
	}

	dateStr, err := os.ReadFile("file-that-has-timestamp-as-content.txt")
	if err != nil {
		return withErr(err)
	}

	date, err := time.Parse(time.RFC3339, string(dateStr))
	if err != nil {
		return withErr(err)
	}

	return date, nil
}

Obviously it's a bit gross but it has some good things going:

  • it reads nice "return with error"
  • it centralizes adding that error prefix to where the function name itself is and adding any necessary dummy result values such as an empty struct or a possible nil pointer.
  • if you add/remove result values there's only one additional centralized error handling place to change them to

Problem

Ok finally we get to the problem..

unparam detects that the func stored in withErr the first param is always nil:

main.go:10:30: readDateFromFile$1 - result 0 (time.Time) is always nil

Warning

But only if there's a defer statement, see the repro case below..

Repro

Save as main.go:

package main

import (
	"fmt"
	"os"
	"time"
)

func readDateFromFile() (time.Time, error) {
	withErr := func(err error) (time.Time, error) {
		return time.Time{}, fmt.Errorf("readDateFromFile: %w", err)
	}

	dateStr, err := os.ReadFile("file-that-has-timestamp-as-content.txt")
	if err != nil {
		return withErr(err)
	}
	defer func() { /* curiously, this defer is needed for the lint to flag the above `withErr()` */ }()

	date, err := time.Parse(time.RFC3339, string(dateStr))
	if err != nil {
		return withErr(err)
	}

	return date, nil
}

func main () {
	_, _ = readDateFromFile()
}

And runtime environment from Dockerfile:

FROM golang:1.23.1

RUN go install mvdan.cc/unparam@latest

WORKDIR /workdir

Then run it:

$ docker build -t repro .
$ docker run --rm -it -v "$(pwd):/workdir" repro
$ unparam main.go
main.go:10:30: readDateFromFile$1 - result 0 (time.Time) is always nil
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant