Provide a way to manually create SampleAnalysisEnvironment #3589
Conversation
| @@ -49,21 +50,22 @@ internal class DescriptorSampleAnalysisEnvironmentCreator( | |||
| // avoid memory leaks through the compiler's ThreadLocals. | |||
| // Might not be relevant if the project stops using coroutines. | |||
| return runBlocking(Dispatchers.Default) { | |||
There was a problem hiding this comment.
minor question: do we still need this here?
As we now provide create() (even if it's delicate), for me it looks a little strange, that creator.use(block) and creator.create().use(block) will behave differently, and most likely users of this API need to know about internals to work with it properly.
WDYT about this?
There was a problem hiding this comment.
minor question: do we still need this here?
I believe we do, otherwise it can lead to the mentioned issues :) This is the controlled environment that addresses the issues that we're aware of, and more hacks and fixes might be added here in the future
for me it looks a little strange, that creator.use(block) and creator.create().use(block) will behave differently
yeah, the naming is unfortunate :( this is a lesson for the future - do not use names that can clash with the names from stdlib. otherwise, I'd say it's expected:
most likely users of this API need to know about internals to work with it properly.
indeed, that is the whole point of create() being unsafe, delicate and NOT recommended, and use() being the safest and recommended option
- You're using
use()and have encountered a bug/leak/etc? Report it, we'll have a look and try to resolve it (most likely by adding some hacks/workarounds touse()) - You're using
create()and have encountered a bug/leak/etc? Well, we gave no guarantees and warned you, you better research and figure out how to mitigate it on your own. Or start usinguse(), then it becomes our problem
If we remove use(), we'll have to add the runBlocking detail (and any other) to the KDoc of create(), but I see several problems with this approach:
- The users will have to read the docs to apply it - the chances of it are slim as it is
- If more issues arise, the KDoc will have to be updated with the new workarounds, and the users will have to revise it every release, which is also very unlikely
- If some issues are no longer relevant and the workarounds need to be removed - same story
- It kinda breaks the abstraction. This API can have multiple implementations with their own limitations and workarounds. Some might apply to K1, some to K2, some to both. Not to mention any potential 3rd party ones that we're not aware of
So to me it makes sense to have use(), which takes care of all this for you
We can soft-rename use to something else to not clash with kotlin.io.use, but let's do it as a separate issue/PR
| @@ -49,21 +50,22 @@ internal class DescriptorSampleAnalysisEnvironmentCreator( | |||
| // avoid memory leaks through the compiler's ThreadLocals. | |||
| // Might not be relevant if the project stops using coroutines. | |||
| return runBlocking(Dispatchers.Default) { | |||
There was a problem hiding this comment.
minor question: do we still need this here?
I believe we do, otherwise it can lead to the mentioned issues :) This is the controlled environment that addresses the issues that we're aware of, and more hacks and fixes might be added here in the future
for me it looks a little strange, that creator.use(block) and creator.create().use(block) will behave differently
yeah, the naming is unfortunate :( this is a lesson for the future - do not use names that can clash with the names from stdlib. otherwise, I'd say it's expected:
most likely users of this API need to know about internals to work with it properly.
indeed, that is the whole point of create() being unsafe, delicate and NOT recommended, and use() being the safest and recommended option
- You're using
use()and have encountered a bug/leak/etc? Report it, we'll have a look and try to resolve it (most likely by adding some hacks/workarounds touse()) - You're using
create()and have encountered a bug/leak/etc? Well, we gave no guarantees and warned you, you better research and figure out how to mitigate it on your own. Or start usinguse(), then it becomes our problem
If we remove use(), we'll have to add the runBlocking detail (and any other) to the KDoc of create(), but I see several problems with this approach:
- The users will have to read the docs to apply it - the chances of it are slim as it is
- If more issues arise, the KDoc will have to be updated with the new workarounds, and the users will have to revise it every release, which is also very unlikely
- If some issues are no longer relevant and the workarounds need to be removed - same story
- It kinda breaks the abstraction. This API can have multiple implementations with their own limitations and workarounds. Some might apply to K1, some to K2, some to both. Not to mention any potential 3rd party ones that we're not aware of
So to me it makes sense to have use(), which takes care of all this for you
We can soft-rename use to something else to not clash with kotlin.io.use, but let's do it as a separate issue/PR
Fixes #3573
minor changes applied to original branch.
Note:
DelicateDokkaApidescription was taken from coroutines with minimal changes - it's description matches perfectly for our use cases.Note: I haven't implemented mine suggestion from #3573 (comment), as may be
Delicateannotation is enough and we don't need to do additional tracking for niche use-case