cushon
commented
9 years ago Original comment posted by lowasser@google.com on 2014-02-03 at 06:35 PM
I don't think I buy that methods returning an immutable or unmodifiable List should be forced to declare that in their return type, but I could get behind making it an error to return a mix of mutable and immutable types, in cases where that could be feasibly detected.
With regard to the second issue, I don't think there's any facility in error-prone to "propagate information" across files in any way.
Original issue created by opinali@google.com on 2014-02-03 at 06:31 PM
Error pattern:
public List<Stuff> getMyStuff() { List<Stuff> list = nastyDao.list(); return list == null // rare condition ? ImmutableList.of() : list; }
void x() { List<Stuff> stuff = getMyStuff(); if (another rare condition) { stuff.add(new Stuff()); } ... }
Two problems here. First, getMyStuff() should not have any code path that returns an immutable collection if its return type is just List. Ideally this should be detected even with "return Collections.emptyList()", which unfortunately is wildly abused as an optimization for "return new ArrayList<>()" because there's no allocation but it's not always a valid replacement. In code using Guava I see "ImmutableXxx.of()" sometimes abused in the same way. But there are other subtle cases, such as calling many Guava utilities like Maps.uniqueIndex(), Iterables.filter() and others that are easy to forget that produce an immutable result (because there's no explicit "immutable" in the class or method name, although there is in the return type).
Second variant, suppose the problem above is fixed by making getMyStuff() return ImmutableList. Now x() is buggy. This is the call-side version of the same problem. Generally, I'd like error-prone to propagate information about immutable collections, and detect attempts to assign ImmutableX to X (potential bug that should get a waring) or to invoke mutating methods on Immutable collections (the case in x(), should result in error, notice that it would often be possible to detect this even through temps like the "List stuff" dropping the immutable type).