Skip to content

Commit c60b8d1

Browse files
ksobolewksobolew
committed
Add RequireNonNullMessageChecker
This is a check to find some common cases of malformed `requireNonNull` calls. See the documentation for more details.
1 parent d06116a commit c60b8d1

File tree

5 files changed

+433
-2
lines changed

5 files changed

+433
-2
lines changed

‎README.md

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ extensions.
66

77
## Bug Checkers
88

9+
### Checkers for annotation-related issues
10+
911
* `DeprecatedApi` - Check for usages of `@Deprecated` APIs.
1012
* `TrinoExperimentalSpi` - Check for usages of Trino `@Experimental` SPIs.
1113

@@ -18,13 +20,64 @@ Both checkers can be fine-tuned with the following flags:
1820
* `-XepOpt:<CheckerName>:IgnoredTypes=io.trino.spi.MyClass,...` - ignore usage of APIs from these
1921
classes
2022

23+
### Checker for issues with `Objects::requireNonNull`
24+
25+
* `RequireNonNullMessage` - Make sure that the message passed as the second argument is not
26+
malformed
27+
28+
This check focuses on cases of simple typos or mismatches of the error message with the thing being
29+
checked. It looks for calls to `Objects#requireNonNull` with a message which is a literal string
30+
matching the pattern:
31+
32+
```
33+
<identifier>[additional information]["is null"][more additional information]
34+
```
35+
36+
The "is null" part can also be one of some other variants, like "is required", "are missing",
37+
"can't be null" etc.
38+
39+
There are two places where some additional information can be placed:
40+
41+
* After the identifier:
42+
43+
```
44+
<identifier> for something is null
45+
```
46+
47+
* After the "is null" pattern:
48+
49+
```
50+
<identifier> is null: this is bad
51+
```
52+
53+
Or it can be both. Note that the "is null" part is also optional, so things like
54+
`requireNonNull(parameter, "parameter")` are also allowed.
55+
56+
If a message matching the above pattern is found, it then checks if the `<identifier>` is identical
57+
to the identifier passed as the first argument to `requireNonNull` and will trigger if they're
58+
not. It will ignore anything more complex than a plain identifier. It will also ignore invocations
59+
with no message at all (these are meant to be quick sanity checks and are harmless) and where the
60+
message is not a literal string (e.g. concatenations, calls to `String#format` or lambdas, which
61+
are all considered custom messages not to be messed with).
62+
63+
Outside constructors, though, the checks are slightly relaxed. The rationale for this is that in
64+
constructors `requireNonNull` is used primarily to sanity check the parameters and (usually) assign
65+
them to respective fields, so there's a lot of regularity which can be exploited for checking
66+
correctness. Outside of constructors, though, these calls are much more free-form and used for more
67+
purposes. Thus, the check will not report cases where the message can't be recognized as an
68+
"X is null" pattern, or if the part before that pattern is not an identifier. So, the following
69+
cases are not allowed in constructors, but are allowed otherwise:
70+
71+
* `requireNonNull(parameter, "Please provide the information we require")`:
72+
* `requireNonNull(parameter, "123 is null")`:
73+
74+
## Usage
75+
2176
Individual warnings can be suppressed with the `@SuppressWarnings("<CheckerName>")` annotation. For
2277
the `DeprecatedApi` checker, `@SuppressWarnings({"deprecation", "DeprecatedApi"})` suppresses both
2378
compiler and error-prone checker warnings. This is necessary as `DeprecatedApi` can be more
2479
restrictive than the compiler deprecation check.
2580

26-
## Usage
27-
2881
To use the checkers configure your project to build with the Error Prone Java compiler and add
2982
`error-prone-checks` annotation processor.
3083

‎src/main/java/io/starburst/errorprone/RequireNonNullMessageChecker.java

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
/*
2+
* Copyright Starburst Data, Inc. All rights reserved.
3+
*
4+
* THIS IS UNPUBLISHED PROPRIETARY SOURCE CODE OF STARBURST DATA.
5+
* The copyright notice above does not evidence any
6+
* actual or intended publication of such source code.
7+
*
8+
* Redistribution of this material is strictly prohibited.
9+
*/
10+
package io.starburst.errorprone;
11+
12+
import com.google.auto.service.AutoService;
13+
import com.google.errorprone.BugPattern;
14+
import com.google.errorprone.VisitorState;
15+
import com.google.errorprone.bugpatterns.BugChecker;
16+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
17+
import com.google.errorprone.fixes.SuggestedFix;
18+
import com.google.errorprone.matchers.Description;
19+
import com.google.errorprone.matchers.Matcher;
20+
import com.sun.source.tree.ExpressionTree;
21+
import com.sun.source.tree.LiteralTree;
22+
import com.sun.source.tree.MethodInvocationTree;
23+
import com.sun.source.tree.MethodTree;
24+
25+
import java.util.List;
26+
import java.util.Objects;
27+
import java.util.regex.Pattern;
28+
29+
import static com.google.errorprone.matchers.Description.NO_MATCH;
30+
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
31+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
32+
import static com.sun.source.tree.Tree.Kind.IDENTIFIER;
33+
import static com.sun.source.tree.Tree.Kind.STRING_LITERAL;
34+
import static java.lang.String.format;
35+
import static java.util.Objects.requireNonNull;
36+
37+
@AutoService(BugChecker.class)
38+
@BugPattern(
39+
name = "RequireNonNullMessage",
40+
summary = "An error message provided to Objects#requireNonNull is incorrect",
41+
explanation = """
42+
The error message passed as the second argument to Objects#requireNonNull \
43+
should make it immediately obvious which value is null; the check will trigger when \
44+
an identifier is passes as the first argument to Objects#requireNonNull, \
45+
but the error message passes as the second argument does not mention its name.""",
46+
linkType = BugPattern.LinkType.NONE,
47+
severity = BugPattern.SeverityLevel.WARNING)
48+
public class RequireNonNullMessageChecker
49+
extends BugChecker
50+
implements MethodInvocationTreeMatcher
51+
{
52+
private static final Pattern MESSAGE_PATTERN = Pattern.compile(" (?:(?:is|are|was|were) (?:null|empty|missing|none)|(?:is|are) required|(?:must not|cannot|can't) be (?:null|empty))");
53+
private static final Pattern IDENTIFIER_PATTERN = Pattern.compile("^\\p{javaUnicodeIdentifierStart}\\p{javaUnicodeIdentifierPart}*\\b");
54+
55+
private static final Matcher<ExpressionTree> requireNonNull = staticMethod()
56+
.onClass(Objects.class.getName())
57+
.named("requireNonNull");
58+
59+
@Override
60+
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state)
61+
{
62+
if (!requireNonNull.matches(tree, state)) {
63+
return NO_MATCH;
64+
}
65+
List<? extends ExpressionTree> arguments = tree.getArguments();
66+
if (arguments.isEmpty()) {
67+
// weird
68+
return NO_MATCH;
69+
}
70+
71+
// no error message at all:
72+
if (arguments.size() < 2) {
73+
// this is considered fine: it's probably just a sanity check
74+
// (we could enable it at some point, but let's just focus on typos for now)
75+
return NO_MATCH;
76+
}
77+
78+
// the first argument: identifier or an expression?
79+
ExpressionTree objectArgument = arguments.get(0);
80+
if (objectArgument.getKind() != IDENTIFIER) {
81+
// we don't want to deal with complex expressions; we only want to detect typos in simple cases
82+
return NO_MATCH;
83+
}
84+
String objectArgumentIdentifier = requireNonNull(state.getSourceForNode(objectArgument));
85+
86+
// inspect the message
87+
ExpressionTree messageArgument = arguments.get(1);
88+
if (messageArgument.getKind() != STRING_LITERAL) {
89+
// something else than a literal: ignore, since we can't inspect it
90+
// TODO: suggest using message supplier
91+
return NO_MATCH;
92+
}
93+
String messageLiteral = (String) ((LiteralTree) messageArgument).getValue();
94+
95+
// we will first see if we can recognize the error message as "X is null" kind of thing, then proceed accordingly
96+
// note: there's no end-of-string anchor in the pattern, so we allow extra information at the end of the message
97+
// (and it starts with a space instead of the start-of-string anchor!)
98+
java.util.regex.Matcher patternMatch = MESSAGE_PATTERN.matcher(messageLiteral);
99+
if (!patternMatch.find()) {
100+
// the message does not fit the pattern - must be some free-form text which we won't try to interpret
101+
// checks in constructors are a special case, though, so require that the message is equal to the identifier's name
102+
if (isWithinConstructor(state) && !messageLiteral.equals(objectArgumentIdentifier)) {
103+
return describeMatch(tree, (LiteralTree) messageArgument, objectArgumentIdentifier, " is null", "");
104+
}
105+
106+
return NO_MATCH;
107+
}
108+
109+
// now that we know it's "X is null" kind of thing, let's interpret the "X" part
110+
// note: the pattern ends with a word-break, so we won't get a positive match
111+
// if only part of the beginning is recognizable as an identifier
112+
java.util.regex.Matcher identifierMatch = IDENTIFIER_PATTERN.matcher(messageLiteral);
113+
if (!identifierMatch.find()) {
114+
// the thing before the pattern is not an identifier at all: ignore, but not in constructors
115+
if (isWithinConstructor(state)) {
116+
return describeMatch(tree, (LiteralTree) messageArgument, objectArgumentIdentifier, patternMatch.group(), messageLiteral.substring(patternMatch.end()));
117+
}
118+
119+
return NO_MATCH;
120+
}
121+
122+
if (!identifierMatch.group().equals(objectArgumentIdentifier)) {
123+
// we matched the identifier at the beginning of the message, and it's not equal to the identifier
124+
// passed as the first argument: report a match
125+
// note: the thing before the pattern may be an identifier and some extra stuff
126+
// (e.g. `requireNonNull(parameter, "parameter somehow is null")` is fine)
127+
return describeMatch(tree, (LiteralTree) messageArgument, objectArgumentIdentifier, patternMatch.group(), messageLiteral.substring(patternMatch.end()));
128+
}
129+
130+
// not a typo: no error
131+
return NO_MATCH;
132+
}
133+
134+
private Description describeMatch(MethodInvocationTree tree, LiteralTree messageArgument, String objectArgumentIdentifier, String pattern, String suffix)
135+
{
136+
return describeMatch(
137+
tree,
138+
SuggestedFix.replace(
139+
messageArgument,
140+
format("\"%s%s%s\"", objectArgumentIdentifier, pattern, suffix)));
141+
}
142+
143+
private boolean isWithinConstructor(VisitorState state)
144+
{
145+
MethodTree enclosingMethodTree = state.findEnclosing(MethodTree.class);
146+
return enclosingMethodTree != null && getSymbol(enclosingMethodTree).isConstructor();
147+
}
148+
}

