search

google / guava

Google core libraries for Java
Apache License 2.0
50.19k stars 10.91k forks source link

Files.readFirstLine and LineReader.readLine should be @Nullable #2611

Open michaelhixson opened 8 years ago

michaelhixson commented 8 years ago

Files.readFirstLine(File, Charset) and LineReader.readLine() are documented to possibly return null, but these methods are not annotated with @Nullable, and I think they should be.

These methods are public and final and they don't override any super method. I can't imagine that adding the @Nullable annotation could cause any harm.

Side note: I am trying gauge interest in adding more @Nullable annotations throughout the codebase. These methods seem like the safest, most obvious candidates, but there are other candidates. If you all aren't interested in marking these @Nullable, then I won't pursue the issue further.

cpovirk commented 8 years ago

We have some tooling for this: https://github.com/google/error-prone/commit/8182e1730618d33f88718bd59a1d7bca26025ff9

We used it to generate https://github.com/google/guava/commit/58986e98ddc00501be697faef89c8ac454b94ac5 on common.base. I will run it on the other packages and see what it spits out.

cpovirk commented 8 years ago

The tool identifies readFirstLine but not readLine (I think because CharSource.readFirstLine is @Nullable but Queue.poll() is, while nullable, not @Nullable :))

Let me see how hard the tool's output is to submit (since it might break some Google users' null checks). If it goes well, then you can have a look and see whether you'd like to do more.

cpovirk commented 8 years ago

I see a double-digit number of nullability-test failures from adding @Nullable to various APIs, so we probably won't pursue this further. Sorry :(

michaelhixson commented 8 years ago

No worries. I hope the failures were false positives!

cpovirk commented 8 years ago

Looks like mostly, though I didn't audit them all :) A lot of them were methods like...

getOrDefault(K key, @Nullable V defaultValue)

...where the return might be null but only if defaultValue is null. This is what @PolyNull is for, but we haven't taken the plunge to the Checker Framework, so we're stuck with plain @Nullable at the moment.

We do have someone internally who's been working on nullability checks (including creating the tool I referred to above), so hopefully we'll have some more progress to report in time.