Semantic: disallow to use return/break/next inside ensure #4486
Conversation
makenowjust
force-pushed
the
fix/crystal/disallow-control-in-ensure
branch
from
dea26d8 to
e089b86
Compare
| @@ -80,6 +80,7 @@ module Crystal | |||
| @block_context : Block? | |||
| @file_module : FileModule? | |||
| @while_vars : MetaVars? | |||
| @while_ensure_stack : Array(Symbol) | |||
There was a problem hiding this comment.
I'm not fully convinced about the name.
Maybe should be block_kind_stack or something like that. Since it does not only trace while and ensure.
But, check the next comment, I think we don't even need an Array here.
There was a problem hiding this comment.
I think so too. Thank you for suggestion good name block_kind_stack!
| @@ -1026,7 +1027,9 @@ module Crystal | |||
| block_visitor.path_lookup = path_lookup || current_type | |||
| block_visitor.block_nest = @block_nest | |||
|
|
|||
| @while_ensure_stack.push :block | |||
There was a problem hiding this comment.
In Crystal::CodeGenVisitor::Context there is a pattern that might be useful here in #with_context the current value is kept in a local variable and then restored. That way we might don't event need an Array and leave the execution stack trace the push/pops in variables across the stack. There will be a need for a single ivar with the top of the stack for sure.
def with_context(new_block_kind)
old_block_kind = @block_kind
@block_kind = new_block_kind
value = yield
@block_kind = old_block_kind
value
end
def ...
with_block_kind :block do
node.body.accept block_visitor
end
endThere was a problem hiding this comment.
with_block utility method looks good, however it needs to use Array at least in this implementation. See line 1624 for example. Not only stack top but also all stack items are important.
Not use Array for stack and add utility method `with_block_kind`, by crystal-lang#4486 (comment). Correct `return` statement error message to fit other `return` statement messages.
|
@bcardiff Updated pull request. I removed |
| @@ -1618,6 +1641,10 @@ module Crystal | |||
| end | |||
|
|
|||
| def visit(node : Return) | |||
| if inside_ensure? | |||
| node.raise "can't return from ensure (see https://github.com/crystal-lang/crystal/issues/4470)" | |||
There was a problem hiding this comment.
Please don't include links to github issues in compiler error messages.
(same goes for all errors below this)
|
@asterite ping |
|
@makenowjust We can probably rebase and merge :-) |
Not use Array for stack and add utility method `with_block_kind`, by crystal-lang#4486 (comment). Correct `return` statement error message to fit other `return` statement messages.
makenowjust
force-pushed
the
fix/crystal/disallow-control-in-ensure
branch
from
fdc82ba to
700904f
Compare
|
@asterite rebased |
|
@makenowjust I don't see the commits rebased in this PR yet |
|
@bcardiff The commits are rebased to |
SSIA
Related issue: #4470