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

Support declaring a function that can accept a throwing lambda #33

Open
daniel-jasinski opened this issue May 18, 2024 · 9 comments
Open
Assignees

Comments

@daniel-jasinski
Copy link

It would be great if we could declare functions like callSomething as safe for throwing lambdas.
Then we could make the Throws declaration on the caller to also apply to the lambda body.

image

@Tvede-dk Tvede-dk self-assigned this Jul 7, 2024
@Tvede-dk
Copy link
Collaborator

Hi,
so this was actually implemented as a POC, based on this small annotations lib that I have as well
https://github.com/csense-oss/csense-kotlin-annotations
In there there is an annotation named "RethrowsExceptions" and "CatchesExceptions". As the name implies if these are put on a lambda, then that would either be the same as "callThoughts" (rethrows exceptions) or whenever it catches the exception.
Eg
image
So far I would recommend to create the annotation in your own project, as I'm working on getting the various libraries into Maven central, which means the package names will have to change ... :(
The package name(s) that the plugin currently recognizes are

//for rethrows / callthough
csense.kotlin.annotations.exceptions.RethrowsExceptions
org.csenseoss.kotlin.annotations.exceptions.RethrowsExceptions
//for catching
csense.kotlin.annotations.exceptions.CatchesExceptions
org.csenseoss.kotlin.annotations.exceptions.CatchesExceptions

I'm at this point not aware of any regular annotation libraries that have these types of annotations. (and or any that is kotlin MPP as well).

There might also be some cases that I have not yet covered in the plugin, but it should work just fine (just as fine as the custom files).

@GeorgeVoloshin
Copy link

Hello,

Sorry, but is there any way to do the same thing with library functions, such as async or launch?
Or is the only way to write my own wrapper with the @RethrowsExceptions or @CatchesExceptions annotation?

@daniel-jasinski
Copy link
Author

Thanks @Tvede-dk !

These annotations are exactly what I was looking for!
Would these annotations also work with SOURCE retention instead of BINARY?

@Tvede-dk
Copy link
Collaborator

Hi @GeorgeVoloshin
The simple answer is that any function with either annotations or contracts (calls in place) should be parsed.
Any other type of function is either hardcoded in the plugin (so I try to hardcode most of the standard libraries), or it comes from the files.
I could also add the possibility of doing a comment on the function, like a "middle" ground between annotations and the other types of declaring this. Do you have an example of what you want?

@Tvede-dk
Copy link
Collaborator

@daniel-jasinski hehe, that depends. If you have directly access to the code via say a submodule it would work with source retention. BUT as soon as its turned into any binary files (jar, war, apk .... etc), then binary retention is needed. Thus if it is a published package at a maven repo, it would need to be binary. (otherwise the plugin usually have no way of getting the annotation(s)). I have seen examples where the plugin gets the source code instead of binary code, however, that is super flaky to rely on. (I have no idea how IDEA / IntelliJ decides what "code" to present to a plugin, its a hit and miss thing). To be on the safe side I would do binary retention.

@GeorgeVoloshin
Copy link

@Tvede-dk
Hmm...
Maybe I'm doing something wrong because your example (#33 (comment)) isn't working for me either.
image

I followed everything mentioned here (https://github.com/csense-oss/csense-kotlin-annotations?tab=readme-ov-file#installation) (just changed the version to 0.0.63), but it still doesn't work for me.

@Tvede-dk
Copy link
Collaborator

@GeorgeVoloshin Ah, so that is because I have not suppoted type annotations in the plugin.
the "working" example is where the annotation is on the parameter itself. So if you move the annotation it should work :)

image

@GeorgeVoloshin
Copy link

GeorgeVoloshin commented Sep 16, 2024

Oh, sorry, my mistake.
I apologize for my carelessness. If I move the annotation, everything starts working in this example.
Thank you very much.

But I would like to return to my original question.
Could you please tell me if there's a way to apply an annotation so that the exceptions thrown in launch, async, and coroutineScope blocks can also be specified in @Throws, and they stop being highlighted as warnings?
Because I use launch, async, and coroutineScope a lot in my code to run asynchronous code, and it's inconvenient when a third of the project is highlighted in yellow because of this.
For example:
image

I specified @Throws(TestException::class), but the error in launch is still highlighted as warning.

@Tvede-dk
Copy link
Collaborator

@GeorgeVoloshin No need to apologize. Not only was I expecting your example to work, but its also not easy to understand the difference (and the real challenge is to update the plugin, which is on me :) )

Now with regards to launch / async ect. The real problem is the flow of exception is NOT linear.
that means, for that to work, the plugin would have to track all scopes, effectively doing a very deep analysis of coroutine scopes. which not to mention, would still require special care for say SupervisorJob (which consumes uncaught exceptions and does not terminate the parent job ...)
This means, that its very hard / unlikely to just be supported.
In your example, its only because of the function "coroutineScope" that it "appears linear".
Take this expanded example
image

it prints
image
(since we do not wait on the launch etc, thus the exception is not throw outright, if there is a "z.join"; then it does throw...)
which is due to the "way" jobs / coroutines handles exceptions.

Now this does highlight a potential consideration: when dealing with coroutines there might be a few constructs that would be easy to recognize (as seen from this talk from kotlin conf https://www.youtube.com/watch?v=a3agLJQ6vt8) the end result is very often (also in my experience) a launch / async on a parent scope, usually on a SupervisorJob. That however does not stop the plugin from potentially highlighting "join / await" locations. (although, you cannot annotate a job / Deferred with throws ... )
thus hypothetically this would be possible to do something like this
image

and yes, coroutineScope would then also need to be handled specially. And it might have to be a bit "eager", and not verify specifically that you are calling say "launch" on the right scope, as that again would be a tracking nightmare for the plugin. Thus detecting that Globalscope (or any other scope) is not in the "containing" scope is very likely to much for now, eg:
image

It will also be quite difficult dealing with custom functions etc, since await & join would be special "functions" that the plugin will look for. although, with more annotations, it would be a simple "tracking" like all the other ones...

I will try and come up with a rough sketch / POC of this, as it is a good idea, and definitely a thing that the plugin should know about :)

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

No branches or pull requests

3 participants