Skip to content

Allow motif scopes to be abstract classes, to allow for internal visibility modifiers #273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wynneplaga wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow motif scopes to be abstract classes, to allow for internal visibility modifiers #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion compiler/src/main/kotlin/motif/compiler/JavaCodeGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ object JavaCodeGenerator {
.apply {
addAnnotation(scopeImplAnnotation.spec())
addModifiers(Modifier.PUBLIC)
addSuperinterface(superClassName.j)
when (superClassName) {
is SuperClassName.Interface -> addSuperinterface(superClassName.name.j)
is SuperClassName.AbstractClass -> superclass(superClassName.name.j)
}
objectsField?.let { addField(it.spec()) }
addField(dependenciesField.spec())
cacheFields.forEach { addField(it.spec(useNullFieldInitialization)) }
Expand Down
5 changes: 4 additions & 1 deletion compiler/src/main/kotlin/motif/compiler/KotlinCodeGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ object KotlinCodeGenerator {
addAnnotation(suppressAnnotationSpec("REDUNDANT_PROJECTION", "UNCHECKED_CAST"))
addAnnotation(scopeImplAnnotation.spec())
addModifiers(if (internalScope) KModifier.INTERNAL else KModifier.PUBLIC)
addSuperinterface(superClassName.kt)
when (superClassName) {
is SuperClassName.Interface -> addSuperinterface(superClassName.name.kt)
is SuperClassName.AbstractClass -> superclass(superClassName.name.kt)
}
objectsField?.let { addProperty(it.spec()) }
addProperty(dependenciesField.spec())
cacheFields.forEach { addProperty(it.spec(useNullFieldInitialization)) }
Expand Down
8 changes: 7 additions & 1 deletion compiler/src/main/kotlin/motif/compiler/ScopeImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import motif.ast.compiler.CompilerMethod
class ScopeImpl(
val useNullFieldInitialization: Boolean,
val className: ClassName,
val superClassName: ClassName,
val superClassName: SuperClassName,
val internalScope: Boolean,
val scopeImplAnnotation: ScopeImplAnnotation,
val objectsField: ObjectsField?,
Expand All @@ -54,6 +54,12 @@ class ScopeImpl(
val dependencies: Dependencies?,
)

sealed interface SuperClassName {
data class Interface(val name: ClassName) : SuperClassName

data class AbstractClass(val name: ClassName) : SuperClassName
}

/**
* ```
* @ScopeImpl(
Expand Down
9 changes: 8 additions & 1 deletion compiler/src/main/kotlin/motif/compiler/ScopeImplFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import motif.models.ConstructorFactoryMethod
import motif.models.FactoryMethod
import motif.models.FactoryMethodSink
import motif.models.Scope
import motif.models.ScopeMustBeAnInterfaceOrAbstractClass
import motif.models.Sink
import motif.models.Spread
import motif.models.Type
Expand Down Expand Up @@ -67,13 +68,19 @@ private constructor(

fun create(): ScopeImpl {
val isInternal = (scope.clazz as? CompilerClass)?.isInternal() ?: false
val superType =
when {
scope.clazz.kind == IrClass.Kind.INTERFACE -> SuperClassName.Interface(scope.typeName)
scope.clazz.isAbstract() -> SuperClassName.AbstractClass(scope.typeName)
else -> throw ScopeMustBeAnInterfaceOrAbstractClass(scope.clazz)
}
return ScopeImpl(
(scope.clazz.annotations
.find { it.className == motif.Scope::class.java.name }!!
.annotationValueMap[SCOPE_ANNOTATION_FIELD_USE_NULL]
as? Boolean) ?: false,
scope.implClassName,
scope.typeName,
superType,
isInternal,
scopeImplAnnotation(),
objectsField(),
Expand Down
5 changes: 3 additions & 2 deletions errormessage/src/main/kotlin/motif/errormessage/ErrorHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import motif.models.ObjectsConstructorFound
import motif.models.ObjectsFieldFound
import motif.models.ParsingError
import motif.models.ScopeExtendsScope
import motif.models.ScopeMustBeAnInterface
import motif.models.ScopeMustBeAnInterfaceOrAbstractClass
import motif.models.UnspreadableType
import motif.models.VoidDependenciesMethod
import motif.models.VoidFactoryMethod
Expand All @@ -60,7 +60,8 @@ internal interface ErrorHandler {
when (error) {
is ParsingError ->
when (error) {
is ScopeMustBeAnInterface -> ScopeMustBeAnInterfaceHandler(error)
is ScopeMustBeAnInterfaceOrAbstractClass ->
ScopeMustBeAnInterfaceOrAbstractClassHandler(error)
is VoidScopeMethod -> VoidScopeMethodHandler(error)
is AccessMethodParameters -> AccessMethodParametersHandler(error)
is ObjectsFieldFound -> ObjectsFieldFoundHandler(error)
Expand Down
9 changes: 5 additions & 4 deletions ...rmessage/ScopeMustBeAnInterfaceHandler.kt → ...ustBeAnInterfaceOrAbstractClassHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,18 @@
*/
package motif.errormessage

import motif.models.ScopeMustBeAnInterface
import motif.models.ScopeMustBeAnInterfaceOrAbstractClass

internal class ScopeMustBeAnInterfaceHandler(private val error: ScopeMustBeAnInterface) :
ErrorHandler {
internal class ScopeMustBeAnInterfaceOrAbstractClassHandler(
private val error: ScopeMustBeAnInterfaceOrAbstractClass,
) : ErrorHandler {

override val name = "SCOPE CLASS"

override fun StringBuilder.handle() {
appendLine(
"""
Scope must be an interface:
Scope must be an interface or abstract class:

${error.scopeClass.qualifiedName}
"""
Expand Down
4 changes: 2 additions & 2 deletions ...llij/src/main/kotlin/motif/intellij/hierarchy/descriptor/ScopeHierarchyErrorDescriptor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import motif.models.NullableParameter
import motif.models.NullableSpreadMethod
import motif.models.ObjectsConstructorFound
import motif.models.ObjectsFieldFound
import motif.models.ScopeMustBeAnInterface
import motif.models.ScopeMustBeAnInterfaceOrAbstractClass
import motif.models.UnspreadableType
import motif.models.VoidDependenciesMethod
import motif.models.VoidFactoryMethod
Expand All @@ -70,7 +70,7 @@ open class ScopeHierarchyErrorDescriptor(
companion object {
fun getElementFromError(error: MotifError): PsiElement =
when (error) {
is ScopeMustBeAnInterface -> {
is ScopeMustBeAnInterfaceOrAbstractClass -> {
(error.scopeClass as IntelliJClass).psiClass
}
is VoidScopeMethod -> {
Expand Down
2 changes: 1 addition & 1 deletion models/src/main/kotlin/motif/models/ParsingError.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import motif.ast.IrType

sealed class ParsingError : RuntimeException(), MotifError

class ScopeMustBeAnInterface(val scopeClass: IrClass) : ParsingError()
class ScopeMustBeAnInterfaceOrAbstractClass(val scopeClass: IrClass) : ParsingError()

class VoidScopeMethod(val scope: Scope, val method: IrMethod) : ParsingError()

Expand Down
4 changes: 3 additions & 1 deletion models/src/main/kotlin/motif/models/Scope.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ class ValidScope internal constructor(clazz: IrClass, useNullFieldInitialization
Scope(useNullFieldInitialization, clazz) {

init {
if (clazz.kind != IrClass.Kind.INTERFACE) throw ScopeMustBeAnInterface(clazz)
if (clazz.kind != IrClass.Kind.INTERFACE && !clazz.isAbstract()) {
throw ScopeMustBeAnInterfaceOrAbstractClass(clazz)
}
detectScopeSuperinterface(this)
}

Expand Down
8 changes: 4 additions & 4 deletions samples/sample/src/main/java/motif/sample/app/photo_grid/PhotoGridScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@
import motif.sample.lib.db.Photo;

@Scope(useNullFieldInitialization = true)
public interface PhotoGridScope {
public abstract class PhotoGridScope {
Copy link
Copy Markdown
Contributor

@TonyTangAndroid TonyTangAndroid Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we examine any other Dependency injection system, like Dagger component, we would notice that it is also an interface, rather than a class. The underlying rationale, if I have to guess, is because that a DI system is suppose to be functioning like a container that gives us something to use through the access method like public abstract PhotoGridView view();. And that is its only purpose.

If we make it as class, it signals that this class is serving other purpose, which complicates things, especially due to the fact that the DI system is already complicated enough.

Hence, though I am not the original author , I would bet that it is a purposeful decision on making it as interface. And I would still recommend that we keep it as interface. If you have legitimate use case that need to make it as class, we could sync offline in Uber internally to see whether there is any alternative that I could suggest to meet your needs.

@jbarr21 @Leland-Takamine

Copy link
Copy Markdown
Author

@wynneplaga wynneplaga Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dagger actually does permit abstract classes to be components (source), as does Metro's graph extensions (source).

I definitely agree that allowing scopes to be abstract classes is less than ideal, but I think preventing modules from encapsulating the types they provide through the DI framework is worse in terms of code cleanliness. I also think it would be trivial to design a detekt plugin that forbids fields in motif scopes.


PhotoGridView view();
public abstract PhotoGridView view();

PhotoGridItemScope photoRow(PhotoGridItemView view, Photo photo);
abstract PhotoGridItemScope photoRow(PhotoGridItemView view, Photo photo);

@motif.Objects
abstract class Objects extends ControllerObjects<PhotoGridController, PhotoGridView> {
abstract static class Objects extends ControllerObjects<PhotoGridController, PhotoGridView> {

abstract PhotoGridAdapter adapter();
}
Expand Down
2 changes: 1 addition & 1 deletion tests/src/main/java/testcases/E024_scope_class/ERROR.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

[SCOPE CLASS]

Scope must be an interface:
Scope must be an interface or abstract class:

testcases.E024_scope_class.Scope

Expand Down