Add checks and error messages for ProtoUnknownFields annotation#5755
Add checks and error messages for ProtoUnknownFields annotation#5755xiaozhikang0916 wants to merge 5 commits intoJetBrains:masterfrom
Conversation
Code Owners
|
|
|
||
| DiagnosticFactory2<KtAnnotationEntry, String, String> PROTOBUF_PROTO_NUM_DUPLICATED = DiagnosticFactory2.create(WARNING); | ||
|
|
||
| DiagnosticFactory2<KtAnnotationEntry, String, String> PROTO_UNKNOWN_FIELDS_MULTIPLE_ANNOTATIONS = DiagnosticFactory2.create(ERROR); |
There was a problem hiding this comment.
@sandwwraith, should we also add these checks into K1?
| reporter: DiagnosticReporter, | ||
| ) { | ||
| val annotatedProps = mutableListOf<FirPropertySymbol>() | ||
| classSymbol.processAllDeclarations(session) { member -> |
There was a problem hiding this comment.
Can we use properties.serializableProperties here?
| ) | ||
| map.put( | ||
| FirSerializationErrors.PROTO_UNKNOWN_FIELDS_MULTIPLE_ANNOTATIONS, | ||
| "In class ''{0}'', fields annotated with @ProtoUnknownFields: ''{1}''; only one field per class is allowed.", |
There was a problem hiding this comment.
I think it's better to start with the cause of the error, like
"@ProtoUnknownFields must not be present on more than one property, specified on {0} in class {1}".
Same for the other messages.
@sandwwraith, WDYT?
|
|
||
| @SerialInfo | ||
| @Target(AnnotationTarget.PROPERTY) | ||
| public annotation class ProtoUnknownFields |
There was a problem hiding this comment.
@sandwwraith, is there a way to remind ourselves to delete these classes when the serialization runtime is updated?
There was a problem hiding this comment.
Should I mark this pr as draft and remove them after Kotlin/kotlinx.serialization#2860 is merged?
|
|
||
| // OK: non-nullable with default value | ||
| @Serializable | ||
| data class ValidNonNullable(@ProtoUnknownFields val unknown: <!SERIALIZER_NOT_FOUND!>ProtoUnknownFieldHolder<!> = ProtoUnknownFieldHolder.Empty) |
There was a problem hiding this comment.
Let's create a dummy serializer for the tests?
After a while, it will be hard to remember why error SERIALIZER_NOT_FOUND is expected. Especially after updating the runtime
| val protoOneOfAnnotationClassId = ClassId.topLevel(protoOneOfAnnotationFqName) | ||
|
|
||
| val protoUnknownFieldsAnnotationClassId = ClassId.topLevel(protoUnknownFieldsAnnotationFqName) | ||
| val protoUnknownFieldHolderClassId = ClassId(FqName("kotlinx.serialization.protobuf"), Name.identifier("ProtoUnknownFieldHolder")) |
There was a problem hiding this comment.
For consistency let's create ...qName property for this class name
Add a dummy KSerializer implementation for ProtoUnknownFieldHolder to avoid SERIALIZER_NOT_FOUND errors in tests. This makes the test intent clearer and more maintainable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
A following up pull request for Kotlin/kotlinx.serialization/pull/2860 to check whether a class is matching these limitations when using
@ProtoUnknownFields@ProtoUnknownFields@ProtoUnknownFieldsshould beProtoUnknownFieldHolder@ProtoUnknownFieldsshould have default valueProtoUnknownFieldHolder.EmptyBuild and test passed locally via
./gradlew :kotlinx-serialization-compiler-plugin:test