-
Notifications
You must be signed in to change notification settings - Fork 409
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
Fix checking for annotations when rendering constructor keyword #3523
Conversation
@@ -1366,6 +1366,63 @@ class SignatureTest : BaseAbstractTest() { | |||
} | |||
} | |||
|
|||
@OnlyDescriptors("#3354") | |||
@Test | |||
fun `should not render constructor for Any from Wasm sources`() = testRender( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I would prefer to have a more general name for this test, e.g. should not render constructor without a renderable annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is such test below, though, named a little more explicit.
This specific case is what was happening in original issue. Mostly to check, that it works the same for kotlin
package and builtin
class, so Im not sure, what will be the best name here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Among other tests this name made me think about a specific logic for Any (we have such a one) or wasm source set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I see why there is a confusion... do you have any other idea on how to name this test, so that the intention will be clear?
Does something like this will be better may be should not render constructor without a renderable annotation for kotlin.Any
? I mean, while there is no special casing for kotlin.Any
, I would still expect to have a test that checks specifically reported problem here, as kotlin builtins are specific things and so it's good to have tests for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name below can be reused as the test has the same logic.
should not render parameterless constructor with annotation without mustBeDocumented annotation - for kotlin.Any
129971f
to
0eafe53
Compare
Fixes #3426
There are 2 cases when annotation is not rendered and so no
constructor
keyword should be rendered:MustBeDocumented
Deprecated
)Note: PR has small API/ABI changes (no breaking changes), that's because ignored annotations are ignored later in pipeline, only during rendering. We can't ignore annotations in
JvmSignatureUtils
, as we need all annotations in other places to make a special handling of those ignored annotations (likeDeprecated
).Also, we can introduce something like
Annotation.shouldBeRendered
instead ofAnnotation.isIgnored
insideJvmSignatureUtils
, but to use it under the hood inannotationsInline
/annotationsBlock
we will need to break API/ABI (or introduce new declaration + deprecate old one) as on current momentignored
annotations there provided explicitly...So I've decided not to go this road.
If we think that it's fine to break API/ABI in
JvmSignatureUtils
and its inheritors (can be seen in API dump changes), then I can update PR.