Skip to content
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.

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

KSP does not read actualized typealias declaration #1676

Closed
ZacSweers opened this issue Jan 8, 2024 · 7 comments
Closed

KSP does not read actualized typealias declaration #1676

ZacSweers opened this issue Jan 8, 2024 · 7 comments
Assignees
Milestone

Comments

@ZacSweers
Copy link
Contributor

This is an odd case I ran into with a multiplatform project. In short, you can consider a case like this with dagger annotations:

// In commonMain
@Target(CLASS) expect annotation class AssistedFactory()

// In jvmMain
actual typealias AssistedFactory = dagger.assisted.AssistedFactory

The surprising behavior I've found is that when KSP represents this as an annotation on a declaration, it does not see the actualized typealias (and thus its target) and instead just reports the common type.

I'm not sure if this is intended to work or not, but I was surprised by it because this was while processing in the kspKotlinJvm task.

For a more involved example and repro, you can see this PR and disable the lenient mode I had to add to the processor to support this: slackhq/circuit#1103

github-merge-queue bot pushed a commit to slackhq/circuit that referenced this issue Jan 11, 2024
This PR converts the STAR sample into a multiplatform project!

The commits are a little winding, so I'd suggest looking at the files
changed view for code and commits just for titles/story telling. The
line diff is almost entirely just the updated baseline profile as I
fixed the missing startup prof along the way.

**Highlights**
- The STAR app now runs on Android and Desktop. This is just a starting
point, support for others to continue later.
- Update to Coil 3.0 alphas (with multiplatform support)
- Revamped CoilRule project (which is now also multiplatform)
- Using commonized annotations for both parcelize and dagger worked
remarkably well, allowing almost all the impls to live in common
sources.
- Focused just on JVM for now for scoping reasons. Generified or
expect/actual'd things where it made sense.
- Expect/actual for overlay functions actually feels like a really slick
indirection pattern, as it can let us use a bottom sheet on mobile and
something more appropriate like a dialog for desktop.
- The `Platform.kt` pattern works well here.
- Multiplatform windowSizeClass works nicely:
https://github.com/chrisbanes/material3-windowsizeclass-multiplatform
- Supports dark mode via cmd+U. Quits with cmd+W.
- Add STAR instrumentation tests to CI.
- Switch to kotlinx-datetime for time APIs

**Rough edges**
- Shared resources don't really have a story in compose multiplatform
currently. There's some libraries and tools out there, but out of scope
for this PR. Now we just have a `Strings.kt` file with our three
strings.
- Kapt cannot run on a KMP project with both Android and JVM targets, so
we have to do annoying tedium to conditionally enable the JVM target
only when requested. Ultimately the better solution would be to move to
KSP or split them into separate subprojects.
- I tried out [kotlin-inject](https://github.com/evant/kotlin-inject) as
a KMP solution and... there's a lot of missing features that I felt made
it a little untenable. Might still be the only KMP solution available
for the foreseeable future though.
  - No modules support, everything is a component
  - Obviously no anvil/contributing support
- Multibindings must be provided in the host component directly. Unclear
how to provide lazy multibindings like `Map<Class, Provider<Activity>>`.
- IDE completions often didn't know about common sources that could be
imported, namely annotations.
- KSP cannot see through typealias'd `actual` annotations 🤔. This
resulted in my needing to support a `lenient` mode that allows for short
name matching on the annotation rather than the fully qualified one.
Filed google/ksp#1676.
- Desktop-specific - unclear where to store persistent data like a DB or
token storage. Currently uses an in-memory DB and `FakeFileSystem` for
token storage 🤷.
- The IDE seems to use significantly more memory when doing a lot of KMP
work. I was maxing out all 4gb of memory assigned to Android Studio in
this branch.

**TODO**

- [x] Filter overlay on Desktop
- [x] Touch up info screen padding
- [x] Touch up landscape detail page
- [x] Escape key as a back nav event on desktop seems flaky at best,
probably worth putting back the nav icon.
- [x] Finish getting this working in CI. It's annoying because `check`
invokes the double kapt jvm/android issue. Probably need to just disable
the desktop target by default and make it opt-in for building locally.



https://github.com/slackhq/circuit/assets/1361086/b8071010-72ef-49c1-8c85-34b9c1c9973c

---------

Co-authored-by: Josh Stagg <[email protected]>
@Whole-Hog-Bakery
Copy link

ksp does not recognise typealias for android room dao

e.g.

given

typealias BaseDao<T> = AbstractDao<T, Long>

@Dao
abstract class AbstractDao<TEntity : IEntity<TKey?>, TKey : Any> {

    @Insert
    abstract fun save(obj: TEntity): Long

    @Insert
    abstract fun save(vararg obj: TEntity)

    @Insert
    abstract suspend fun saveAsync(vararg obj: TEntity)

    @Update
    abstract fun update(obj: TEntity)

    @Delete
    abstract fun delete(obj: TEntity)

ksp will not generate any of the functions if i extend BaseDao<> however by using AbstractDao directly ksp works fine

@ZacSweers
Copy link
Contributor Author

It seems this may be a limit in kotlinc. @madsager was seeing some similar stuff at the compiler level with the parcelize plugin in K2 as well

@ting-yuan
Copy link
Collaborator

In the slackhq/circuit#1103, this check failed:

          it.annotationType.resolve().declaration.qualifiedName?.asString() == qualifiedName

because it.annotationType.resolve().declaration points to a KSTypeAlias rather than KSClassDeclaration. Processors need to explicitly resolve type aliases to the underlying declaration, like here.

I'm sorry that this may appear to be inconvenient but it is actually a trade-off between convenience and the ability to visit the type aliases.

@ZacSweers
Copy link
Contributor Author

Gotcha, thanks!

@ZacSweers
Copy link
Contributor Author

That doesn't appear to work for expect/actual aliased annotations

See: slackhq/circuit#1521

Namely, it's resolving as a KSClassDeclaration and not a KSTypeAlias

image

@ting-yuan
Copy link
Collaborator

ting-yuan commented Oct 30, 2024

Sorry my previous comment doesn't apply to expect / actual. These are 2 issues: expect/actual and typealias.

For typealias, processors either need to unwrap it explicitly, or they can use getSymbolsWithAnnotation after the fix is merged.

For expect / actual, I guess what you need is KSDeclaration.findActuals. Unfortunately, it is (imho) the Kotlin / K2 design that finding actuals from a expect is discouraged. Because of that, we can't find a reliable way to implement KSDeclaration.findActuals and have to make it part of incompatible change in KSP2.

Going forward, I guess the alternatives @ZacSweers mentioned in google/dagger#4356 (comment) are the ways to go.

@ting-yuan
Copy link
Collaborator

@ZacSweers I'm closing this as WAI for now. Please feel free to reopen for more discussion or if I misunderstand anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants