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

Compiler: fix closured instance variable not being considered as such #8588

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions spec/compiler/semantic/closure_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -524,4 +524,79 @@ describe "Semantic: closure" do
),
"can't send closure to C function (closured vars: x)"
end

it "correctly detects previous var as closured (#5609)" do
assert_error %(
def block(&block)
block.call
end

def times
yield
yield
end

x = 1
times do
if x.is_a?(Int32)
x &+ 2
end
block do
x = "hello"
end
end
),
"undefined method '&+' for String"
end

it "doesn't assign all types to metavar if closured but only assigned to once" do
semantic(%(
def capture(&block)
block
end

x = 1 == 2 ? 1 : nil
if x
capture do
x &+ 1
end
end
))
end

it "does assign all types to metavar if closured but only assigned to once in a loop" do
assert_error %(
def capture(&block)
block
end

while 1 == 1
x = 1 == 2 ? 1 : nil
if x
capture do
x &+ 1
end
end
end
),
"undefined method '&+'"
end

it "considered as closure if assigned once but comes from a method arg" do
assert_error %(
def capture(&block)
block
end

def foo(x)
capture do
x &+ 1
end
x = 1 == 2 ? 1 : nil
end

foo(1)
),
"undefined method '&+'"
end
end
35 changes: 33 additions & 2 deletions src/compiler/crystal/semantic/ast.cr
Original file line number Diff line number Diff line change
Expand Up @@ -447,12 +447,42 @@ module Crystal
# where it wasn't created.
property? closured = false

# Is this metavar assigned a value?
# Is this var used inside a while loop?
property? inside_loop = false

# Was something assigned to this metavar?
property? assigned_to = false

# Was something assigned to this metavar multiple times?
property? assigned_to_multiple_times = false

# Local variables associated with this meta variable.
# Can be Var or MetaVar.
property(local_vars) { [] of ASTNode }

def initialize(@name : String, @type : Type? = nil)
end

# Incrase the times something was assigned to this metavar.
def increase_assigned_count
if assigned_to?
@assigned_to_multiple_times = true
else
@assigned_to = true
end
end

# Is this metavar associated with any local vars?
def local_vars?
@local_vars
end

# Is this var closured and also all local vars related to it
# should be bound to it?
def closured_multibound?
closured? && (assigned_to_multiple_times? || inside_loop?)
end

# True if this variable belongs to the given context
# but must be allocated in a closure.
def closure_in?(context)
Expand Down Expand Up @@ -480,7 +510,8 @@ module Crystal
end
io << " (nil-if-read)" if nil_if_read?
io << " (closured)" if closured?
io << " (assigned-to)" if assigned_to?
io << " (assign-to)" if assigned_to?
io << " (assign-to-multiple-times)" if assigned_to_multiple_times?
io << " (object id: #{object_id})"
end

Expand Down
8 changes: 8 additions & 0 deletions src/compiler/crystal/semantic/bindings.cr
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,14 @@ module Crystal
nodes.each &.remove_observer self
end

def bound_to?(node : ASTNode)
@dependencies.try &.any? &.same?(node)
end

def bind_to_unless_bound(node : ASTNode)
bind_to(node) unless bound_to?(node)
end

def add_observer(observer)
observers = @observers ||= [] of ASTNode
observers.push observer
Expand Down
80 changes: 54 additions & 26 deletions src/compiler/crystal/semantic/main_visitor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ module Crystal
vars.each do |name, var|
meta_var = new_meta_var(name)
meta_var.bind_to(var)
meta_var.increase_assigned_count
meta_vars[name] = meta_var
end
end
Expand Down Expand Up @@ -350,6 +351,7 @@ module Crystal

meta_var = @meta_vars[node.name]
check_closured meta_var
check_closure_multibound meta_var, var

if var.nil_if_read?
# Once we know a variable is nil if read we mark it as nilable
Expand All @@ -360,10 +362,6 @@ module Crystal
node.bind_to(@program.nil_var)
end

if meta_var.closured?
var.bind_to(meta_var)
end

node.bind_to(var)

if needs_type_filters?
Expand Down Expand Up @@ -796,9 +794,6 @@ module Crystal
target.raise ex.message
end

meta_var.assigned_to = true
check_closured meta_var

simple_var = MetaVar.new(var_name)

