Skip to content

Conversation

@cyangle
Copy link
Contributor

@cyangle cyangle commented Nov 6, 2025

Fixes #15489

Root Cause

The bug was caused by an incorrect variable lookup order in the Crystal interpreter. When a block argument, like parser, was used in a nested block, the compiler correctly identified it as a closure variable. However, the interpreter's lookup_local_var_or_closured_var method would search for a local variable in all scopes (from current to global) before searching the closure scope. This caused it to find the global parser variable first, leading to a type error, instead of finding the correct parser block argument from the closure.

The Fix

To resolve this, the variable lookup logic was reordered to correctly prioritize scopes. The lookup_local_var_or_closured_var method was modified to search in this order:

  • The current block's local scope.
  • The closure scope.
  • Parent-level local scopes (from inner to outer).

This ensures that block arguments and other local variables are found before any variables from outer or global scopes, correctly implementing variable shadowing and fixing the bug.

@HertzDevil Could you have a look at this?

@cyangle cyangle marked this pull request as ready for review November 7, 2025 02:16
@ysbaddaden
Copy link
Contributor

We don't need to search in a range. We need to:

  1. search from @block_level and down;
  2. search from @block_level - 1 and down;
  3. search at @block_level only.

Which can be simplified into a couple method: lookup from and lookup at, with the from variant calling the at variant.

For example:

diff --git a/src/compiler/crystal/interpreter/compiler.cr b/src/compiler/crystal/interpreter/compiler.cr
index c40aa4ded..5e0521ce5 100644
--- a/src/compiler/crystal/interpreter/compiler.cr
+++ b/src/compiler/crystal/interpreter/compiler.cr
@@ -791,8 +791,9 @@ class Crystal::Repl::Compiler < Crystal::Visitor
  end

  def lookup_local_var_or_closured_var(name : String) : LocalVar | ClosuredVar
-    lookup_local_var?(name) ||
+    lookup_local_var?(name, at: @block_level) ||
      lookup_closured_var?(name) ||
+      lookup_local_var?(name, from: @block_level - 1) ||
      raise("BUG: can't find closured var or local var #{name}")
  end

@@ -800,19 +801,18 @@ class Crystal::Repl::Compiler < Crystal::Visitor
    lookup_local_var?(name) || raise("BUG: can't find local var #{name}")
  end

-  def lookup_local_var?(name : String) : LocalVar?
-    block_level = @block_level
-    while block_level >= 0
-      index = @local_vars.name_to_index?(name, block_level)
-      if index
-        type = @local_vars.type(name, block_level)
-        return LocalVar.new(index, type)
+  def lookup_local_var?(name : String, *, from = @block_level) : LocalVar?
+    from.downto(0) do |block_level|
+      if local_var = lookup_local_var?(name, at: block_level)
+        return local_var
      end
-
-      block_level -= 1
    end
+  end

-    nil
+  def lookup_local_var?(name : String, *, at block_level : Int32) : LocalVar?
+    if index = @local_vars.name_to_index?(name, block_level)
+      LocalVar.new(index, @local_vars.type(name, block_level))
+    end
  end

  def lookup_closured_var(name : String) : ClosuredVar

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:interpreter labels Nov 15, 2025
@straight-shoota straight-shoota added this to the 1.19.0 milestone Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:interpreter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: missing downcast_distinct from String to OptionParser (Crystal::NonGenericClassType to Crystal::NonGenericClassType)

3 participants