facebook / facebook/litho

Use of TypeMirror#toString in SpecElementTypeDeterminator is fragile

Open
#1,083 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Kotlin
Stars
7.8k
Forks
768
PR merge metrics
No merged PRs in 30d

Description

`SpecElementTypeDeterminator` uses `TypeMirror#toString` to test types:

https://github.com/facebook/litho/blob/55e28e58930a53133620fd32406ec8c3a9116c0c/litho-processor/src/main/java/com/facebook/litho/specmodels/processor/SpecElementTypeDeterminator.java#L75

This is fragile because `TypeMirror#toString` will return any type annotations that are present on the type, which breaks the comparison.

I think this isn't currently causing problems because that logic is used to test fields in Kotlin classes, and when using kapt it creates stubs with fully qualified class names, and there's a javac bug that causes the type annotations to not be correctly handled in that case ([JDK-8381925](https://bugs.openjdk.org/browse/JDK-8381925)).

For example with

```
package com.facebook.litho;

public static class FakeKotlinSingleton {
@Nullable public static final com.facebook.litho.FakeKotlinSingleton INSTANCE = null;
}
```

The type should be printed as `com.facebook.litho.@Nullable FakeKotlinSingleton`.

The tests for SpecElementTypeDeterminator don't cover this because they use `javax.annotation.Nullable` instead of `org.jetbrains.annotations.NotNull`, and the javax annotation isn't a type annotation.

https://github.com/facebook/litho/blob/55e28e58930a53133620fd32406ec8c3a9116c0c/litho-it/src/test/com/facebook/litho/specmodels/processor/SpecElementTypeDeterminatorTest.java#L41

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.