# When we assign to a local variable with a fixed type, and it's
Expand All @@ -808,9 +803,9 @@ module Crystal
else
simple_var.bind_to(target)

if meta_var.closured?
simple_var.bind_to(meta_var)
end
meta_var.increase_assigned_count
check_closured meta_var
check_closure_multibound(meta_var, simple_var)
end

@vars[var_name] = simple_var
Expand Down Expand Up @@ -878,7 +873,7 @@ module Crystal
unless scope.has_instance_var_initializer?(var_name)
meta_var = (@meta_vars[var_name] ||= new_meta_var(var_name))
meta_var.bind_to value
meta_var.assigned_to = true
meta_var.increase_assigned_count

simple_var = MetaVar.new(var_name)
simple_var.bind_to(target)
Expand Down Expand Up @@ -2544,7 +2539,7 @@ module Crystal
case exp = node.exp
when Var
meta_var = @meta_vars[exp.name]
meta_var.assigned_to = true
meta_var.increase_assigned_count
meta_var
when InstanceVar
lookup_instance_var exp
Expand Down Expand Up @@ -2700,8 +2695,9 @@ module Crystal
var = @vars[node_name] = new_meta_var(node_name)
meta_var = (@meta_vars[node_name] ||= new_meta_var(node_name))
check_closured(meta_var)
check_closure_multibound(meta_var, var)
meta_var.bind_to(var)
meta_var.assigned_to = true
meta_var.increase_assigned_count

if types
unified_type = @program.type_merge(types).not_nil!
Expand Down Expand Up @@ -3170,6 +3166,10 @@ module Crystal
end

def check_closured(var)
unless @while_stack.empty?
var.inside_loop = true
end

return if @typeof_nest > 0

if var.name == "self"
Expand All @@ -3179,7 +3179,12 @@ module Crystal

context = current_context
var_context = var.context
if !var_context.same?(context)
if var_context.same?(context)
var_context = var_context.context if var_context.is_a?(Block)
if var.closured?
mark_as_closured(var, var_context)
end
else
# If the contexts are not the same, it might be that we are in a block
# inside a method, or a block inside another block. We don't want
# those cases to closure a variable. So if any context is a block
Expand All @@ -3190,20 +3195,32 @@ module Crystal

closured = !context.same?(var_context)
if closured
var.closured = true
mark_as_closured(var, var_context)
end
end
end

# Go up and mark proc literal defs as closured until we get
# to the context where the variable is defined
visitor = self
while visitor
visitor_context = visitor.closure_context
break if visitor_context == var_context
def mark_as_closured(var, var_context)
var.closured = true

visitor_context.closure = true if visitor_context.is_a?(Def)
visitor = visitor.parent
end
if var.closured_multibound?
# Bind all local vars related to the metavar and bind them
# to the metavar. This is a fix for #5609.
var.local_vars?.try &.each do |local_var|
local_var.bind_to_unless_bound var
end
end

# Go up and mark proc literal defs as closured until we get
# to the context where the variable is defined
visitor = self
while visitor
visitor_context = visitor.closure_context
break if visitor_context == var_context

visitor_context.closure = true if visitor_context.is_a?(Def)
visitor = visitor.parent
end
end

def check_self_closured
Expand Down Expand Up @@ -3335,8 +3352,8 @@ module Crystal
meta_var = (@meta_vars[name] ||= new_meta_var(name))
meta_var.bind_to value
meta_var.bind_to program.nil_var unless meta_var.dependencies.any? &.same?(program.nil_var)
meta_var.assigned_to = true
check_closured meta_var
meta_var.increase_assigned_count
check_closured(meta_var)

@vars[name] = meta_var
meta_var
Expand Down Expand Up @@ -3412,6 +3429,17 @@ module Crystal
nil_exp
end

# If the metavar is a closure multibound then bind var
# to it. Otherwise add it to the local vars so that they could
# be bound later on.
def check_closure_multibound(meta_var, var)
if meta_var.closured_multibound?
var.bind_to_unless_bound(meta_var)
else
meta_var.local_vars << var
end
end

def visit(node : When | Unless | Until | MacroLiteral | OpAssign)
raise "BUG: #{node.class_desc} node '#{node}' (#{node.location}) should have been eliminated in normalize"
end
Expand Down