Skip to content

Commit

Permalink
DangerousIdentityKey identifies unsafe map and set keys (#1731)
Browse files Browse the repository at this point in the history
DangerousIdentityKey warns where a collection key type does not override equals and hashCode, so comparisons will be done on reference equality only and is likely not what one expects.
  • Loading branch information
schlosna authored Apr 17, 2021
1 parent 86a8a48 commit 7667a35
Show file tree
Hide file tree
Showing 5 changed files with 541 additions and 0 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
- `ClassInitializationDeadlock`: Detect type structures which can cause deadlocks initializing classes.
- `ConsistentLoggerName`: Ensure Loggers are named consistently.
- `PreferImmutableStreamExCollections`: It's common to use toMap/toSet/toList() as the terminal operation on a stream, but would be extremely surprising to rely on the mutability of these collections. Prefer `toImmutableMap`, `toImmutableSet` and `toImmutableList`. (If the performance overhead of a stream is already acceptable, then the `UnmodifiableFoo` wrapper is likely tolerable).
- `DangerousIdentityKey`: Key type does not override equals() and hashCode, so comparisons will be done on reference equality only.

### Programmatic Application

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* (c) Copyright 2021 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.common.collect.Iterables;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.VisitorState;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.util.Name;

/**
* Warns that users should not have a {@link java.util.regex.Pattern} as a key to a Set or Map.
*/
@BugPattern(
name = "DangerousIdentityKey",
summary = "Key type does not override equals() and hashCode, so comparisons will be done on"
+ " reference equality only. If neither deduplication nor lookup are needed,"
+ " consider using a List instead. Otherwise, use IdentityHashMap/Set,"
+ " or an Iterable/List of pairs.",
severity = SeverityLevel.WARNING)
public final class DangerousIdentityKey extends MoreAbstractAsKeyOfSetOrMap {

@Override
protected boolean isBadType(Type type, VisitorState state) {
// Only flag final types, otherwise we'll encounter false positives when presented with overrides.
if (type == null || !type.isFinal()) {
return false;
}
return !implementsMethod(state.getTypes(), type, state.getNames().equals, state)
|| !implementsMethod(state.getTypes(), type, state.getNames().hashCode, state);
}

private static boolean implementsMethod(Types types, Type type, Name methodName, VisitorState state) {
MethodSymbol equals =
(MethodSymbol) state.getSymtab().objectType.tsym.members().findFirst(methodName);
return !Iterables.isEmpty(types.membersClosure(type, false)
.getSymbolsByName(methodName, m -> m != equals && m.overrides(equals, type.tsym, types, false)));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/*
* (c) Copyright 2021 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.VisitorState;
import com.google.errorprone.bugpatterns.AbstractAsKeyOfSetOrMap;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import java.util.List;
import java.util.function.IntPredicate;

/** Extension atop {@link AbstractAsKeyOfSetOrMap} to apply to caches and collectors. */
abstract class MoreAbstractAsKeyOfSetOrMap extends AbstractAsKeyOfSetOrMap {

private static final Matcher<ExpressionTree> GUAVA_CACHE_BUILDER = MethodMatchers.instanceMethod()
.onDescendantOf("com.google.common.cache.CacheBuilder")
.named("build");

private static final Matcher<ExpressionTree> CAFFEINE_CACHE_BUILDER = MethodMatchers.instanceMethod()
.onDescendantOf("com.github.benmanes.caffeine.cache.Caffeine")
.namedAnyOf("build", "buildAsync");

private static final Matcher<ExpressionTree> SET_COLLECTOR = MethodMatchers.staticMethod()
.onClass("java.util.stream.Collectors")
.named("toSet")
.withParameters();

private static final Matcher<ExpressionTree> UNMODIFIABLE_SET_COLLECTOR = MethodMatchers.staticMethod()
.onClass("java.util.stream.Collectors")
.named("toUnmodifiableSet")
.withParameters();

private static final Matcher<ExpressionTree> IMMUTABLE_SET_COLLECTOR = MethodMatchers.staticMethod()
.onClass("com.google.common.collect.ImmutableSet")
.named("toImmutableSet")
.withParameters();

private static final Matcher<MethodInvocationTree> MAP_COLLECTOR = Matchers.allOf(
// We could inspect the fourth argument for hash-based maps in the future. That method
// is relatively uncommon, and the other overloads use HashMap.
argumentCount(count -> count < 4),
MethodMatchers.staticMethod().onClass("java.util.stream.Collectors").named("toMap"));

private static final Matcher<MethodInvocationTree> CONCURRENT_MAP_COLLECTOR = Matchers.allOf(
// We could inspect the fourth argument for hash-based maps in the future. That method
// is relatively uncommon, and the other overloads use HashMap.
argumentCount(count -> count < 4),
MethodMatchers.staticMethod().onClass("java.util.stream.Collectors").named("toConcurrentMap"));

// Unlike map and concurrentMap collectors, 'toUnmodifiableMap' provides no overload to specify the map
// implementation.
private static final Matcher<ExpressionTree> UNMODIFIABLE_MAP_COLLECTOR =
MethodMatchers.staticMethod().onClass("java.util.stream.Collectors").named("toUnmodifiableMap");

private static final Matcher<ExpressionTree> IMMUTABLE_MAP_COLLECTOR = MethodMatchers.staticMethod()
.onClass("com.google.common.collect.ImmutableMap")
.named("toImmutableMap");

private static final Matcher<MethodInvocationTree> HASH_KEYED_METHODS =
Matchers.anyOf(GUAVA_CACHE_BUILDER, CAFFEINE_CACHE_BUILDER);

private static final Matcher<MethodInvocationTree> HASH_KEYED_COLLECTOR_METHODS = Matchers.anyOf(
SET_COLLECTOR,
UNMODIFIABLE_SET_COLLECTOR,
IMMUTABLE_SET_COLLECTOR,
MAP_COLLECTOR,
CONCURRENT_MAP_COLLECTOR,
UNMODIFIABLE_MAP_COLLECTOR,
IMMUTABLE_MAP_COLLECTOR);

@Override
public final Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Description superResult = super.matchMethodInvocation(tree, state);
if (!Description.NO_MATCH.equals(superResult)) {
return superResult;
}
if (HASH_KEYED_METHODS.matches(tree, state)) {
return checkType(tree, ASTHelpers.getResultType(tree), state);
}
if (HASH_KEYED_COLLECTOR_METHODS.matches(tree, state)) {
Type collectorType = ASTHelpers.getResultType(tree);
if (collectorType == null) {
return Description.NO_MATCH;
}
Symbol collectorSymbol = state.getSymbolFromString("java.util.stream.Collector");
if (collectorSymbol == null) {
return Description.NO_MATCH;
}
Type asParameterizedCollector = state.getTypes().asSuper(collectorType, collectorSymbol);
if (asParameterizedCollector == null) {
return Description.NO_MATCH;
}
// Collector<T, A, R>
// [0] T: input
// [1] A: accumulator
// [2] R: result type
Type collectorResultType =
asParameterizedCollector.getTypeArguments().get(2);
return checkType(tree, collectorResultType, state);
}

return Description.NO_MATCH;
}

private Description checkType(Tree tree, Type type, VisitorState state) {
List<Type> typeArguments = type.getTypeArguments();
if (!typeArguments.isEmpty() && isBadType(typeArguments.get(0), state)) {
return describeMatch(tree);
}
return Description.NO_MATCH;
}

private static Matcher<MethodInvocationTree> argumentCount(IntPredicate intPredicate) {
return (methodInvocationTree, state) ->
intPredicate.test(methodInvocationTree.getArguments().size());
}
}
Loading

0 comments on commit 7667a35

Please sign in to comment.