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

Added OptionalOrElseGetConstant error prone check. #1401

Merged
merged 34 commits into from
Jun 17, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
bf17125
Added OptionalOrElseGetConstant error prone check.
aioobe Jun 11, 2020
6cb06f5
Excavator: Upgrade buildscript dependencies (#1402)
svc-excavator-bot Jun 11, 2020
f8fa3d1
Excavator: Upgrade buildscript dependencies (#1403)
svc-excavator-bot Jun 11, 2020
09ca77a
--write-locks no longer implicitly runs checkClassUniqueness task (#1…
iamdanfox Jun 11, 2020
b99ee1d
Autorelease 3.24.0
svc-autorelease Jun 11, 2020
6604106
Excavator: Upgrade dependencies (#1399)
svc-excavator-bot Jun 11, 2020
93bca37
Improvement: Upgrade Checkstyle to 8.33 (#1404)
robert3005 Jun 11, 2020
4407bb5
Autorelease 3.25.0
svc-autorelease Jun 11, 2020
d12519d
Fix: Adjust checkstyle config for new version (#1409)
robert3005 Jun 12, 2020
0401037
Autorelease 3.25.1
svc-autorelease Jun 12, 2020
329b939
Excavator: Render CircleCI file using template specified in .circleci…
svc-excavator-bot Jun 12, 2020
1eaa6ce
Check now also covers other simple expressions. Readme updated.
aioobe Jun 12, 2020
eb4d55b
StrictUnusedVariable handles Java 14 records (#1412)
ferozco Jun 12, 2020
7209e9b
Autorelease 3.26.0
svc-autorelease Jun 12, 2020
ce1d2e1
Ran spotlessApply.
aioobe Jun 12, 2020
54f4f32
Added a compile time constant test.
aioobe Jun 12, 2020
03c85a0
Reworded description in README.
aioobe Jun 12, 2020
87eb1ff
Changed to multiple returns.
aioobe Jun 12, 2020
c4fd4b2
Don't wrap method declaration (#1416)
hesselink Jun 16, 2020
02303a8
Autorelease 3.27.0
svc-autorelease Jun 16, 2020
e9309b7
Excavator: Upgrade buildscript dependencies (#1417)
svc-excavator-bot Jun 17, 2020
ac4956d
Excavator: Ensure consistent gradle/publish-(jar|dist).gradle files (…
svc-excavator-bot Jun 17, 2020
1d0cc65
Excavator: Upgrade dependencies (#1419)
svc-excavator-bot Jun 17, 2020
13fc372
Added changelog entry.
aioobe Jun 17, 2020
3bd2a1e
Improve gradle-baseline-java integration with IntelliJ import (#1411)
aldexis Jun 17, 2020
f1c1f63
Autorelease 3.28.0
svc-autorelease Jun 17, 2020
3b331f1
Added OptionalOrElseGetConstant error prone check.
aioobe Jun 11, 2020
aa20422
Check now also covers other simple expressions. Readme updated.
aioobe Jun 12, 2020
15a48bf
Ran spotlessApply.
aioobe Jun 12, 2020
83993a1
Added a compile time constant test.
aioobe Jun 12, 2020
e135dff
Reworded description in README.
aioobe Jun 12, 2020
d5e0139
Changed to multiple returns.
aioobe Jun 12, 2020
7ece87b
Added changelog entry.
aioobe Jun 17, 2020
689346f
Merge branch 'orElseGet-constant' of github.com:aioobe/gradle-baselin…
aioobe Jun 17, 2020
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
- `NonComparableStreamSort`: Stream.sorted() should only be called on streams of Comparable types.
- `DangerousStringInternUsage`: Disallow String.intern() invocations in favor of more predictable, scalable alternatives.
- `OptionalOrElseThrowThrows`: Optional.orElseThrow argument must return an exception, not throw one.
- `OptionalOrElseGetValue`: Prefer `Optional.orElse(value)` over `Optional.orElseGet(() -> value)` for trivial expressions.
- `LambdaMethodReference`: Lambda should use a method reference.
- `SafeLoggingExceptionMessageFormat`: SafeLoggable exceptions do not interpolate parameters.
- `StrictUnusedVariable`: Functions shouldn't have unused parameters.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* (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.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher;
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.IdentifierTree;
import com.sun.source.tree.LambdaExpressionTree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;

@AutoService(BugChecker.class)
@BugPattern(
name = "OptionalOrElseGetValue",
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
severity = SeverityLevel.WARNING,
summary = "If lambda passed to Optional#orElseGet returns a simple expression, use Optional#orElse instead")
public final class OptionalOrElseGetValue extends BugChecker implements MethodInvocationTreeMatcher {

private static final long serialVersionUID = 1L;
private static final Matcher<ExpressionTree> OR_ELSE_GET_METHOD =
MethodMatchers.instanceMethod().onExactClass("java.util.Optional").named("orElseGet");
private static final Matcher<ExpressionTree> COMPILE_TIME_CONSTANT = new CompileTimeConstantExpressionMatcher();

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
match:
{
if (!OR_ELSE_GET_METHOD.matches(tree, state)) {
break match;
}

ExpressionTree orElseGetArg = tree.getArguments().get(0);

if (orElseGetArg.getKind() != Tree.Kind.LAMBDA_EXPRESSION) {
break match;
}

LambdaExpressionTree lambdaExpressionTree = (LambdaExpressionTree) orElseGetArg;
LambdaExpressionTree.BodyKind bodyKind = lambdaExpressionTree.getBodyKind();

if (bodyKind != LambdaExpressionTree.BodyKind.EXPRESSION) {
break match;
aioobe marked this conversation as resolved.
Show resolved Hide resolved
}

ExpressionTree expressionBody = (ExpressionTree) lambdaExpressionTree.getBody();
if (COMPILE_TIME_CONSTANT.matches(expressionBody, state)
aioobe marked this conversation as resolved.
Show resolved Hide resolved
|| isTrivialExpression(expressionBody)
|| isTrivialSelect(expressionBody)) {
return buildDescription(tree)
.setMessage("Prefer Optional#orElse instead of Optional#orElseGet for compile time constants")
.addFix(SuggestedFix.builder()
.merge(SuggestedFixes.renameMethodInvocation(tree, "orElse", state))
.replace(orElseGetArg, state.getSourceForNode(expressionBody))
.build())
.build();
}
}
return Description.NO_MATCH;
}

private static boolean isTrivialExpression(ExpressionTree tree) {
return tree instanceof LiteralTree || tree instanceof IdentifierTree;
}

private static boolean isTrivialSelect(ExpressionTree tree) {
return tree instanceof MemberSelectTree && isTrivialExpression(((MemberSelectTree) tree).getExpression());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
/*
* (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.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

public final class OptionalOrElseGetValueTest {

private CompilationTestHelper compilationHelper;
private RefactoringValidator refactoringTestHelper;

@BeforeEach
public void before() {
compilationHelper = CompilationTestHelper.newInstance(OptionalOrElseGetValue.class, getClass());
refactoringTestHelper = RefactoringValidator.of(new OptionalOrElseGetValue(), getClass());
}

@Test
public void testOrElseGetStringLiteral() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" // BUG: Diagnostic contains: Prefer Optional#orElse",
" private final String string = Optional.of(\"hello\").orElseGet(() -> \"world\");",
"}")
.doTest();
}

@Test
public void testOrElseGetVariable() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" String s = \"hello\";",
" // BUG: Diagnostic contains: Prefer Optional#orElse",
" private final String string = Optional.of(\"hello\").orElseGet(() -> s);",
"}")
.doTest();
}

@Test
public void testOrElseGetMethodInvocation() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" String s = \"hello\";",
" private final String string = Optional.of(\"hello\").orElseGet(() -> s.toString());",
"}")
.doTest();
}

@Test
public void testOrElseGetConstantString() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" private static final String constant = \"constant\";",
" // BUG: Diagnostic contains: Prefer Optional#orElse",
" private final String string = Optional.of(\"hello\").orElseGet(() -> constant);",
"}")
.doTest();
}

@Test
public void testOrElseGetThisField() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" private static double field = Math.random();",
" // BUG: Diagnostic contains: Prefer Optional#orElse",
" private final double string = Optional.of(0.1).orElseGet(() -> this.field);",
"}")
.doTest();
}

@Test
public void testOrElseGetOtherField() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" private Test test = new Test();",
" private double field = Math.random();",
" // BUG: Diagnostic contains: Prefer Optional#orElse",
" private final double string = Optional.of(0.1).orElseGet(() -> test.field);",
"}")
.doTest();
}

@Test
public void testOrElseGetCompileTimeConstant() {
compilationHelper
.addSourceLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" private final String part1 = \"Hello \";",
" private final String part2 = \"World\";",
" // BUG: Diagnostic contains: Prefer Optional#orElse",
" private final String string = Optional.of(\"str\").orElseGet(() -> part1 + part2);",
"}")
.doTest();
}

@Test
public void testReplacementOfLiteral() {
refactoringTestHelper
.addInputLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" private final boolean b = Optional.of(true).orElseGet(() -> false);",
"}")
.addOutputLines(
"Test.java",
"import java.util.Optional;",
"class Test {",
" private final boolean b = Optional.of(true).orElse(false);",
"}")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}
}