Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Forbid modules who extend from non-Object class types. #196

Merged
merged 1 commit into from

3 participants

@JakeWharton
Owner

Closes #188.

...n/java/dagger/internal/codegen/ProvidesProcessor.java
((12 lines not shown))
- if (result.containsKey(moduleType)) continue;
- result.put(moduleType, new ArrayList<ExecutableElement>());
+ for (Element module : env.getElementsAnnotatedWith(Module.class)) {
+ if (module.getKind().equals(ElementKind.CLASS)) {
+ TypeElement moduleType = (TypeElement) module;
+
+ // Verify that all modules do not extend from non-Object types.
+ if (moduleType.getSuperclass().equals(objectType)) {
+ error("Modules may not extend from other classes: " + moduleType, moduleType);
+ }
+
+ String moduleName = moduleType.getQualifiedName().toString();
+ if (result.containsKey(moduleName)) continue;
+ result.put(moduleName, new ArrayList<ExecutableElement>());
+ } else {
+ throw new AssertionError();
@JakeWharton Owner

Was unsure about this. Enforced by @Target on the annotation, right?

@JakeWharton Owner

Should I flip the logic?

if (!module.getKind().equals(ElementKind.CLASS)) {
  throw new AssertionError();
}

TypeElement moduleType = ...
@JakeWharton Owner

Ah, I guess it could be on an interface or enum. I'll mark as an error as well.

@swankjesse Owner

Yup. But flipping the logic is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tbroyer tbroyer commented on the diff
core/src/test/java/dagger/ModuleIncludesTest.java
@@ -181,4 +181,16 @@
public void childModuleMissingModuleAnnotation() {
ObjectGraph.create(new ChildModuleMissingModuleAnnotation());
}
+
+ @Module
+ static class ThreadModule extends Thread {}
+
+ @Test public void moduleExtendingClassThrowsException() {
@tbroyer Collaborator
tbroyer added a note

Should the class be renamed as ModuleTest as this is no longer only testing for @Module(includes=…)?

@JakeWharton Owner

Yep. Good call. Updated.

@swankjesse Owner

Push your changes?

@JakeWharton Owner

Heh, I'm trying! Stupid bluetooth tether over cell network...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@swankjesse
Owner

LGTM

@JakeWharton JakeWharton merged commit 079a3dc into master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 27, 2013
  1. @JakeWharton
This page is out of date. Refresh to see the latest.
View
1  compiler/src/it/module-type-validation/invoker.properties
@@ -0,0 +1 @@
+invoker.buildResult=failure
View
53 compiler/src/it/module-type-validation/pom.xml
@@ -0,0 +1,53 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Copyright (C) 2013 Square, Inc.
+
+ 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.
+-->
+<project
+ xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <groupId>com.example.dagger.tests</groupId>
+ <artifactId>module-type-validation</artifactId>
+ <version>HEAD-SNAPSHOT</version>
+ <dependencies>
+ <dependency>
+ <groupId>@dagger.groupId@</groupId>
+ <artifactId>dagger</artifactId>
+ <version>@dagger.version@</version>
+ </dependency>
+ </dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <version>3.0</version>
+ <configuration>
+ <source>1.5</source>
+ <target>1.5</target>
+ <!-- workaround for http://jira.codehaus.org/browse/MCOMPILER-202 -->
+ <forceJavacCompilerUse>true</forceJavacCompilerUse>
+ </configuration>
+ <dependencies>
+ <dependency>
+ <groupId>@dagger.groupId@</groupId>
+ <artifactId>dagger-compiler</artifactId>
+ <version>@dagger.version@</version>
+ </dependency>
+ </dependencies>
+ </plugin>
+ </plugins>
+ </build>
+</project>
View
30 compiler/src/it/module-type-validation/src/main/java/test/TestModule.java
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2013 Square, Inc.
+ *
+ * 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 test;
+
+import dagger.Module;
+
+@Module
+class ThreadModule extends Thread {
+}
+
+@Module
+enum EnumModule {
+}
+
+@Module
+interface InterfaceModule {
+}
View
10 compiler/src/it/module-type-validation/verify.bsh
@@ -0,0 +1,10 @@
+import dagger.testing.it.BuildLogValidator;
+import java.io.File;
+
+File buildLog = new File(basedir, "build.log");
+new BuildLogValidator().assertHasText(buildLog, new String[]{
+ "Modules must not extend from other classes: test.ThreadModule"});
+new BuildLogValidator().assertHasText(buildLog, new String[]{
+ "Modules must be classes: test.EnumModule"});
+new BuildLogValidator().assertHasText(buildLog, new String[]{
+ "Modules must be classes: test.InterfaceModule"});
View
25 compiler/src/main/java/dagger/internal/codegen/ProvidesProcessor.java
@@ -45,6 +45,7 @@
import javax.lang.model.element.TypeElement;
import javax.lang.model.element.VariableElement;
import javax.lang.model.type.TypeMirror;
+import javax.lang.model.util.Elements;
import javax.tools.Diagnostic;
import javax.tools.JavaFileObject;
@@ -71,7 +72,6 @@
return SourceVersion.latestSupported();
}
- // TODO: include @Provides methods from the superclass
@Override public boolean process(Set<? extends TypeElement> types, RoundEnvironment env) {
remainingTypes.putAll(providerMethodsByClass(env));
for (Iterator<String> i = remainingTypes.keySet().iterator(); i.hasNext();) {
@@ -148,14 +148,27 @@ private void error(String msg, Element element) {
methods.add(providerMethodAsExecutable);
}
+ Elements elementUtils = processingEnv.getElementUtils();
+ TypeMirror objectType = elementUtils.getTypeElement("java.lang.Object").asType();
+
// Catch any stray modules without @Provides since their entry points
// should still be registered and a ModuleAdapter should still be written.
- for (Element type : env.getElementsAnnotatedWith(Module.class)) {
- if (type.getKind().equals(ElementKind.CLASS)) {
- String moduleType = ((TypeElement) type).getQualifiedName().toString();
- if (result.containsKey(moduleType)) continue;
- result.put(moduleType, new ArrayList<ExecutableElement>());
+ for (Element module : env.getElementsAnnotatedWith(Module.class)) {
+ if (!module.getKind().equals(ElementKind.CLASS)) {
+ error("Modules must be classes: " + module, module);
+ continue;
+ }
+
+ TypeElement moduleType = (TypeElement) module;
+
+ // Verify that all modules do not extend from non-Object types.
+ if (!moduleType.getSuperclass().equals(objectType)) {
+ error("Modules must not extend from other classes: " + module, module);
}
+
+ String moduleName = moduleType.getQualifiedName().toString();
+ if (result.containsKey(moduleName)) continue;
+ result.put(moduleName, new ArrayList<ExecutableElement>());
}
return result;
}
View
4 core/src/main/java/dagger/internal/plugins/reflect/ReflectivePlugin.java
@@ -52,6 +52,10 @@
if (annotation == null) {
throw new IllegalArgumentException("No @Module on " + moduleClass.getName());
}
+ if (moduleClass.getSuperclass() != Object.class) {
+ throw new IllegalArgumentException(
+ "Modules must not extend from other classes: " + moduleClass.getName());
+ }
return (ModuleAdapter<T>) new ReflectiveModuleAdapter(moduleClass, annotation);
}
View
14 core/src/test/java/dagger/ModuleIncludesTest.java → core/src/test/java/dagger/ModuleTest.java
@@ -24,7 +24,7 @@
import static org.junit.Assert.fail;
@RunWith(JUnit4.class)
-public final class ModuleIncludesTest {
+public final class ModuleTest {
static class TestEntryPoint {
@Inject String s;
}
@@ -181,4 +181,16 @@
public void childModuleMissingModuleAnnotation() {
ObjectGraph.create(new ChildModuleMissingModuleAnnotation());
}
+
+ @Module
+ static class ThreadModule extends Thread {}
+
+ @Test public void moduleExtendingClassThrowsException() {
+ try {
+ ObjectGraph.create(new ThreadModule());
+ fail();
+ } catch (IllegalArgumentException e) {
+ assertThat(e.getMessage()).startsWith("Modules must not extend from other classes: ");
+ }
+ }
}
Something went wrong with that request. Please try again.