The type was answering a question nobody asked

Background

That is the whole setting, and it is worth seeing before the types. One source of bytes, two destinations, a counter, and a threshold at which the second destination is swapped for one that discards.

Under the limit

Once the count passes the maximum

At the moment the branch is swapped, the callback fires, and the partial copy already sitting in the cache buffer is thrown away. The response keeps flowing either way. Everything this change touches is in that one callback.

The callback used to be typed as a Guava Function, whose wildcard return says the value is not interesting.

uPortal-rendering/.../cache/LimitingTeeOutputStream.java, before

    private final Function<LimitingTeeOutputStream, ?> limitReachedCallback;

A Function exists to return something. So calling one and dropping the result is the pattern ErrorProne's CheckReturnValue reports. For that check to reach a Guava method, Guava has to opt its own package in, which it does through a package-level annotation. That annotation lives in the Guava jar, which is not part of this checkout, so the page takes it from the library rather than from anything checkable here. The invocation site looked like this.

uPortal-rendering/.../cache/LimitingTeeOutputStream.java, before

            if (this.limitReachedCallback != null) {
                // Named 'unused' to satisfy ErrorProne's CheckReturnValue; the callback's
                // signature is Function<T,?> but its return value is intentionally discarded.
                @SuppressWarnings("unused")
                Object unused = this.limitReachedCallback.apply(this);
            }

Two comment lines, an annotation, and an assignment, to call one callback. The pull request body says that block landed in the immediately preceding pull request. The checkout is a single commit, so it cannot confirm that.

Intuition

Choosing the type that matches the intent removes the problem. Nothing suppresses CheckReturnValue afterwards, because there is no return value to discard: Consumer.accept is declared void.

The last two rows are connected, and they are the part that is easy to miss. The type change did not only silence a check. It changed what a caller is allowed to write.

Why the call sites collapsed

Both callbacks do one thing: clear the half-collected cache. The methods that do it return void.

In Java, a lambda whose body is a single void method call is a statement, not an expression with a value. That satisfies a Consumer and cannot satisfy a Function, which needs something to return. So under the old type, a caller had no way to write the short form, and had to spell out an anonymous class ending in return null.

uPortal-rendering/.../cache/CachingPortletOutputHandler.java:104, before. The stream call site at :130 is the same shape.

                            new Function<LimitingTeeWriter, Object>() {
                                @Override
                                public Object apply(LimitingTeeWriter input) {
                                    // Limit hit, clear the cache
                                    clearCachedWriter();
                                    return null;
                                }
                            });

uPortal-rendering/.../cache/CachingPortletOutputHandler.java:104, after

                            input -> clearCachedWriter());

The old type required a value back, so the anonymous class had to end in return null.

Code walkthrough

The flow, end to end. A portlet writes bytes. The tee counts them and copies them to a cache buffer. Past the limit, it redirects the branch to a discarding stream and invokes the callback, which throws away the partial copy. The change is entirely in how that callback is typed and invoked.

$ grep -c '^diff --git' diff.txt
6

Six files: two production classes, one caller, and three test files.

1. The two classes, in step

uPortal-rendering/.../cache/LimitingTeeOutputStream.java:32

    private final Consumer<LimitingTeeOutputStream> limitReachedCallback;

LimitingTeeWriter.java:32 carries the same declaration at the same line number, with Writer in place of OutputStream. The two classes are the same design twice, one for bytes and one for characters, differing in the names of their counters and in which discarding sentinel they use. Every change in this diff lands on both.

2. The invocation

uPortal-rendering/.../cache/LimitingTeeOutputStream.java:83

            if (this.limitReachedCallback != null) {
-                // Named 'unused' to satisfy ErrorProne's CheckReturnValue; the callback's
-                // signature is Function<T,?> but its return value is intentionally discarded.
-                @SuppressWarnings("unused")
-                Object unused = this.limitReachedCallback.apply(this);
+                this.limitReachedCallback.accept(this);
            }

The null guard stays, because the three-argument constructors of both classes pass null for the callback.

3. The two mechanisms, separated

