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.

Sign up for GitHub

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

Jump to bottom

Rework framing for super and __callee__ #7098

Open
wants to merge 7 commits into
base: jruby-9.3
Choose a base branch
from
Open

Rework framing for super and __callee__ #7098

wants to merge 7 commits into from

Conversation

headius
Copy link
Member

@headius headius commented Feb 14, 2022

This is a series of commits to make super less dynamic (or exactly as dynamic as it needs to be) in order for us to support the __callee__ method which does need to be dynamic.

@headius
Use static name for super in a module method
a151aad 
Module methods have a dynamic hierarchy, but do not have a dynamic
super target name. This eliminates part of the frame requirement
for super within a module method, on the way to getting __callee__
working.
@headius
Always use ModuleSuper and rewrite define_method
cb27869 
The changes here attempt to get more super calls using a static
name, so we can eliminate dependency on a call frame to get that
name.

* super in a block in any method will start out as a ModuleSuper
  using the name from the method.
* define_method clones and rewrites the block to retarget any
  super calls to the newly defined name.
* all super instructions get rewritten to the appropriate type for
  the target class, instance/class/module

Remaining cases that use UnresolvedSuper may only be the ones
where super is invalid, such as a block at top-level or within a
class or module body.
enebo and others added 5 commits February 17, 2022 09:29
@enebo
Add to existing instr list. unresolved swapped two operands in builder
34b33a2
@enebo
Only do work of replacing super if it contains super
fd50860
@enebo
work around problem in picking right super instr
eed5a35
@enebo
Use startup instead of full for super rewriting
31465a3 
startup is simpler time to do this and also the
define_method opto will not work if the AST is consumed
which is what happens if you full compile.
@headius
Additional tweaks for define_methods super
7e71845 
* Alter scope class to be the target class, so it will be used as
  the base class when doing InstanceSuper. This fixes the last
  remaining failure in specs.
* Also consider and rewrite super in nested closures. This fixes
  the last remaining failure in CRuby tests.
@rdubya
Copy link

rdubya commented Mar 2, 2023

I think fixing __callee__ would fix this failure from the jdbc adapter tests:

  1) Failure:
ExcludingTest#test_raises_on_record_from_different_class [~/rails/activerecord/test/cases/excluding_test.rb:53]:
--- expected
+++ actual
@@ -1 +1 @@
-"You must only pass a single or collection of Post objects to #without."
+"You must only pass a single or collection of Post objects to #excluding."

@headius
Copy link
Member Author

headius commented Mar 2, 2023

Yeah we've been kicking around a shorter-term option that would allow us to fix __callee__ without a major overhaul of framing. For now we are working on getting 9.4.2 out but we'll circle back to this soon.

@headius
Copy link
Member Author

headius commented Aug 14, 2023

Following up: we did get __callee__ functional in #7702 by encoding it and the real method name on the stack. There may be a future change that adds a separate stack entry for the callee name, but what we have works without any drastic changes.

cc @rdubya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@headius @rdubya @enebo