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

Highlight not always present #15

Closed
cap-dlisbona opened this issue Aug 10, 2023 · 5 comments
Closed

Highlight not always present #15

cap-dlisbona opened this issue Aug 10, 2023 · 5 comments

Comments

@cap-dlisbona
Copy link

cap-dlisbona commented Aug 10, 2023

Hello and thanks a lot for your work,

The highlight seems to work for the "throw" key word but not for the use of the function itself.

  • Highlight shown :
    image

  • Highlight I would expect to have :
    image

  • Settings :
    image

Currently using Android studio Giraffe with 2.0.0 Csense plugin and don't know what I am missing : /

Regards,

David

@Tvede-dk
Copy link
Collaborator

Hi,
Im just happy to help :)
So if the method "exception thrower" is not marked with throws, then the plugin assumes that it does not throw.
if you added the throws it would of cause highlight it (as the plugin is trying to help you do, by highligting "throw Exception()" .

Eg.
image

Then adding the throws annotation "propergates" the exception handling. think of it just like java, where the responsible code is the code "seeing" the exception that is not "expected" to throw (eg the throws annotation marks which function is expected to throw exceptions and for which ones its unexpected).

image

If you wanted the plugin to "ignore" these boundries, and show ALL potential exceptions, it would have to do both an expensive analysis of the entire codebase (which would tank performance), but also (even worse) existing code bases would very likely ALL be highlighted if there is any use of exceptions (as it would backtrack thoughout the whole code).
Eg, the use of kotlins "first()" extension on lists throws (if the list is empty), and I imagine most people do not look into all the various exception the kotlin std lib actually throws (quite a lot actually, but as they documented its "exceptions", not for "regular code flow")

@cap-dlisbona
Copy link
Author

Thanks a lot for these really clear explanations and precisions.
The first() is indeed one of the method I would like to be taken into account.
I understand the cost and the limitation but maybe a N-1 or N-2 check would be enough to cover most of the cases without being too expensive ?

@Tvede-dk
Copy link
Collaborator

My plan as of now is to hard code the kotlin std lib (for very obvious reasons the kotlin team will not annotate each method with throws which from their design stand point makes total sense) functions that does throw, and potentially add a setting to highlight them / not (since that might be against the "design" of kotlin). (it may require a tool since the kotlin std lib is quite huge... and its ever changing) :)

So if I did that rather than starting to highlight calling methods which can have an exception at say N depth, would that be sufficient?
(Its a "bit" more complicated than it might seem, e.g., when the plugin see the "first" method its sometimes complied code, and thus there is no chance that it can determine that it throws, unless its annotated, which may explain why this is so cumbersome )
Or is the request "just" to highlight every function that could "have" an exception happen? :) (if so, is it just as a "can any exception happen here" type of question, because then a special inspection might make more sense that can manually be invoked via code inspections say)?

@cap-dlisbona
Copy link
Author

Hello and thanks again for the detailed answer,

Not sure if I understood correctly the third and last point but for the second point I get the tricky part of an already compiled called function that throws.

I did not think of this cumbersome case. I was "naively" expecting smt that inspects the callee and search for a "throw" keyword inside it.

For the first point , indeed that looks like colossal and temporary job.

@Tvede-dk
Copy link
Collaborator

The code for just searching for a throws is "easy", its all the various details that is hard.
So in your original example, you expected (as I read it) that accessing / using any function that can throw (even if not annotated) should be marked (if not handled ofc). The issue with an "n depth" is performance. However Idea does have the abillity to make inspections that are not ran on the fly, but rather through the "inspect code" menu. eg.
image
then I could instead add another inspection that does this "full" depth analysis and showcases all potential uncaught exceptions.
image
like your example? :)

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

2 participants