Skip to content

Commit

Permalink
Fix a bug where an @Inject method has a parameter name that is valid …
Browse files Browse the repository at this point in the history
…on the JVM but invalid for the Java language.

This can happen with the following code in Kotlin:

class Test {
    @set:Inject
    internal var foo: String? = null
}

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=251647410
  • Loading branch information
ronshapiro committed Jun 6, 2019
1 parent c4775c1 commit a11605c
Showing 1 changed file with 28 additions and 4 deletions.
32 changes: 28 additions & 4 deletions java/dagger/internal/codegen/InjectionMethod.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.google.common.base.Preconditions.checkArgument;
import static com.squareup.javapoet.MethodSpec.methodBuilder;
import static dagger.internal.codegen.SourceFiles.protectAgainstKeywords;
import static dagger.internal.codegen.javapoet.CodeBlocks.makeParametersCodeBlock;
import static dagger.internal.codegen.langmodel.Accessibility.isRawTypePubliclyAccessible;
import static javax.lang.model.element.Modifier.PUBLIC;
Expand All @@ -38,6 +39,7 @@
import dagger.internal.codegen.langmodel.DaggerElements;
import java.util.List;
import java.util.Optional;
import javax.lang.model.SourceVersion;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.Parameterizable;
import javax.lang.model.element.VariableElement;
Expand Down Expand Up @@ -86,8 +88,7 @@ MethodSpec toMethodSpec() {
.addParameters(parameters().keySet())
.addCode(methodBody());
returnType().map(TypeName::get).ifPresent(builder::returns);
nullableAnnotation()
.ifPresent(nullableType -> CodeBlocks.addAnnotation(builder, nullableType));
nullableAnnotation().ifPresent(nullableType -> CodeBlocks.addAnnotation(builder, nullableType));
exceptions().stream().map(TypeName::get).forEach(builder::addException);
return builder.build();
}
Expand Down Expand Up @@ -126,12 +127,19 @@ abstract static class Builder {
private DaggerElements elements;

abstract ImmutableMap.Builder<ParameterSpec, TypeMirror> parametersBuilder();

abstract ImmutableList.Builder<TypeVariableName> typeVariablesBuilder();

abstract Builder name(String name);

abstract Builder varargs(boolean varargs);

abstract Builder returnType(TypeMirror returnType);

abstract Builder exceptions(Iterable<? extends TypeMirror> exceptions);

abstract Builder nullableAnnotation(Optional<DeclaredType> nullableAnnotation);

abstract Builder methodBody(CodeBlock methodBody);

final CodeBlock.Builder methodBodyBuilder() {
Expand Down Expand Up @@ -173,13 +181,29 @@ CodeBlock copyParameters(ExecutableElement method) {
CodeBlock copyParameter(VariableElement parameter) {
TypeMirror elementType = parameter.asType();
boolean useObject = !isRawTypePubliclyAccessible(elementType);
TypeMirror publicType = useObject ? objectType() : elementType;
ParameterSpec parameterSpec = addParameter(parameter.getSimpleName().toString(), publicType);
TypeMirror publicType = useObject ? objectType() : elementType;
ParameterSpec parameterSpec =
addParameter(validJavaName(parameter.getSimpleName().toString()), publicType);
return useObject
? CodeBlock.of("($T) $N", elementType, parameterSpec)
: CodeBlock.of("$N", parameterSpec);
}

private static String validJavaName(String name) {

This comment has been minimized.

Copy link
@tbroyer

tbroyer Aug 21, 2019

How about using JavaPoet's NameAllocator.toJavaIdentifier() instead of rolling your own? It does basically the same as you're doing here, except using code points rather than characters (and it does not protectAgainstKeywords; NameAllocator.newName will do that separately, simply appending a _ to the result of toJavaIdentifier: https://github.com/square/javapoet/blob/e9460b84fc41464c2aa2ef85c84dd1ac87ae1692/src/main/java/com/squareup/javapoet/NameAllocator.java#L112-L116)

if (SourceVersion.isIdentifier(name)) {
return protectAgainstKeywords(name);
}

StringBuilder newName = new StringBuilder(name.length());
char firstChar = name.charAt(0);
if (!Character.isJavaIdentifierStart(firstChar)) {
newName.append('_');
}

name.chars().forEach(c -> newName.append(Character.isJavaIdentifierPart(c) ? c : '_'));
return newName.toString();
}

private TypeMirror objectType() {
return elements.getTypeElement(Object.class).asType();
}
Expand Down

0 comments on commit a11605c

Please sign in to comment.