Skip to content

Commit

Permalink
Added single capital letter followed by numeral fix to TypeParameterN…
Browse files Browse the repository at this point in the history 
…aming based on the Google Java Style Guide: https://google.github.io/styleguide/javaguide.html#s5.2.8-type-variable-names

RELNOTES: n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=205685357
  • Loading branch information
@lius2020 @ronshapiro
lius2020 authored and ronshapiro committed Aug 2, 2018
1 parent 5016ea2 commit 31c1219
Show file tree
Hide file tree
Showing 3 changed files with 259 additions and 8 deletions.
112 changes: 108 additions & 4 deletions core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterNaming.java
View file
Expand Up @@ -13,8 +13,10 @@
* See the License for the specific language governing permissions and * See the License for the specific language governing permissions and
* limitations under the License. * limitations under the License.
*/ */

package com.google.errorprone.bugpatterns; package com.google.errorprone.bugpatterns;


import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.errorprone.BugPattern.Category.JDK; import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;


Expand All @@ -32,13 +34,25 @@
import com.google.errorprone.bugpatterns.BugChecker.TypeParameterTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.TypeParameterTreeMatcher;
import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Description;
import com.google.errorprone.names.NamingConventions; import com.google.errorprone.names.NamingConventions;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.Tree;
import com.sun.source.tree.TypeParameterTree; import com.sun.source.tree.TypeParameterTree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.TypeVariableSymbol;
import java.util.ArrayList;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern; import java.util.regex.Pattern;
import java.util.stream.Collectors; import java.util.stream.Collectors;
import java.util.stream.Stream; import java.util.stream.Stream;
import javax.lang.model.element.Name; import javax.lang.model.element.Name;


