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

Further try/catch/finally improvements #7689

Open
AndrolGenhald opened this issue Feb 17, 2022 · 3 comments
Open

Further try/catch/finally improvements #7689

AndrolGenhald opened this issue Feb 17, 2022 · 3 comments
Labels
control flow enhancement hard problems Problems without an obvious easy solution

Comments

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Feb 17, 2022

Thoughts on further improvements after #7688 is merged.

Right now, TryAnalyzer assumes that a Throwable could be thrown at any point in the try block, and no uncaught Throwable will ever be thrown in a catch block. A better option would be to keep track of possibly throwing statements and track the possibilities for each variable before each possibly throwing statement.

TryCatchScope would have to be changed so that instead of tracking all assignments in the scope, it should clone all variable states before each possibly throwing statement, then TryAnalyzer could use that data to union the possible types instead of using the assignment data.

The main difficulty here is figuring out which statements can throw. We can use @throws, but that's potentially dangerous if one is missing. checkForThrowsDocblock helps for user code but not for dependencies, so maybe we'd just want to assume all non-internal functions can throw Throwable? Maybe we should add a config option to toggle that behavior for user code and/or library code? We would also have to track down every type of statement that can cause an Error, like 1 / 0.

#2912 could make this much less dangerous. If we assume any function without a @throws annotation can throw Throwable, this might actually work fairly well. I think for it to be completely safe we'd also have to show a warning when calling a function without any @throws or @psalm-never-throw inside a function with @throws SomeException, since it might throw a Throwable.

@AndrolGenhald AndrolGenhald added enhancement hard problems Problems without an obvious easy solution control flow labels Feb 17, 2022
@psalm-github-bot
Copy link

Hey @AndrolGenhald, can you reproduce the issue on https://psalm.dev ?

@orklah
Copy link
Collaborator

orklah commented Feb 20, 2022

Yeah, that seems really hard to do...

The behaviour right now is dumb, but it can be understood by user easily. Trying to make it a super smart behaviour that users won't understand may not be a good move either...

@AndrolGenhald
Copy link
Collaborator Author

Trying to make it a super smart behaviour that users won't understand may not be a good move either...

That's something to consider as well. At the very least, the smarter we try to make it, the surer we have to be that it's actually accurate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
control flow enhancement hard problems Problems without an obvious easy solution
Projects
None yet
Development

No branches or pull requests

2 participants