Compiler: fix incorrect var type insisde nested exception handler #11926
Conversation
|
Well, this was not right... |
|
This was actually good because I'm starting to understand the problem... but it'll take me longer to fix this. |
|
What happened with CI here? 😕 |
HertzDevil
added
kind:bug
|
GitHub actions seems to have a service disruption. |
| a = 1.5 | ||
| begin | ||
| a = 2 | ||
| a = 'a' | ||
| a = "hello" | ||
| rescue ex | ||
| a = false | ||
| end | ||
| a | ||
| )) { union_of [float64, int32, char, string, bool] of Type } |
There was a problem hiding this comment.
This looks a bit odd. I realize this spec is not new. But could the type be more restricted?
After the begin/end it can really only be String (last type from begin branch) or Bool (last type from rescue branch).
Just mentioning this. It's probably a separate issue.
There was a problem hiding this comment.
The compiler doesn't check what's actually happening in the begin body. All it does is to assume that if a variable is assigned a value inside it, well, it might never be assigned a value at all because an exception could be thrown in the middle. And that exception could also be thrown in-between assignments. I know that a simple value like 2 or "hello" can't raise, but the compiler doesn't take that into account. It's an improvement for sure. But I think this assumption is actually simpler. If you don't want that to happen, extract the variables outside of the exception handler: problem solved, and cleaner code.
There was a problem hiding this comment.
Actually this is quite tricky to change. For example:
a = 2
raise "OH NO!" if something
a = 'a'So it's of course not just about them being simple assignments. It's about knowing that an exception could be thrown somewhere in the middle. It's a pretty complex change, and I'm not actually sure it's either doable or worth doing. Nobody complained about this so far, so I think we shouldn't change this.
| property exception_handler_vars : MetaVars? = nil | ||
| property all_exception_handler_vars : Array(MetaVars)? = nil |
There was a problem hiding this comment.
To explain the change with the minimum amount of words: instead of tracking a single set of variables to track on exception handlers, we need a list of them to take into account nested contexts.
|
Replaced by #11928 |
|
I suppose you might've triggered CI by just adding an empty commit to the branch. But new PR is fine. |
Fixes #9769