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

Warn on usage of Maps.transformValues #2518

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.baseline.errorprone;

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;

@AutoService(BugChecker.class)
@BugPattern(
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
severity = SeverityLevel.ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the issue that spurred this PR, I think this should be a SUGGESTION as there are many uses of Maps#transformValues that I would not want to robotically replace with eager transformations.

There are many places where we very intentionally use Maps.transformValues to avoid intermediate collection copies (due to both allocations, CPU, and memory tradeoffs) in places where we're handling large volumes of transformations. Some public examples would be AtlasDB's use throughout much of its data processing.

summary = "Disallow usage of Guava Map's .transformValues().")
public final class DangerousGuavaTransformValuesUsage extends BugChecker
implements BugChecker.MethodInvocationTreeMatcher {
private static final long serialVersionUID = 1L;
private static final String ERROR_MESSAGE = "The transformValues API of Guava Maps creates a lazily evaluated "
+ "view of the source Map. Repeated access of the same key leads to repeated evaluations of the "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how feasible this would be, but a more specifically targeted checker around reuse of lazily transformed collections might lead to higher signal warnings. Possibly couple this with checks identified RPCs in lazy evaluation might also be high signal (as those may be places that should be utilizing a batch API if available).

+ "transformer function. This is often unintended and can cause severe performance degradation."
+ "Where this is actually intended, suppress this warning.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, I'm hesitant to treat lazy transforms as something that completely needs to be avoided or surprised. If we were to do that, do we block all the other lazy APIs from Guava (e.g. Iterators/(Fluent)Iterables/Collections2/Lists/Sets/Maps transform* methods)? Do we forbid returning Stream from methods as these are lazy until terminal operation? Do we forbid all Guava APIs that return collection/map views?

I would argue no we should not forbid these to all of the above. That said, in general most places should probably prefer eager transforms by default (especially as it is simple to stream & collect into an Immutable* or unmodifiable collection), but I don't think it is an error to use these well documented APIs as they were designed.


private static final Matcher<ExpressionTree> TRANSFORM_VALUES_CALL = MethodMatchers.instanceMethod()
.onExactClass("com.google.common.collect.Maps")
.named("transformValues");

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!TRANSFORM_VALUES_CALL.matches(tree, state)) {
return Description.NO_MATCH;
}

// Fail on any 'transformValues(...)' usage
return buildDescription(tree).setMessage(ERROR_MESSAGE).build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* (c) Copyright 2022 Palantir Technologies Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.palantir.baseline.errorprone;

import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

class DangerousGuavaTransformValuesUsageUsageTest {
private CompilationTestHelper compilationHelper;

@BeforeEach
public void before() {
compilationHelper = CompilationTestHelper.newInstance(DangerousGuavaTransformValuesUsage.class, getClass());
}

@Test
public void should_error_when_transforms_values_is_used() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Map;",
"import com.google.common.collect.Maps;",
"class Test {",
" public static final void main(String[] args) {",
" Map<Integer, Integer> map = Map.of(1, 2, 3, 4, 5, 6);",
" Maps.transformValues(map, value -> value + 1);",
" }",
"}")
.doTest();
}
}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2518.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Warn on usage of Maps.transformValues
links:
- https://github.com/palantir/gradle-baseline/pull/2518