/** Enforces type parameters match the google style guide. */ /**
* Enforces type parameters match the google style guide.
*
* @author siyuanl@google.com (Siyuan Liu)
* @author glorioso@google.com (Nick Glorioso)
*/
@BugPattern( @BugPattern(
name = "TypeParameterNaming", name = "TypeParameterNaming",
summary = summary =
Expand All @@ -52,15 +66,16 @@
link = "https://google.github.io/styleguide/javaguide.html#s5.2.8-type-variable-names" link = "https://google.github.io/styleguide/javaguide.html#s5.2.8-type-variable-names"
) )
public class TypeParameterNaming extends BugChecker implements TypeParameterTreeMatcher { public class TypeParameterNaming extends BugChecker implements TypeParameterTreeMatcher {

private static final Pattern SINGLE_PLUS_MAYBE_DIGIT = Pattern.compile("[A-Z]\\d?"); private static final Pattern SINGLE_PLUS_MAYBE_DIGIT = Pattern.compile("[A-Z]\\d?");
private static final Pattern TRAILING_DIGIT_EXTRACTOR = Pattern.compile("^(.*?)(\\d+)$");


private static String upperCamelToken(String s) { private static String upperCamelToken(String s) {
return "" + Ascii.toUpperCase(s.charAt(0)) + (s.length() == 1 ? "" : s.substring(1)); return "" + Ascii.toUpperCase(s.charAt(0)) + (s.length() == 1 ? "" : s.substring(1));
} }


@Override @Override
public Description matchTypeParameter(TypeParameterTree tree, VisitorState state) { public Description matchTypeParameter(TypeParameterTree tree, VisitorState state) {

if (matchesTypeParameterNamingScheme(tree.getName())) { if (matchesTypeParameterNamingScheme(tree.getName())) {
return Description.NO_MATCH; return Description.NO_MATCH;
} }
Expand All @@ -75,16 +90,105 @@ public Description matchTypeParameter(TypeParameterTree tree, VisitorState state
TypeParameterShadowing.renameTypeVariable( TypeParameterShadowing.renameTypeVariable(
tree, tree,
state.getPath().getParentPath().getLeaf(), state.getPath().getParentPath().getLeaf(),
replacementName(tree.getName().toString()), suggestedNameFollowedWithT(tree.getName().toString()),
state))
.addFix(
TypeParameterShadowing.renameTypeVariable(
tree,
state.getPath().getParentPath().getLeaf(),
suggestedSingleLetter(tree.getName().toString(), tree),
state)) state))
.build(); .build();
} }


// Get list of type params of every enclosing class
private static List<TypeVariableSymbol> typeVariablesEnclosing(Symbol sym) {
List<TypeVariableSymbol> typeVarScopes = new ArrayList<>();
outer:
while (!sym.isStatic()) {
sym = sym.owner;
switch (sym.getKind()) {
case PACKAGE:
break outer;
case METHOD:
case CLASS:
typeVarScopes.addAll(0, sym.getTypeParameters());
break;
default: // fall out
}
}
return typeVarScopes;
}

private static String suggestedSingleLetter(String id, Tree tree) {
char firstLetter = id.charAt(0);
Symbol sym = ASTHelpers.getSymbol(tree);
List<TypeVariableSymbol> enclosingTypeSymbols = typeVariablesEnclosing(sym);

for (TypeVariableSymbol typeName : enclosingTypeSymbols) {
char enclosingTypeFirstLetter = typeName.toString().charAt(0);
if (enclosingTypeFirstLetter == firstLetter
&& !matchesTypeParameterNamingScheme(typeName.name)) {
List<String> typeVarsInScope =
Streams.concat(enclosingTypeSymbols.stream(), sym.getTypeParameters().stream())
.map(v -> v.name.toString())
.collect(toImmutableList());

return firstLetterReplacementName(id, typeVarsInScope);
}
}

return Character.toString(firstLetter);
}
// T -> T2
// T2 -> T3
// T -> T4 (if T2 and T3 already exist)
// TODO(siyuanl) : combine this method with TypeParameterShadowing.replacementTypeVarName
private static String firstLetterReplacementName(String name, List<String> superTypeVars) {
String firstLetterOfBase = Character.toString(name.charAt(0));
int typeVarNum = 2;
boolean first = true;

Matcher matcher = TRAILING_DIGIT_EXTRACTOR.matcher(name);
if (matcher.matches()) {
name = matcher.group(1);
typeVarNum = Integer.parseInt(matcher.group(2)) + 1;
}

String replacementName = "";

// Look at the type names to the left of the current type
// Since this bugchecker doesn't rename as it goes, we have to check which type names
// would've been renamed before the current ones
for (String superTypeVar : superTypeVars) {
if (superTypeVar.equals(name)) {
if (typeVarNum == 2 && first) {
return firstLetterOfBase;
}
break;
} else if (superTypeVar.charAt(0) == name.charAt(0)) {
if (!first) {
typeVarNum++;
} else {
first = false;
}
replacementName = firstLetterOfBase + typeVarNum;
}
}

while (superTypeVars.contains(replacementName)) {
typeVarNum++;
replacementName = firstLetterOfBase + typeVarNum;
}

return replacementName;
}

static boolean matchesTypeParameterNamingScheme(Name name) { static boolean matchesTypeParameterNamingScheme(Name name) {
return SINGLE_PLUS_MAYBE_DIGIT.matcher(name).matches() || matchesClassWithT(name.toString()); return SINGLE_PLUS_MAYBE_DIGIT.matcher(name).matches() || matchesClassWithT(name.toString());
} }


private static String replacementName(String identifier) { private static String suggestedNameFollowedWithT(String identifier) {
Preconditions.checkArgument(!identifier.isEmpty()); Preconditions.checkArgument(!identifier.isEmpty());


// Some early checks: // Some early checks:
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/TypeParameterShadowing.java
View file
Expand Up @@ -81,11 +81,13 @@ private Description findDuplicatesOf(
if (symbol == null) { if (symbol == null) {
return Description.NO_MATCH; return Description.NO_MATCH;
} }

List<TypeVariableSymbol> enclosingTypeSymbols = typeVariablesEnclosing(symbol); List<TypeVariableSymbol> enclosingTypeSymbols = typeVariablesEnclosing(symbol);
if (enclosingTypeSymbols.isEmpty()) { if (enclosingTypeSymbols.isEmpty()) {
return Description.NO_MATCH; return Description.NO_MATCH;
} }


// See if the passed in list typeParameters exist in the enclosingTypeSymbols
List<TypeVariableSymbol> conflictingTypeSymbols = new ArrayList<>(); List<TypeVariableSymbol> conflictingTypeSymbols = new ArrayList<>();
typeParameters.forEach( typeParameters.forEach(
param -> param ->
Expand All @@ -94,11 +96,11 @@ private Description findDuplicatesOf(
.filter(tvs -> tvs.name.contentEquals(param.getName())) .filter(tvs -> tvs.name.contentEquals(param.getName()))
.findFirst() .findFirst()
.ifPresent(conflictingTypeSymbols::add)); .ifPresent(conflictingTypeSymbols::add));

if (conflictingTypeSymbols.isEmpty()) { if (conflictingTypeSymbols.isEmpty()) {
return Description.NO_MATCH; return Description.NO_MATCH;
} }


// Describes what's the conflicting type and where it is
Description.Builder descriptionBuilder = buildDescription(tree); Description.Builder descriptionBuilder = buildDescription(tree);
String message = String message =
"Found aliased type parameters: " "Found aliased type parameters: "
Expand All @@ -109,6 +111,7 @@ private Description findDuplicatesOf(


descriptionBuilder.setMessage(message); descriptionBuilder.setMessage(message);


// Map conflictingTypeSymbol to its new name
Set<String> typeVarsInScope = Set<String> typeVarsInScope =
Streams.concat(enclosingTypeSymbols.stream(), symbol.getTypeParameters().stream()) Streams.concat(enclosingTypeSymbols.stream(), symbol.getTypeParameters().stream())
.map(v -> v.name.toString()) .map(v -> v.name.toString())
Expand Down Expand Up @@ -203,6 +206,7 @@ public void visitIdent(JCTree.JCIdent tree) {
return fixBuilder.build(); return fixBuilder.build();
} }


// Get list of type params of every enclosing class
private static List<TypeVariableSymbol> typeVariablesEnclosing(Symbol sym) { private static List<TypeVariableSymbol> typeVariablesEnclosing(Symbol sym) {
List<TypeVariableSymbol> typeVarScopes = new ArrayList<>(); List<TypeVariableSymbol> typeVarScopes = new ArrayList<>();
outer: outer:
Expand Down
149 changes: 146 additions & 3 deletions core/src/test/java/com/google/errorprone/bugpatterns/TypeParameterNamingTest.java
View file
Expand Up @@ -19,6 +19,7 @@


import com.google.common.truth.BooleanSubject; import com.google.common.truth.BooleanSubject;
import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers;
import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.CompilationTestHelper;
import org.junit.Before; import org.junit.Before;
import org.junit.Test; import org.junit.Test;
Expand All @@ -44,16 +45,16 @@ public void positiveCases() {
compilationHelper compilationHelper
.addSourceLines( .addSourceLines(
"Test.java", "Test.java",
"// BUG: Diagnostic contains: ", "// BUG: Diagnostic contains: TypeParameterNaming",
"class Test<BadName> {", "class Test<BadName> {",
" // BUG: Diagnostic contains: Foo", " // BUG: Diagnostic contains: TypeParameterNaming",
" public <T, Foo> void method(Exception e) {}", " public <T, Foo> void method(Exception e) {}",
"}") "}")
.doTest(); .doTest();
} }


@Test @Test
public void refactoring() throws Exception { public void refactoring_trailing() throws Exception {
refactoring refactoring
.addInputLines( .addInputLines(
"in/Test.java", "in/Test.java",
Expand All @@ -71,6 +72,147 @@ public void refactoring() throws Exception {
" FooT d = f;", " FooT d = f;",
" }", " }",
"}") "}")
.setFixChooser(FixChoosers.FIRST)
.doTest();
}

@Test
public void refactoring_single() throws Exception {
refactoring
.addInputLines(
"in/Test.java",
"class Test<BadName> {",
" public <T, Foo> void method(Foo f) {",
" BadName bad = null;",
" Foo d = f;",
" }",
"}")
.addOutputLines(
"out/Test.java",
"class Test<B> {",
" public <T, F> void method(F f) {",
" B bad = null;",
" F d = f;",
" }",
"}")
.setFixChooser(FixChoosers.SECOND)
.doTest();
}

@Test
public void refactoring_single_number() throws Exception {
refactoring
.addInputLines(
"in/Test.java",
"class Test<Bar> {",
" public <T, Baz> void method(Baz f) {",
" Bar bad = null;",
" Baz d = f;",
" }",
"}")
.addOutputLines(
"out/Test.java",
"class Test<B> {",
" public <T, B2> void method(B2 f) {",
" B bad = null;",
" B2 d = f;",
" }",
"}")
.setFixChooser(FixChoosers.SECOND)
.doTest();
}

@Test
public void refactoring_single_number_enclosing() throws Exception {
refactoring
.addInputLines(
"in/Test.java",
"class Test<Bar> {",
" public <T, Baz, Boo> void method(Baz f) {",
" Bar bad = null;",
" Baz d = f;",
" Boo wow = null;",
" }",
"}")
.addOutputLines(
"out/Test.java",
"class Test<B> {",
" public <T, B2, B3> void method(B2 f) {",
" B bad = null;",
" B2 d = f;",
" B3 wow = null;",
" }",
"}")
.setFixChooser(FixChoosers.SECOND)
.doTest();
}

@Test
public void refactoring_single_number_within_scope() throws Exception {
refactoring
.addInputLines(
"in/Test.java",
"class Test {",
" public <T, Baz, Boo> void method(Baz f) {",
" Baz d = f;",
" Boo wow = null;",
" }",
"}")
.addOutputLines(
"out/Test.java",
"class Test {",
" public <T, B, B2> void method(B f) {",
" B d = f;",
" B2 wow = null;",
" }",
"}")
.setFixChooser(FixChoosers.SECOND)
.doTest();
}

@Test
public void refactoring_single_number_many_ok() throws Exception {
refactoring
.addInputLines(
"in/Test.java",
"class Test {",
" public <B, B2, B3, B4, Bad> void method(Bad f) {",
" Bad d = f;",
" B2 wow = null;",
" }",
"}")
.addOutputLines(
"out/Test.java",
"class Test {",
" public <B, B2, B3, B4, B5> void method(B5 f) {",
" B5 d = f;",
" B2 wow = null;",
" }",
"}")
.setFixChooser(FixChoosers.SECOND)
.doTest();
}

@Test
public void refactoring_single_number_ok_after() throws Exception {
refactoring
.addInputLines(
"in/Test.java",
"class Test {",
" public <B, Bad, B2> void method(Bad f) {",
" Bad d = f;",
" B2 wow = null;",
" }",
"}")
.addOutputLines(
"out/Test.java",
"class Test {",
" public <B, B3, B2> void method(B3 f) {",
" B3 d = f;",
" B2 wow = null;",
" }",
"}")
.setFixChooser(FixChoosers.SECOND)
.doTest(); .doTest();
} }


Expand All @@ -93,6 +235,7 @@ public void refactoring_newNames() throws Exception {
" FooT d = f;", " FooT d = f;",
" }", " }",
"}") "}")
.setFixChooser(FixChoosers.FIRST)
.doTest(); .doTest();
} }


Expand Down

0 comments on commit 31c1219

Please sign in to comment.