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

Ruby: fix bug with captured variable reads in lambdas #8517

Merged
merged 2 commits into from Mar 23, 2022

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Mar 22, 2022

In the following example, we currently identify the read of x as a method call:

x = 1
f = ->(y) { x }

This change fixes that, so we instead identify it as a captured variable read.

Evaluation shows no significant performance impact, and otherwise no changes that I can see.

@github-actions github-actions bot added the Ruby label Mar 22, 2022
hmac added 2 commits Mar 22, 2022
This reveals a bug where we identify reads of captured variables in
lambdas as method calls. This is fixed in a followup commit.
These were previously identified as method calls. The fix is to
recognise lambdas as a scope which can inherit variables from its
parent.
@hmac hmac force-pushed the hmac/lambda-captured-var branch from 60c896a to 99b5c58 Compare Mar 22, 2022
@hmac hmac marked this pull request as ready for review Mar 23, 2022
@hmac hmac requested a review from as a code owner Mar 23, 2022
@hmac hmac added the no-change-note-required label Mar 23, 2022
Copy link
Contributor

@aibaars aibaars left a comment

Looks good to me!

Copy link
Contributor

@hvitved hvitved left a comment

LGTM, thanks for fixing.

# calls inside lambdas
y = 1
id = ->(x) { y }
Copy link
Contributor

@hvitved hvitved Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would call it e.g. one, id makes me expect that it is the identify function.

Copy link
Contributor Author

@hmac hmac Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh true, haha. I think I initially wrote the identity function and then changed it to be relevant to this fix...

Copy link
Contributor Author

@hmac hmac Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this in a followup PR, so you don't have to re-approve this one.

Copy link
Contributor Author

@hmac hmac Mar 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hmac hmac merged commit 3b4206c into github:main Mar 23, 2022
32 of 33 checks passed
@hmac hmac deleted the hmac/lambda-captured-var branch Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required Ruby
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants