Skip to content

Commit

Permalink
Disallow @Objects-annotated classes to have fields or constructors
Browse files Browse the repository at this point in the history
  • Loading branch information
Leland Takamine committed Aug 20, 2018
1 parent 45f0a96 commit c9919c1
Show file tree
Hide file tree
Showing 18 changed files with 364 additions and 10 deletions.
2 changes: 2 additions & 0 deletions compiler/src/main/java/motif/compiler/Processor.kt
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class Processor : AbstractProcessor() {

// For testing only.
var validationErrors: GraphValidationErrors? = null
var parsingError: ParsingError? = null

private val hasErrors: Boolean
get() = !(validationErrors?.isEmpty() ?: true)
Expand All @@ -56,6 +57,7 @@ class Processor : AbstractProcessor() {
val sourceSet = try {
SourceSetFactory(processingEnv).create(roundEnv)
} catch (e: ParsingError) {
this.parsingError = e
processingEnv.messager.printMessage(Diagnostic.Kind.ERROR, "\n${e.message}\n${e.element}\n", e.element)
return
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,17 @@ import javax.lang.model.element.Element
import javax.lang.model.element.Modifier
import javax.lang.model.type.DeclaredType

class ObjectsImplFactory(override val env: ProcessingEnvironment) : JavaxUtil {
class ObjectsClassFactory(override val env: ProcessingEnvironment) : JavaxUtil {

fun create(scopeType: DeclaredType): ObjectsClass? {
val objectsType = scopeType.annotatedInnerType(Objects::class) ?: return null
if (objectsType.hasFieldsRecursive()) {

This comment has been minimized.

Copy link
@andreasnomikos

andreasnomikos Aug 20, 2018

how about static fields?

This comment has been minimized.

Copy link
@Leland-Takamine

Leland-Takamine Aug 20, 2018

Collaborator

This will also prevent static fields which we technically can support, but it seems clearer from an API standpoint to just ban all fields here.

This comment has been minimized.

Copy link
@andreasnomikos

andreasnomikos Aug 20, 2018

Seems that we should not be limiting the developers in this case since the point of this is that the Objects class should not be Instantiable.

This comment has been minimized.

Copy link
@andreasnomikos

andreasnomikos Aug 20, 2018

However it probably is not a big deal in the short term so pick whatever is easier.

This comment has been minimized.

Copy link
@Leland-Takamine

Leland-Takamine Aug 20, 2018

Collaborator

I'll update this to allow static fields.

This comment has been minimized.

Copy link
@Leland-Takamine

Leland-Takamine Aug 20, 2018

Collaborator

Updated: d5ef7b9

throw ParsingError(objectsType.asElement(), "@Objects-annotated class may not declare any fields.")
}
if (objectsType.hasNonDefaultConstructor()) {
throw ParsingError(objectsType.asElement(), "@Objects-annotated class may not declare any non-default constructors.")
}

val methods = objectsType.methods()
.onEach {
if (it.isVoid) throw ParsingError(it.element, "Factory method must not return void")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class ScopeClassFactory(override val env: ProcessingEnvironment) : JavaxUtil {

private val childFactory = ChildDeclarationFactory(env)
private val accessMethodFactory = AccessMethodFactory(env)
private val objectsClassFactory = ObjectsImplFactory(env)
private val objectsClassFactory = ObjectsClassFactory(env)
private val explicitDependenciesFactory = ExplicitDependenciesFactory(env)

fun create(scopeType: DeclaredType): ScopeClass {
Expand Down
26 changes: 26 additions & 0 deletions compiler/src/main/java/motif/compiler/javax/JavaxUtil.kt
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import javax.lang.model.element.ExecutableElement
import javax.lang.model.element.TypeElement
import javax.lang.model.type.DeclaredType
import javax.lang.model.type.ExecutableType
import javax.lang.model.type.TypeKind
import javax.lang.model.type.TypeMirror
import javax.lang.model.util.ElementFilter
import kotlin.reflect.KClass
Expand Down Expand Up @@ -73,6 +74,31 @@ interface JavaxUtil {
return ElementFilter.constructorsIn(asElement().enclosedElements)
}

fun DeclaredType.hasFieldsRecursive(): Boolean {
val typeElement: TypeElement = asElement() as TypeElement
if (ElementFilter.fieldsIn(typeElement.enclosedElements).isNotEmpty()) {
return true
}

val superclass: TypeMirror = typeElement.superclass
if (superclass.kind == TypeKind.DECLARED) {
if ((superclass as DeclaredType).hasFieldsRecursive()) {
return true
}
} else if (superclass.kind != TypeKind.NONE) {
// TODO Is it possible for TypeElement.superclass to return a TypeMirror of TypeKind other than

This comment has been minimized.

Copy link
@andreasnomikos

andreasnomikos Aug 20, 2018

It can be TypeKind.Error

This comment has been minimized.

Copy link
@Leland-Takamine

Leland-Takamine Aug 20, 2018

Collaborator

Unsure if we want to support this case. This is only valid if the parent class is generated in a subsequent annotation processing round right?

This comment has been minimized.

Copy link
@andreasnomikos

andreasnomikos Aug 20, 2018

depending on the setup can also happen if the developer mistyped the parent or forgot to add a dependency to the classpath in which case it is a missing symbol and the motif compiler should back up gracefully and let javac surface the error. Should probably add a test to make sure that the correct missing symbol error is thrown in that case and that motif compiler itself doesn't hard fail or increase error spew.

This comment has been minimized.

Copy link
@Leland-Takamine

Leland-Takamine Aug 20, 2018

Collaborator

I'll open a separate issue for this since we'll need to handle this in a bunch of other places.

This comment has been minimized.

Copy link
@Leland-Takamine

Leland-Takamine Aug 20, 2018

Collaborator

Tracked here: #15

// DECLARED or NONE? If so, then we should not throw an Exception here and instead handle the
// recursion properly.
throw IllegalStateException("Unknown superclass type.")
}

return false
}

fun DeclaredType.hasNonDefaultConstructor(): Boolean {
return constructors().find { !it.parameters.isEmpty() } != null
}

fun DeclaredType.packageName(): String {
return MoreElements.getPackage(asElement()).qualifiedName.toString()
}
Expand Down
13 changes: 11 additions & 2 deletions compiler/src/test/java/motif/compiler/TestHarness.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.common.truth.Truth;
import com.google.testing.compile.Compilation;
import com.google.testing.compile.MotifTestCompiler;
import motif.compiler.errors.parsing.ParsingError;
import motif.ir.graph.errors.GraphValidationErrors;
import motif.stubcompiler.StubProcessor;
import org.junit.Ignore;
Expand Down Expand Up @@ -132,8 +133,16 @@ public void test() throws Throwable {
GraphValidationErrors errors = processor.getValidationErrors();
Truth.assertThat(errors).isNotNull();
expectedException.set(null, errors);
} catch (NoSuchFieldException ignore) {
assertThat(compilation).succeeded();
} catch (NoSuchFieldException e) {
try {
Field expectedException = testClass.getField("parsingError");
assertThat(compilation).failed();
ParsingError error = processor.getParsingError();
Truth.assertThat(error).isNotNull();
expectedException.set(null, error);
} catch (NoSuchFieldException ee) {
assertThat(compilation).succeeded();
}
}

try {
Expand Down
26 changes: 26 additions & 0 deletions it/src/main/java/testcases/E019_objects_fields/Scope.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E019_objects_fields;

@motif.Scope
public interface Scope {

@motif.Objects
class Objects {

private final String s = "s";
}
}
29 changes: 29 additions & 0 deletions it/src/main/java/testcases/E019_objects_fields/Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E019_objects_fields;

import motif.compiler.errors.parsing.ParsingError;

import static com.google.common.truth.Truth.assertThat;

public class Test {

public static ParsingError parsingError;

public static void run() {
assertThat(parsingError).isNotNull();
}
}
26 changes: 26 additions & 0 deletions it/src/main/java/testcases/E020_objects_constructor/Scope.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E020_objects_constructor;

@motif.Scope
public interface Scope {

@motif.Objects
class Objects {

Objects(String s) {}
}
}
29 changes: 29 additions & 0 deletions it/src/main/java/testcases/E020_objects_constructor/Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E020_objects_constructor;

import motif.compiler.errors.parsing.ParsingError;

import static com.google.common.truth.Truth.assertThat;

public class Test {

public static ParsingError parsingError;

public static void run() {
assertThat(parsingError).isNotNull();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E021_nullable_factory_method_return;

import javax.annotation.Nullable;

@motif.Scope
public interface Scope {

@motif.Objects
class Objects {

@Nullable
String string() {
return "s";
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E021_nullable_factory_method_return;

import motif.compiler.errors.parsing.ParsingError;

import static com.google.common.truth.Truth.assertThat;

public class Test {

public static ParsingError parsingError;

public static void run() {
assertThat(parsingError).isNotNull();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E022_nullable_factory_method_param;

import javax.annotation.Nullable;

@motif.Scope
public interface Scope {

@motif.Objects
class Objects {

Integer string(@Nullable String s) {
return 1;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E022_nullable_factory_method_param;

import motif.compiler.errors.parsing.ParsingError;

import static com.google.common.truth.Truth.assertThat;

public class Test {

public static ParsingError parsingError;

public static void run() {
assertThat(parsingError).isNotNull();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E023_objects_fields_inherited;

public class ObjectsParent {

private final String s = "s";

public String s() {
return s;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright (c) 2018 Uber Technologies, 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 testcases.E023_objects_fields_inherited;

@motif.Scope
public interface Scope {

@motif.Objects
class Objects extends ObjectsParent {}
}
Loading

0 comments on commit c9919c1

Please sign in to comment.