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

CC @robpike

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

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.

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

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.

@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.

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

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.

@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.

#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.

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.)

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.

In the proposed change, it would only trigger the warning if it consumed the reader (Scanner.done == true) and there was an error, and the output would be a stray log to stderr, not a "breaking change" per se.

If someone can think of a better API, I would like to see that proposal, but I can't really imagine what it could do to enforce the error check. If adding vet or logging warnings with bufio.Scanner are deemed too instrusive, there could be bufio.StrictScanner, which is just like bufio.Scanner, except that go vet enforces the error check and maybe there is a panicking finalizer to really drive it home…

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.

If the function's signature is:

func containsLine(rd io.Reader, want string) (..., error)

then the Err() should be checked for sure. For other cases, it may be still good to check Err() as a defensive programming practice.

The general sentiment here seems to be that printing to stderr at some arbitrary time after the code executes to tell you it forgot to do something is problematic in the standard library, and that we should probably look into the vet check instead.

Do I have that right?

I think the vet check is preferable except for the issue of false positives.

Another approach besides bufio.StrictScanner is to add bufio.LaxScanner as a type alias of bufio.Scanner that vet can deliberately ignore.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@carlmjohnson

I think the vet check is preferable except for the issue of false positives.

False positives of the vet check would also (usually) be false positives for printing a warning. I don't think they are worse for vet than here.

FWIW I've often used Scanner on strings.Reader where there's no possibility of error.

"false positives" of the vet check would also (usually) be false positives for printing a warning. I don't think they are worse for vet than here.

They would be worse than if @bcmills' suggestion was implemented, AFAICS, because vet would probably find it harder to detect cases where the underlying reader cannot return an error:

@rogpeppe Fair enough.

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

No change in consensus, so declined.
— rsc for the proposal review group