The old block did two different things, and the comment described them as one.

The assignment is what satisfied CheckReturnValue. That check fires when a call's result is discarded, meaning the call stands alone as a statement. Assigning the result to a variable means it is no longer discarded, so the check has nothing to report. Note that no @SuppressWarnings("CheckReturnValue") appears anywhere; the assignment did the work.

The annotation then dealt with the problem the assignment created. A local variable that is written and never read draws ErrorProne's UnusedVariable, and @SuppressWarnings("unused") is what silences that.

Which of the two mattered for the build depends on severity, and the project configures none of it.

build.gradle:57

    errorprone 'com.google.errorprone:error_prone_core:2.3.4'

There is no errorprone { } block anywhere in the build, no per-check enable, disable, or severity override, and nothing setting -Werror, the flag that would turn warnings into build failures. So every check runs at whatever ErrorProne 2.3.4 ships as its default severity.

Those defaults are the one input this page cannot get from the checkout. ErrorProne is a compile-time dependency, resolved from a repository, and no copy of it is here to read. The values below come from ErrorProne's own documentation for those two checks, not from anything in uPortal, so treat the conclusion that follows as resting on them.

On those severities, the assignment was required to compile and the annotation was tidying. Either way, the annotation addressed the unused local, and the comment above it named the return-value check.

4. The call site that could not change type

One file in this diff is in a different package and changes for a different reason.

uPortal-webapp/.../events/aggr/PortalRawEventsAggregatorImplTest.java:178

                                Boolean ignored =
                                        ((Function<PortalEvent, Boolean>)
                                                        invocation.getArguments()[3])
                                                .apply(

Here the Function is the parameter type of the method under test, so the test cannot choose a different one. The same discarded-result problem applies, and the same assignment fixes it, with a local named ignored and no annotation.

When you own the interface, change the type. When you do not, the assignment is the answer. This diff contains one of each.

5. The argument no caller reads

Both classes pass themselves to the callback, and every callback in the checkout ignores it. The two production lambdas are input -> clearCachedWriter() and input -> clearCachedStream(); the two test callbacks reset a buffer they captured instead. No caller reads the argument. CachingPortletOutputHandler is the only production code that constructs either class; the two test files construct them eight times between them.

The parameter is still declared and still supplied. Nothing in the record says why it stayed. My reading is that it leaves room for a callback that wants the stream, which a parameterless Runnable would foreclose.

6. How far this reaches

The Guava import leaves five files, and java.util.function.Consumer arrives in two. The three other files need no new import, because a lambda needs none.

Guava itself stays. It is not declared in this module directly; it arrives transitively as an api dependency of uPortal-utils-core. Counting what remains in the module:

$ grep -rn 'import com.google.common' --include='*.java' uPortal-rendering/src | wc -l
33

Six of those 33 are still com.google.common.base.Function, found with:

$ grep -rn 'import com.google.common.base.Function' --include='*.java' uPortal-rendering/src | wc -l
6

So the same import exists at six other places in this module. What those six do with their Function is a separate question from this change.

One constraint worth knowing, since it explains why the fix uses what it uses. The project's AGENTS.md bans Java 9 and later language features and APIs. Consumer and lambdas are both Java 8, so this change stays inside that rule.

7. What the checkout cannot confirm

The pull request body tells a story about how this change came to be a separate pull request: a contributor proposed the Consumer version on another branch, the author force-pushed a cherry-pick onto the earlier pull request, and that earlier pull request had already merged with the @SuppressWarnings version. So this lands as a separate follow-up, which the body says credits the contributor as co-author.

Most of that is not checkable here. The checkout is a single commit with no ancestor history, so the referenced pull requests, the branch, and the force-push cannot be inspected. The file list and the per-file description in the body do match the diff exactly.

The co-author credit is checkable, and it does not match. The single commit carries two Co-Authored-By trailers, and neither names the contributor the body says is credited. What that means is not something the checkout can tell you; a credit can also be recorded in a merge commit this clone does not have.

Quiz

Five questions about why the change is shaped the way it is. Click an option to see the answer.