std::io: New ErrorKind value InvalidData

[mzabaluev]

This takes the cases from InvalidInput where a data format error
was encountered. This is different from the documented semantics
of InvalidInput, which more likely indicate a programming error.

Fixesrust-lang/rfcs#906

[rust-highfive]

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

[mzabaluev]

This is a PR for rust-lang/rfcs#906.

[mzabaluev]

To bikeshed a bit more, I'd rename InvalidInput to InvalidParam, but it's perhaps too late for this breaking change.

[Stebalien]

This should probably stay as-is. CString::new returns a NulError when its argument (input) contains a null.

[mzabaluev]

The character data may have come from outside the program. In general, it's not a programming error for strings to contain null bytes, so to me it belongs in the same class of data conversion failures as, say, encountering malformed UTF-8 in an input operation that reads to a string.

[alexcrichton]

I'm not sure I personally understand the distinction here, it seems like selecting one or the other can be ambiguous from time to time and the distinction may not actually carry much weight in the wild. Could you elaborate a little more on how these two are distinct and how they should be interpreted separately?

[alexcrichton]

cc @aturon

[mzabaluev]

@alexcrichton Other than data format errors, InvalidInput is currently used for e.g. seeking to an invalid offset. The latter is clearly a programming error, while malformed data can occur on input of a correctly working program.

[alexcrichton]

Hm I guess to me those seem like very similar classes of errors as they're both "invalid inputs" to the function. I think that InvalidData as an error type for data being produced makes a little more sense to me, for example read_to_string returning InvalidData can be rationalized in my head a bit more, but I don't personally see a difference between seeking to a negative offset and providing a string with an interior nul byte.

[mzabaluev]

read_to_string may be reading from a file, in which case InvalidInput arises due to no fault of the program.

[mzabaluev]

#25406 is an alternative change where, conversely, the invalid-parameter cases are reclassified from InvalidInput to InvalidParam.

[aturon]

FWIW, I agree that it would be good to have distinct errors for "You passed in arguments that broke the contract" versus "The OS returned some data that was not in the expected form".

I'm not in love with InvalidData for the latter, though; it's a bit too vague. Maybe InvalidOutput?

I also agree with others that the CString related errors should stay as InvalidInput since they represent contract violations on the Rust side.

[bluss]

I'm not in love with InvalidData for the latter, though; it's a bit too vague. Maybe InvalidOutput?

When we are decoding a stream of text, it's invalid input to the program, not output.

[aturon]

@bluss

I'm not in love with InvalidData for the latter, though; it's a bit too vague. Maybe InvalidOutput?

When we are decoding a stream of text, it's invalid input to the program, not output.

The error variant is relative to the operation being performed, not the broader context of the program. We need the pair of variants to clearly signify whether the problem was with the arguments passed to the operation (InvalidInput) or with the data produced by the operation (hence InvalidOutput).

[mzabaluev]

I have updated the branch, thanks for your input. In my personal opinion, there is still potential for confusion between InvalidInput and InvalidOutput. I'd rename the former to clearly mean "invalid argument used for the operation", but I don't see how it can be done in a backwards-compatible way; the whole issue does not seem big enough to demand more drastic changes.

[bluss]

My head is spinning. Run this past a bit more people to see what they think? Why do we use the terms Input / Output when we don't mean I/O? 😄 InvalidParameter, InvalidArgument, anything like that is better. W.r.t if data we process using the APIs is regarded as output from something else or input to us, or the reverse, that I don't know. I suggest just saying data for that, hence InvalidData.

[aturon]

@bluss

The usage here is just the notion of arguments being "input" to a function, and the results it produces being its "output".

Admittedly, InvalidArgument and InvalidResult would probably be more clear, but that would require a breaking change.

[aturon]

OK, after talking to @alexcrichton for a while, and stewing a bit more on the comments here, I think @bluss is right and the names InvalidInput and InvalidOutput, while technically meaningful, are likely to be too confusing in this context. Given that we're committed to InvalidInput, I'm now on board with InvalidData as the originally proposed complement.

[bluss]

I would be happier. Consider that we use InvalidOutput/InvalidData for the case when stdin contains non-utf-8 data.

[mzabaluev]

Rebased and condensed into one commit.

[alexcrichton]

This stability tag should now say 1.1.0, and the feature name can be something like io_invalid_data.

Could you also expand the docs here a bit to clarify what InvalidData means with respect to InvalidInput?

[mzabaluev]

Sorry, I have copy-pasted it without thinking.

[mzabaluev]

Updated and rebased the commit, now with a more descriptive doc text and corrected stability attribute.

[alexcrichton]

Gah oops, sorry but I forgot we've entered 1.2 territory at this point, so this should actually be 1.2 instead of 1.1. Other than that though this looks good to me!

[mzabaluev]

Note that this is, strictly speaking, a breaking change: error-handling code that matches for InvalidInput may work differently between 1.0 and the version where this change lands.
This means it may need to be deferred to 2.0, when InvalidInput can also be renamed.

[alexcrichton]

@bors: r+ 0ad019f

[bors]

⌛ Testing commit 0ad019f with merge 8ffe552...

[bors]

💔 Test failed - auto-linux-64-opt

[alexcrichton]

@bors: retry

On Mon, Jun 1, 2015 at 11:57 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-opt
http://buildbot.rust-lang.org/builders/auto-linux-64-opt/builds/5172

—
Reply to this email directly or view it on GitHub
#25246 (comment).

[bors]

⚡ Previous build results for auto-linux-32-nopt-t, auto-linux-64-nopt-t, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt are reusable. Rebuilding only auto-linux-32-opt, auto-linux-64-opt...

[alexcrichton]

@bors: retry clean force

[bors]

⌛ Testing commit 0ad019f with merge 613e57b...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt