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] Crash with primitive-named assisted parameters #3995

Closed
ZacSweers opened this issue Aug 3, 2023 · 2 comments
Closed

[KSP] Crash with primitive-named assisted parameters #3995

ZacSweers opened this issue Aug 3, 2023 · 2 comments

Comments

@ZacSweers
Copy link

The following code snippet crashes the KSP processor with Dagger version 2.47

package com.squareup.test

import dagger.assisted.Assisted
import dagger.assisted.AssistedFactory
import dagger.assisted.AssistedInject

data class AssistedService @AssistedInject constructor(
  val int: Int,
  @Assisted val string: String,
  @Assisted val long: Long
)

@AssistedFactory
interface AssistedServiceFactory {
  fun create(long: Long, other: String): AssistedService
}

Trace

e: Error occurred in KSP, check log for detail
e: [ksp] java.lang.IllegalArgumentException: not a valid name: long
	at com.squareup.javapoet.Util.checkArgument(Util.java:53)
	at com.squareup.javapoet.ParameterSpec.builder(ParameterSpec.java:117)
	at dagger.internal.codegen.xprocessing.MethodSpecs.overriding(MethodSpecs.java:52)
	at dagger.internal.codegen.processingstep.AssistedFactoryProcessingStep$AssistedFactoryImplGenerator.topLevelTypes(AssistedFactoryProcessingStep.java:306)
	at dagger.internal.codegen.processingstep.AssistedFactoryProcessingStep$AssistedFactoryImplGenerator.topLevelTypes(AssistedFactoryProcessingStep.java:232)
	at dagger.internal.codegen.base.SourceFileGenerator.generate(SourceFileGenerator.java:77)
	at dagger.internal.codegen.processingstep.AssistedFactoryProcessingStep.process(AssistedFactoryProcessingStep.java:114)
	at dagger.internal.codegen.processingstep.AssistedFactoryProcessingStep.process(AssistedFactoryProcessingStep.java:76)
	at dagger.internal.codegen.processingstep.TypeCheckingProcessingStep.lambda$process$0(TypeCheckingProcessingStep.java:82)
	at com.google.common.collect.SingletonImmutableBiMap.forEach(SingletonImmutableBiMap.java:68)
	at dagger.internal.codegen.processingstep.TypeCheckingProcessingStep.process(TypeCheckingProcessingStep.java:70)
	at dagger.internal.codegen.processingstep.TypeCheckingProcessingStep.process(TypeCheckingProcessingStep.java:48)
	at dagger.spi.shaded.androidx.room.compiler.processing.XProcessingStep.process(XProcessingStep.kt:59)
	at dagger.spi.shaded.androidx.room.compiler.processing.CommonProcessorDelegate.processRound(XBasicAnnotationProcessor.kt:130)
	at dagger.spi.shaded.androidx.room.compiler.processing.ksp.KspBasicAnnotationProcessor.process(KspBasicAnnotationProcessor.kt:62)
@bcorso
Copy link

bcorso commented Aug 3, 2023

Thanks for reporting this!

In theory, I think this is likely an issue for any parameter name in a user's sources that Dagger ends up using in generated sources, though it may be the case that AssistedInject is where this is easiest to hit. It mostly works for KAPT today because KAPT either completely elides elements that have java keywords as names, or it just renames them.

However, given that Dagger is still generating Java source, I'm leaning towards just reporting an error in these cases (i.e. it wouldn't be allowed in KAPT or KSP). I'll talk it over with the team, but also if you have concerns with this feel free to state them.

@ZacSweers
Copy link
Author

Interesting! Good to know this is easy to fix on the consumer side, I completely missed that this was the parameter name and thought it was about the parameter type at first.

@ZacSweers ZacSweers changed the title [KSP] Crash with primitive assisted parameters [KSP] Crash with primitive-named assisted parameters Aug 3, 2023
copybara-service bot pushed a commit that referenced this issue Aug 16, 2023
Using keywords as parameter names has been working with KAPT because KAPT just renames the parameters to something that is valid Java in the generated Java stubs. However, in order to support this in KSP we need to switch over to use `XExecutableParameterElement#jvmName` rather than `XExecutableParameterElement#name`.

This CL also requires updates from XProcessing, so I've updated Dagger's XProcessing jars as well.

Fixes #3995: Allow kotlin source to use java keywords as parameter names in KSP

RELNOTES=Fixed #3995: Allow kotlin source to use java keywords as parameter names in KSP
PiperOrigin-RevId: 553376843
copybara-service bot pushed a commit that referenced this issue Aug 16, 2023
Using keywords as parameter names has been working with KAPT because KAPT just renames the parameters to something that is valid Java in the generated Java stubs. However, in order to support this in KSP we need to switch over to use `XExecutableParameterElement#jvmName` rather than `XExecutableParameterElement#name`.

This CL also requires updates from XProcessing, so I've updated Dagger's XProcessing jars as well.

Fixes #3995: Allow kotlin source to use java keywords as parameter names in KSP

RELNOTES=Fixed #3995: Allow kotlin source to use java keywords as parameter names in KSP
PiperOrigin-RevId: 553376843
copybara-service bot pushed a commit that referenced this issue Aug 16, 2023
Using keywords as parameter names has been working with KAPT because KAPT just renames the parameters to something that is valid Java in the generated Java stubs. However, in order to support this in KSP we need to switch over to use `XExecutableParameterElement#jvmName` rather than `XExecutableParameterElement#name`.

This CL also requires updates from XProcessing, so I've updated Dagger's XProcessing jars as well.

Fixes #3995: Allow kotlin source to use java keywords as parameter names in KSP

RELNOTES=Fixed #3995: Allow kotlin source to use java keywords as parameter names in KSP
PiperOrigin-RevId: 553376843
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

Successfully merging a pull request may close this issue.

2 participants