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

os: document that NewFile can return nil for invalid fd #20023

Closed
ghost opened this issue Apr 18, 2017 · 7 comments
Closed

os: document that NewFile can return nil for invalid fd #20023

ghost opened this issue Apr 18, 2017 · 7 comments

Comments

@ghost
Copy link

ghost commented Apr 18, 2017

Since the os.NewFile function does not return an error value, one might assume that the function always returns a non-nil value. A line in the docs saying otherwise might save some users a little grief.

@davecheney
Copy link
Contributor

I don't think this is a documentation bug; NewFile can only return nil if someone passes a negative value. Given we cannot change the signature now, I argue this method should panic, as the construction of an invalid value of fd is a programming error.

@ALTree
Copy link
Member

ALTree commented Apr 18, 2017

panic or nil, the doc should say "panics / returns nil when..". The fact that panicing is better than returning nil is a differe issue IMHO.

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 20, 2017
@ALTree ALTree added this to the Go1.9 milestone Apr 20, 2017
@ALTree
Copy link
Member

ALTree commented Apr 21, 2017

Noting in the doc that NewFile can return nil "when the file descriptor uint is not valid" is band-aid; dave wrote on CL 41211:

I don't think this is an improvement. What is an invalid file descriptor? Is it a number larger than ulimit -n? Is it something that was obtained from os.File.Fd() and since closed? Is it a negative fd number? technically yes, but given the function accepts unsigned values.

I think that rather than getting stuck in a documentation rabbit hole, we should do either:

  1. panic, rather than return nil
  2. or, return some *os.File that is already closed.

Moving this to Proposal as suggested to decide if a (vague) doc addition is enough or we also should change the function to panic instead.

@ALTree ALTree added Proposal and removed Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Apr 21, 2017
@ALTree ALTree modified the milestones: Proposal, Go1.9 Apr 21, 2017
@ALTree ALTree changed the title os: docs do not state that NewFile can return nil proposal: make os.NewFile panic on bad file descriptor Apr 21, 2017
@gopherbot
Copy link

CL https://golang.org/cl/41211 mentions this issue.

@rsc rsc changed the title proposal: make os.NewFile panic on bad file descriptor proposal: os: make NewFile panic on bad file descriptor May 15, 2017
@rsc
Copy link
Contributor

rsc commented May 15, 2017

It has always behaved this way (return nil for fd < 0, back to 2008), but it does seem weird that fd=-1 is handled differently from fd=1e9. Perhaps we should return a non-nil *File that returns an appropriate error from future operations (whatever happens when you use the fd later).

@rsc
Copy link
Contributor

rsc commented May 15, 2017

Actually, all the methods already return ErrInvalid for nil *File and the docs for ErrInvalid already say "methods on File will return this error when the receiver is nil". So maybe this is fine and just a doc update is needed.

@rsc
Copy link
Contributor

rsc commented May 15, 2017

Based on discussion with @golang/proposal-review (reflected above), the doc change seems to be the best, least disruptive path forward. Since nil is usable and has been forever, it seems OK to continue that and just document how it happens.

@rsc rsc changed the title proposal: os: make NewFile panic on bad file descriptor proposal: os: document that NewFile can return nil for invalid fd May 15, 2017
@rsc rsc changed the title proposal: os: document that NewFile can return nil for invalid fd os: document that NewFile can return nil for invalid fd May 15, 2017
@rsc rsc modified the milestones: Go1.9, Proposal May 15, 2017
@golang golang locked and limited conversation to collaborators May 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants