Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add parsing logic for translating from standard API strings (what we …
…use in the CRV stats,[] This will be necessary for marking JDK/3p code (as either `@CheckReturnValue` or `@CanIgnoreReturnValue`). #checkreturnvalue PiperOrigin-RevId: 442548486
- Loading branch information
Showing
2 changed files
with
337 additions
and
0 deletions.
There are no files selected for viewing
211 changes: 211 additions & 0 deletions
211
core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/Api.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,211 @@ | ||
/* | ||
* Copyright 2022 The Error Prone Authors. | ||
* | ||
* 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.google.errorprone.bugpatterns.checkreturnvalue; | ||
|
||
import static com.google.common.base.CharMatcher.whitespace; | ||
import static com.google.common.base.Preconditions.checkArgument; | ||
import static com.google.common.collect.ImmutableList.toImmutableList; | ||
import static com.google.common.collect.ImmutableSet.toImmutableSet; | ||
import static com.google.errorprone.matchers.Matchers.anyMethod; | ||
import static com.google.errorprone.matchers.Matchers.anyOf; | ||
import static com.google.errorprone.matchers.Matchers.constructor; | ||
import static java.lang.Character.isJavaIdentifierPart; | ||
import static java.lang.Character.isJavaIdentifierStart; | ||
|
||
import com.google.auto.value.AutoValue; | ||
import com.google.common.base.Joiner; | ||
import com.google.common.base.Splitter; | ||
import com.google.common.collect.ImmutableList; | ||
import com.google.common.collect.ImmutableSet; | ||
import com.google.errorprone.matchers.Matcher; | ||
import com.sun.source.tree.ExpressionTree; | ||
import java.util.List; | ||
|
||
/** | ||
* Represents a Java method or constructor. Provides a method to parse an API from a string format, | ||
* and another method to create an ErrorProne {@link Matcher} for the API. | ||
*/ | ||
// TODO(kak): do we want to be able to represent classes in addition to methods/constructors? | ||
// TODO(kak): if not, then consider renaming to `MethodSignature` or something | ||
@AutoValue | ||
public abstract class Api { | ||
|
||
// TODO(b/223668437): use this (or something other than the Matcher<> API) | ||
static Matcher<ExpressionTree> createMatcherFromApis(List<String> apis) { | ||
return anyOf(apis.stream().map(Api::parse).map(Api::matcher).collect(toImmutableList())); | ||
} | ||
|
||
static ImmutableSet<Api> createSetFromApis(List<String> apis) { | ||
return apis.stream().map(Api::parse).collect(toImmutableSet()); | ||
} | ||
|
||
/** Returns the fully qualified type that contains the given method/constructor. */ | ||
abstract String className(); | ||
|
||
/** | ||
* Returns the simple name of the method. If the API is a constructor (i.e., {@code | ||
* isConstructor() == true}), then {@code "<init>"} is returned. | ||
*/ | ||
abstract String methodName(); | ||
|
||
/** Returns the list of fully qualified parameter types for the given method/constructor. */ | ||
abstract ImmutableList<String> parameterTypes(); | ||
|
||
@Override | ||
public final String toString() { | ||
return String.format( | ||
"%s#%s(%s)", className(), methodName(), Joiner.on(',').join(parameterTypes())); | ||
} | ||
|
||
/** Returns whether this API represents a constructor or not. */ | ||
boolean isConstructor() { | ||
return methodName().equals("<init>"); | ||
} | ||
|
||
private Matcher<ExpressionTree> matcher() { | ||
return isConstructor() | ||
? constructor().forClass(className()).withParameters(parameterTypes()) | ||
: anyMethod() | ||
.onClass(className()) | ||
.named(methodName()) | ||
// TODO(b/219754967): what about arrays | ||
.withParameters(parameterTypes()); | ||
} | ||
|
||
private static final Splitter PARAM_SPLITTER = Splitter.on(','); | ||
|
||
/** | ||
* Parses an API string into an {@link Api}. Example API strings are: | ||
* | ||
* <ul> | ||
* <li>a constructor (e.g., {@code java.net.URI#<init>(java.lang.String)}) | ||
* <li>a static method (e.g., {@code java.net.URI#create(java.lang.String)}) | ||
* <li>an instance method (e.g., {@code java.util.List#get(int)}) | ||
* <li>an instance method with types erased (e.g., {@code java.util.List#add(java.lang.Object)}) | ||
* </ul> | ||
*/ | ||
static Api parse(String apiWithWhitespace) { | ||
// TODO(kak): consider removing whitespace from the String as we step through the String | ||
String api = whitespace().removeFrom(apiWithWhitespace); | ||
|
||
boolean isConstructor = false; | ||
int hashIndex = -1; | ||
int openParenIndex = -1; | ||
int closeParenIndex = -1; | ||
int lessThanIndex = -1; | ||
int greaterThanIndex = -1; | ||
for (int i = 0; i < api.length(); i++) { | ||
char ch = api.charAt(i); | ||
switch (ch) { | ||
case '#': | ||
check(hashIndex == -1, api, "it contains more than one '#'"); | ||
hashIndex = i; | ||
break; | ||
case '(': | ||
check(openParenIndex == -1, api, "it contains more than one '('"); | ||
openParenIndex = i; | ||
break; | ||
case ')': | ||
check(closeParenIndex == -1, api, "it contains more than one ')'"); | ||
closeParenIndex = i; | ||
break; | ||
case '<': | ||
check(lessThanIndex == -1, api, "it contains more than one '<'"); | ||
lessThanIndex = i; | ||
isConstructor = true; | ||
break; | ||
case '>': | ||
check(greaterThanIndex == -1, api, "it contains more than one '>'"); | ||
greaterThanIndex = i; | ||
isConstructor = true; | ||
break; | ||
case ',': // for separating parameters | ||
case '.': // for package names and fully qualified parameter names | ||
break; | ||
default: | ||
check(isJavaIdentifierPart(ch), api, "'" + ch + "' is not a valid identifier"); | ||
} | ||
} | ||
|
||
// make sure we've seen a hash, open paren, and close paren | ||
check(hashIndex != -1, api, "it must contain a '#'"); | ||
check(openParenIndex != -1, api, "it must contain a '('"); | ||
check(closeParenIndex == api.length() - 1, api, "it must end with ')'"); | ||
|
||
// make sure they came in the correct order: <sometext>#<sometext>(<sometext>) | ||
check(hashIndex < openParenIndex, api, "'#' must come before '('"); | ||
check(openParenIndex < closeParenIndex, api, "'(' must come before ')'"); | ||
|
||
if (isConstructor) { | ||
// make sure that if we've seen a < or >, we also have seen the matching one | ||
check(lessThanIndex != -1, api, "must contain both '<' and '>'"); | ||
check(greaterThanIndex != -1, api, "must contain both '<' and '>'"); | ||
|
||
// make sure the < comes directly after the # | ||
check(lessThanIndex == hashIndex + 1, api, "'<' must come directly after '#'"); | ||
|
||
// make sure that the < comes before the > | ||
check(lessThanIndex < greaterThanIndex, api, "'<' must come before '>'"); | ||
|
||
// make sure that the > comes directly before the ( | ||
check(greaterThanIndex == openParenIndex - 1, api, "'>' must come directly before '('"); | ||
|
||
// make sure the only thing between the < and > is exactly "init" | ||
String constructorName = api.substring(lessThanIndex + 1, greaterThanIndex); | ||
check(constructorName.equals("init"), api, "invalid method name: " + constructorName); | ||
} | ||
|
||
String className = api.substring(0, hashIndex); | ||
String methodName = api.substring(hashIndex + 1, openParenIndex); | ||
String parameters = api.substring(openParenIndex + 1, closeParenIndex); | ||
|
||
ImmutableList<String> paramList = | ||
parameters.isEmpty() | ||
? ImmutableList.of() | ||
: PARAM_SPLITTER.splitToStream(parameters).collect(toImmutableList()); | ||
|
||
// make sure the class name, method name, and parameter names are not empty | ||
check(!className.isEmpty(), api, "the class name cannot be empty"); | ||
check(!methodName.isEmpty(), api, "the method name cannot be empty"); | ||
for (String parameter : paramList) { | ||
check(!parameter.isEmpty(), api, "parameters cannot be empty"); | ||
|
||
check( | ||
isJavaIdentifierStart(parameter.charAt(0)), | ||
api, | ||
"parameters must start with a valid character"); | ||
} | ||
// make sure the class name starts with a valid Java identifier character | ||
check( | ||
isJavaIdentifierStart(className.charAt(0)), | ||
api, | ||
"the class name must start with a valid character"); | ||
|
||
if (!isConstructor) { | ||
// make sure the method name starts with a valid Java identifier character | ||
check( | ||
isJavaIdentifierStart(methodName.charAt(0)), | ||
api, | ||
"the method name must start with a valid character"); | ||
} | ||
|
||
return new AutoValue_Api(className, methodName, paramList); | ||
} | ||
|
||
private static void check(boolean condition, String api, String reason) { | ||
checkArgument(condition, "Unable to parse '%s' because %s", api, reason); | ||
} | ||
} |
126 changes: 126 additions & 0 deletions
126
core/src/test/java/com/google/errorprone/bugpatterns/checkreturnvalue/ApiTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
/* | ||
* Copyright 2022 The Error Prone Authors. | ||
* | ||
* 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.google.errorprone.bugpatterns.checkreturnvalue; | ||
|
||
import static com.google.common.truth.Truth.assertThat; | ||
import static org.junit.Assert.assertThrows; | ||
|
||
import com.google.common.collect.ImmutableSet; | ||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.junit.runners.JUnit4; | ||
|
||
/** Tests for {@link Api}. */ | ||
@RunWith(JUnit4.class) | ||
public final class ApiTest { | ||
|
||
private static final ImmutableSet<String> UNPARSEABLE_APIS = | ||
ImmutableSet.of( | ||
"", | ||
"#()", | ||
"#(java.lang.String)", | ||
"#foo()", | ||
"#foo(java.lang.String)", | ||
"java.lang.String", | ||
"java.lang.String##foo()", | ||
"java.lang.String#fo#o()", | ||
"java.lang.String#()", | ||
"java.lang.String#foo)()", | ||
"java.lang.String#foo)(", | ||
"java.lang.String#<init>(,)", | ||
"java.lang.String#<>()", | ||
"java.lang.String#hi<>()", | ||
"java.lang.String#<>hi()", | ||
"java.lang.String#hi<init>()", | ||
"java.lang.String#<init>hi()", | ||
"java.lang.String#<iniT>()", | ||
"java.lang.String#<init>((java.lang.String)", | ||
"java.lang.String#<init>(java.lang.String", | ||
"java.lang.String#<init>(java.lang.String,)", | ||
"java.lang.String#<init>(,java.lang.String)", | ||
"java.lang.String#<init>(java.lang.String))"); | ||
|
||
@Test | ||
public void parseApi_badInputs() { | ||
// TODO(b/223670489): would be nice to use expectThrows() here | ||
for (String badApi : UNPARSEABLE_APIS) { | ||
assertThrows( | ||
"Api.parse(\"" + badApi + "\")", IllegalArgumentException.class, () -> Api.parse(badApi)); | ||
} | ||
} | ||
|
||
@Test | ||
public void parseApi_constructorWithoutParams() { | ||
String string = "com.google.async.promisegraph.testing.TestPromiseGraphModule#<init>()"; | ||
Api api = Api.parse(string); | ||
assertThat(api.className()) | ||
.isEqualTo("com.google.async.promisegraph.testing.TestPromiseGraphModule"); | ||
assertThat(api.methodName()).isEqualTo("<init>"); | ||
assertThat(api.parameterTypes()).isEmpty(); | ||
assertThat(api.isConstructor()).isTrue(); | ||
assertThat(api.toString()).isEqualTo(string); | ||
} | ||
|
||
@Test | ||
public void parseApi_constructorWithParams() { | ||
String string = "com.google.api.client.http.GenericUrl#<init>(java.lang.String)"; | ||
Api api = Api.parse(string); | ||
assertThat(api.className()).isEqualTo("com.google.api.client.http.GenericUrl"); | ||
assertThat(api.methodName()).isEqualTo("<init>"); | ||
assertThat(api.parameterTypes()).containsExactly("java.lang.String").inOrder(); | ||
assertThat(api.isConstructor()).isTrue(); | ||
assertThat(api.toString()).isEqualTo(string); | ||
} | ||
|
||
@Test | ||
public void parseApi_methodWithoutParams() { | ||
String string = "com.google.api.services.drive.model.File#getId()"; | ||
Api api = Api.parse(string); | ||
assertThat(api.className()).isEqualTo("com.google.api.services.drive.model.File"); | ||
assertThat(api.methodName()).isEqualTo("getId"); | ||
assertThat(api.parameterTypes()).isEmpty(); | ||
assertThat(api.isConstructor()).isFalse(); | ||
assertThat(api.toString()).isEqualTo(string); | ||
} | ||
|
||
@Test | ||
public void parseApi_methodWithParamsAndSpaces() { | ||
String string = | ||
"com.google.android.libraries.stitch.binder.Binder" | ||
+ "#get(android.content.Context,java.lang.Class)"; | ||
Api api = Api.parse(string); | ||
assertThat(api.methodName()).isEqualTo("get"); | ||
assertThat(api.parameterTypes()) | ||
.containsExactly("android.content.Context", "java.lang.Class") | ||
.inOrder(); | ||
assertThat(api.isConstructor()).isFalse(); | ||
assertThat(api.toString()).isEqualTo(string); | ||
} | ||
|
||
@Test | ||
public void parseApi_methodWithArray_b219754967() { | ||
IllegalArgumentException thrown = | ||
assertThrows( | ||
"b/219754967 - cannot parse array signatures", | ||
IllegalArgumentException.class, | ||
() -> | ||
Api.parse( | ||
"com.google.inject.util.Modules.OverriddenModuleBuilder" | ||
+ "#with(com.google.inject.Module[])")); | ||
assertThat(thrown).hasMessageThat().contains("'[' is not a valid identifier"); | ||
} | ||
} |