proposal: bufio: Add finalizer to Scanner that warns if .Err not called

[earthboundkid]

It is unfortunately common for users to incorrectly use Scanner without calling .Err(). (In particular, I've seen it in three different social media posts recently.) We can't change the API, but we can add a runtime.Finalizer that will print a warning to stderr if a Scanner gets garbage collected with Scanner.scanCalled and Scanner.done true but not (new field I would add) Scanner.errCalled.

[earthboundkid]

Cf. #17747 which is probably a better idea but more intrusive. In either case, the way to silence the warning is _ = s.Err()

[ianlancetaylor]

CC @robpike

[robpike]

Interesting idea, but isn't this more a vet-check kind of thing?

[ericlagergren]

I do not think the standard library should ever write unprompted warnings to stderr. It means any code parsing stderr (logs, etc.) has to handle these new warnings. And if you throw away stderr then the warning is useless.

[earthboundkid]

Go writes to standard error if an http.Handler panics.

[earthboundkid]

I’d be happy with #17747. It wouldn’t help people who don’t test their code, but they’re sort of hopeless anyway. The finalizer has the problem that it might not run at all if the program exits before garbage collection.

[ericlagergren]

@carlmjohnson net/http uses ErrorLog, which uses the log package when nil. So, it’s trivially configurable.

The only package I can think of is crypto/rand, and that only occurs on startup and under abnormal conditions.

[seankhliao]

Having net/http recover panics and print them was considered a mistake: #25245

[earthboundkid]

You should only see the warning if your program is incorrectly written.

Thinking more, the real objection is that logging requires package os and bufio should not have it as a requirement.

[ericlagergren]

@carlmjohnson sure, but incorrectly written programs can still run correctly. Perhaps Scan never returns an error or reaching EOF is irrelevant. This proposal would start writing to stderr, potentially causing problems.

It's why Rob's idea of a vet check seems like the correct approach to me: you catch most incorrect uses of the API ahead of time without causing new problems by writing to stderr.

[earthboundkid]

#17747 is marked as “Needs Decision”. From the feedback here, it seems to be the preferred option. I would also be happy with it. I’d be glad to close this issue if there is consensus to revive that issue and implement it.

[bcmills]

A less-invasive alternative here would be to only emit the warning if the Err would have returned a non-nil error. That would prevent spurious output for the small subset of cases where an error really is impossible, but at the cost of missing the diagnostic in the typical case.

(FWIW, I prefer #17747 too.)

[mpx]

Wouldn't mandating checking Err() be a breaking change or cause unwanted side-effects?

There is likely non-buggy code that doesn't need to call Err(). A somewhat contrived example:

func containsLine(rd io.Reader, want string) bool {
  s := bufio.NewScanner(rd)
  for s.Scan() {
    if s.Text() == want {
      return true
    }
  }
  return false
}

Not great code, but potentially fine depending on the context.

I think it would be better to provide an easy to use + hard to misuse replacement API instead. Package docs (and maybe linters) can recommend the better API instead.