Skip to content

Kernel#lambda: return forwarded block as non-lambda proc #2289

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

Merged
merged 3 commits into from
Dec 21, 2019

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented Jul 14, 2019

Before this commit, Kernel#lambda can't tell the difference between a
directly passed literal block and one passed with an ampersand.

A block passed with an ampersand is semantically speaking already a
non-lambda proc. When Kernel#lambda receives a non-lambda proc, it
should simply return it.

Implementation wise, when the VM calls a method with a literal block, it
places the code for the block on the calling control frame and passes a
pointer (block handler) to the callee. Before this commit, the VM
forwards block arguments by simply forwarding the block handler, which
leaves the slot for block code unused when a control frame forwards its
block argument. I use the vacant space to indicate that a frame has
forwarded its block argument and look for this in Kernel#lambda to
detect forwarded blocks.

This is a very ad-hoc solution and relies heavily on the way block
passing works in the VM. However, it's the most self-contained solution
I have.

Bug #15620

@XrXr XrXr marked this pull request as ready for review July 14, 2019 17:17
@XrXr XrXr force-pushed the two-four-lambda branch from 3be5a65 to 41f1479 Compare July 14, 2019 17:21
@XrXr XrXr changed the title Kernel#lambda: return forwarded block as non-lambda proc [WIP] Kernel#lambda: return forwarded block as non-lambda proc Jul 14, 2019
@XrXr XrXr force-pushed the two-four-lambda branch from 41f1479 to 74e066b Compare July 14, 2019 23:35
@XrXr
Copy link
Member Author

XrXr commented Jul 14, 2019

It looks like this changes the semantics of calling into lambda using zsuper. Will look for solutions

EDIT: Okay, found a way to do it.

@XrXr XrXr force-pushed the two-four-lambda branch from 74e066b to 1c74774 Compare July 15, 2019 21:43
@XrXr XrXr changed the title [WIP] Kernel#lambda: return forwarded block as non-lambda proc Kernel#lambda: return forwarded block as non-lambda proc Jul 15, 2019
@XrXr
Copy link
Member Author

XrXr commented Jul 15, 2019

The extra specs are just for making sure I didn't make unintentional semantic changes. I checked that they pass on supported versions and on master.

Some notes:

  • it's not enough to simply check whether lambda got a forwarded block since it needs to behave differently for block forwarded using & vs super
  • another possible solution is to make block handler an offset from the bottom of the stack plus a tag, packed into a VALUE. It would be a bigger change that might have wider perf consequences
  • I tried using another tag for the block handler (it's already tagged), but there aren't enough bits for another tag on 32 bit platforms Make Kernel#lambda return non-lambda when block is not literal #2288

k0kubun added a commit to XrXr/ruby that referenced this pull request Aug 15, 2019
@XrXr XrXr force-pushed the two-four-lambda branch 2 times, most recently from 895dadd to d5e8b25 Compare September 28, 2019 03:35
@XrXr
Copy link
Member Author

XrXr commented Sep 28, 2019

Updated to issue a warning in cases where Kernel#lambda (unintuitively) returns the proc it receives.

@XrXr XrXr force-pushed the two-four-lambda branch 2 times, most recently from 61d0caf to acb3667 Compare November 13, 2019 22:55
@eregon
Copy link
Member

eregon commented Nov 27, 2019

👍 for the warning.

I'm not sure about the super cases, it seems extremely uncommon to have a method named lambda and call super in it. But it's probably best to keep behavior as it-is in this PR.

@ko1
Copy link
Contributor

ko1 commented Dec 19, 2019

I'm not sure we can show warning.
Other than that, it looks good.

Personally, I hesitate to introduce complexity only for lambda(&b) (`b is lazy proc) ( I agree the current behavior is a bug).

@ko1
Copy link
Contributor

ko1 commented Dec 19, 2019

BTW, without this patch, I found that we can eliminate to initialize cfp->block_code at vm_push_frame() which are called frequently because nobody access it :p

@ko1
Copy link
Contributor

ko1 commented Dec 20, 2019

@XrXr could you merge it without warning?

For warning, let's discuss for Ruby 3. (do we have a ticket?)

@eregon
Copy link
Member

eregon commented Dec 20, 2019

@ko1 The original ticket is https://bugs.ruby-lang.org/issues/15620.
Also related is https://bugs.ruby-lang.org/issues/15973#note-27.
There matz said ... I think we should warn this inconsistent behavior..
I agree, I think we should warn here and it is the general consensus on that issue.

@ko1
Copy link
Contributor

ko1 commented Dec 20, 2019

I agree that warning is one strong option. However, I couldn't get approval to introduce warning at this timing (just before Ruby 2.7) from Matz. Furthermore, please ask matz.

Before this commit, Kernel#lambda can't tell the difference between a
directly passed literal block and one passed with an ampersand.

A block passed with an ampersand is semantically speaking already a
non-lambda proc. When Kernel#lambda receives a non-lambda proc, it
should simply return it.

Implementation wise, when the VM calls a method with a literal block, it
places the code for the block on the calling control frame and passes a
pointer (block handler) to the callee. Before this commit, the VM
forwards block arguments by simply forwarding the block handler, which
leaves the slot for block code unused when a control frame forwards its
block argument. I use the vacant space to indicate that a frame has
forwarded its block argument and inspect that in Kernel#lambda to detect
forwarded blocks.

This is a very ad-hoc solution and relies *heavily* on the way block
passing works in the VM. However, it's the most self-contained solution
I have.

[Bug #15620]
@XrXr XrXr merged commit 85a337f into ruby:master Dec 21, 2019
@XrXr XrXr deleted the two-four-lambda branch December 21, 2019 14:08
@eregon
Copy link
Member

eregon commented Dec 21, 2019

I agree that warning is one strong option. However, I couldn't get approval to introduce warning at this timing (just before Ruby 2.7) from Matz. Furthermore, please ask matz.

OK, I asked matz in https://bugs.ruby-lang.org/issues/15973
It would probably be useful to add it the next dev-meeting after the 2.7 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants