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

Incorrect behaviour when capturing var and changing it in a loop #5609

Closed
asterite opened this issue Jan 19, 2018 · 4 comments · Fixed by #9986
Closed

Incorrect behaviour when capturing var and changing it in a loop #5609

asterite opened this issue Jan 19, 2018 · 4 comments · Fixed by #9986
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic topic:compiler

Comments

@asterite
Copy link
Member

Crystal 0.24.1

def capture(&block)
  block
end

x = 1
3.times do
  p x
  capture do
    x = "hello"
  end.call
end

Output:

1
194613232
194613232

The problem is that we only bind the variable to the metavariable if it's a closured var on a first pass, but the first read of x happens before we determine it's a closure.

I found this while thinking about how to fix #4359

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler topic:compiler:semantic labels Jan 19, 2018
@RX14 RX14 reopened this May 28, 2018
@SlayerShadow
Copy link

SlayerShadow commented Apr 14, 2019

It works correctly with

def capture
  yield
end

And doesn't work with block.call...
Crystal version: 0.28.0-dev [d1cdab2]

@straight-shoota
Copy link
Member

straight-shoota commented Apr 14, 2019

@SlayerShadow Yes, that's to be expected because yield essentially inlines the block directly. With capturing + block.call it's indirected through a Proc and that seems to have a bug somewhere, probably in code generation.

@asterite
Copy link
Member Author

The problem is actually in the semantic phase.

@miketheman
Copy link
Contributor

#CodeTriage
The behavior today emits the following output:

1
-1334769312
-1334769312

So it's still incorrect. The referenced linked PR #8588 appears to try and get this working, however that has yet to be merged, and there's further discussion that can be followed over there.

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:semantic topic:compiler
Projects
None yet
5 participants