‎src/test/java/io/starburst/errorprone/TestRequireNonNullMessageChecker.java

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/*
2+
* Copyright Starburst Data, Inc. All rights reserved.
3+
*
4+
* THIS IS UNPUBLISHED PROPRIETARY SOURCE CODE OF STARBURST DATA.
5+
* The copyright notice above does not evidence any
6+
* actual or intended publication of such source code.
7+
*
8+
* Redistribution of this material is strictly prohibited.
9+
*/
10+
package io.starburst.errorprone;
11+
12+
import com.google.errorprone.CompilationTestHelper;
13+
import org.testng.annotations.Test;
14+
15+
public class TestRequireNonNullMessageChecker
16+
{
17+
private CompilationTestHelper compilationTestHelper()
18+
{
19+
return CompilationTestHelper.newInstance(RequireNonNullMessageChecker.class, getClass());
20+
}
21+
22+
@Test
23+
public void testNegativeCases()
24+
{
25+
compilationTestHelper().addSourceFile("RequireNonNullMessageNegativeCases.java").doTest();
26+
}
27+
28+
@Test
29+
public void testPositiveCases()
30+
{
31+
compilationTestHelper().addSourceFile("RequireNonNullMessagePositiveCases.java").doTest();
32+
}
33+
}

‎src/test/java/io/starburst/errorprone/testdata/RequireNonNullMessageNegativeCases.java

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
/*
2+
* Copyright Starburst Data, Inc. All rights reserved.
3+
*
4+
* THIS IS UNPUBLISHED PROPRIETARY SOURCE CODE OF STARBURST DATA.
5+
* The copyright notice above does not evidence any
6+
* actual or intended publication of such source code.
7+
*
8+
* Redistribution of this material is strictly prohibited.
9+
*/
10+
package io.starburst.errorprone.testdata;
11+
12+
import java.util.List;
13+
import java.util.Objects;
14+
15+
import static java.util.Objects.requireNonNull;
16+
17+
public class RequireNonNullMessageNegativeCases
18+
{
19+
private Object placeholder;
20+
21+
@SuppressWarnings("RegexpSingleline") // "Objects.requireNonNull should only be used with static imports" - we do it on purpose
22+
public RequireNonNullMessageNegativeCases(Object fullPatternInConstructorWithoutStaticImport)
23+
{
24+
this.placeholder = Objects.requireNonNull(fullPatternInConstructorWithoutStaticImport, "fullPatternInConstructorWithoutStaticImport is null");
25+
}
26+
27+
public RequireNonNullMessageNegativeCases(Byte bareIdentifierInConstructor)
28+
{
29+
this.placeholder = requireNonNull(bareIdentifierInConstructor, "bareIdentifierInConstructor");
30+
}
31+
32+
public RequireNonNullMessageNegativeCases(Byte fullPatternInConstructor, int dummy)
33+
{
34+
this.placeholder = requireNonNull(fullPatternInConstructor, "fullPatternInConstructor is null");
35+
}
36+
37+
public RequireNonNullMessageNegativeCases(Byte fullPatternWithInfixInConstructor, long dummy)
38+
{
39+
this.placeholder = requireNonNull(fullPatternWithInfixInConstructor, "fullPatternWithInfixInConstructor clearly is null");
40+
}
41+
42+
public RequireNonNullMessageNegativeCases(Boolean fullPatternNestedInConstructor)
43+
{
44+
if (true == false) {
45+
for (String s : List.of("a", "b", "c")) {
46+
this.placeholder = requireNonNull(fullPatternNestedInConstructor, "fullPatternNestedInConstructor is null");
47+
}
48+
}
49+
}
50+
51+
public RequireNonNullMessageNegativeCases(Integer messageMissingInConstructor)
52+
{
53+
// this is considered fine: just a quick sanity check
54+
this.placeholder = requireNonNull(messageMissingInConstructor);
55+
}
56+
57+
public RequireNonNullMessageNegativeCases(String complexExpressionInConstructor)
58+
{
59+
// we only want to find typos and other simple cases, so ignore complex expressions
60+
this.placeholder = requireNonNull(getClass(), "I don't like this parameter");
61+
}
62+
63+
@SuppressWarnings("RegexpSingleline") // "Objects.requireNonNull should only be used with static imports" - we do it on purpose
64+
public void fullPatternInMethodWithoutStaticImport(String parameter)
65+
{
66+
this.placeholder = Objects.requireNonNull(parameter, "parameter is null");
67+
}
68+
69+
public void bareIdentifierInMethod(Object parameter)
70+
{
71+
this.placeholder = requireNonNull(parameter, "parameter");
72+
}
73+
74+
public void fullPatternInMethod(Object parameter)
75+
{
76+
this.placeholder = requireNonNull(parameter, "parameter is null");
77+
}
78+
79+
public void fullPatternWithInfixInMethod(Object parameter)
80+
{
81+
this.placeholder = requireNonNull(parameter, "parameter clearly is null");
82+
}
83+
84+
public void fullPatternNestedInMethod(Object parameter)
85+
{
86+
if (true == false) {
87+
for (String s : List.of("a", "b", "c")) {
88+
this.placeholder = requireNonNull(parameter, "parameter is null");
89+
}
90+
}
91+
}
92+
93+
public void messageMissingInMethod(String parameter)
94+
{
95+
// this is considered fine: just check the stack trace
96+
this.placeholder = requireNonNull(parameter);
97+
}
98+
99+
public void messageNotFittingPatternInMethod(String parameter)
100+
{
101+
// someone wanted to provide a more descriptive message, and this is fine
102+
this.placeholder = requireNonNull(parameter, "I don't like this parameter");
103+
}
104+
105+
public void complexExpressionInMethod(String parameter)
106+
{
107+
// we only want to find typos and other simple cases, so ignore complex expressions
108+
this.placeholder = requireNonNull(parameter.getClass(), "I don't like this parameter");
109+
}
110+
}

0 commit comments

Comments
 (0)