Skip to content

Allow collection-backed annotation array values in regression test#3023

Open
Yong-yuan-X wants to merge 1 commit into
google:mainfrom
Yong-yuan-X:fix-3008-array-annotation-value
Open

Allow collection-backed annotation array values in regression test#3023
Yong-yuan-X wants to merge 1 commit into
google:mainfrom
Yong-yuan-X:fix-3008-array-annotation-value

Conversation

@Yong-yuan-X

@Yong-yuan-X Yong-yuan-X commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Refs #3008

Summary

  • Narrow the annotationArrayValueType regression test to document the currently supported value shapes for array-valued annotation arguments.
  • Update the test to accept annotation array values represented as either Array<*> or Collection<*>.
  • Make the test fail if the value is not one of the supported documented shapes.
  • Keep the regression test as a regular passing runTest.

Tests

  • Ran .\gradlew.bat :kotlin-analysis-api:test --tests "com.google.devtools.ksp.test.AAConfiguredUnitTestSuite.testAnnotationArrayValueType" --tests "com.google.devtools.ksp.test.PsiConfiguredUnitTestSuite.testAnnotationArrayValueType"
  • AAConfiguredUnitTestSuite > testAnnotationArrayValueType() PASSED
  • PsiConfiguredUnitTestSuite > testAnnotationArrayValueType() PASSED
  • BUILD SUCCESSFUL

@jaschdoc

jaschdoc commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Hey @Yong-yuan-X, thank you for another PR. We may have to rethink this a bit. It occurred to me that it's possibly better to treat this defect as a documentation bug. So instead of fixing all occurrences of List<T>, it's better to document that the value can be a list. The real design problem here is that the KSValueArgument.value property has type Any?. Instead, we should have a sum type, e.g., KSValueArgumentElement, that describes the possible values of KSValueArgument.value. However, to avoid breaking changes, we should add this type outside of the API and introduce an extension function or property, KSValueArgument.typedValue : KSValueArgumentElement that users can use in the future.

Thus, I think this PR should update the test you made earlier to allow for the Collection<T> / List<T> case and throw errors if it's not one of the documented types. That change probably also requires changing the expected results, but that's okay. At least we can change it to a passing test.

In the meantime, I will write up a better design in #3008 and if you still want to implement it we can discuss further there.

Again, thank you for the contribution and apologies for changing the goal midway through the work. I hope you understand! :)

@Yong-yuan-X Yong-yuan-X force-pushed the fix-3008-array-annotation-value branch from f2ac2a1 to df3ba04 Compare July 9, 2026 06:27
@Yong-yuan-X

Copy link
Copy Markdown
Contributor Author

@jaschdoc Thank you for the clarification, and no worries about the change in direction. I appreciate the explanation and will adjust the PR accordingly.

I understand the concern that this is more about documenting the currently supported value shapes for KSValueArgument.value, rather than forcing annotation array values to be represented as Array<*>.

I'll update this PR to narrow the scope:

  • revert the production changes that convert list-backed annotation values to arrays;
  • update the annotationArrayValueType regression test to allow Collection/List for array-valued annotation arguments;
  • keep the test as a regular passing runTest;
  • make it fail only when the value is not one of the documented supported types.

I'll also follow the design update in #3008, and I'm happy to help with the future typedValue direction once it is settled.

@Yong-yuan-X Yong-yuan-X changed the title Fix annotation array value type for KSValueArgument Allow collection-backed annotation array values in regression test Jul 9, 2026
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 this pull request may close these issues.

2 participants