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

Failing tests on cancancan #2418

Closed
coorasse opened this issue Aug 2, 2021 · 8 comments
Closed

Failing tests on cancancan #2418

coorasse opened this issue Aug 2, 2021 · 8 comments
Assignees
@eregon
Labels
bug
Milestone
21.3.0

Comments

@coorasse
Copy link

coorasse commented Aug 2, 2021

Hi @eregon , there seems to be an issue in the latest cancancan test suite running on truffleruby: https://github.com/CanCanCommunity/cancancan/runs/3220131164?check_suite_focus=true

No big information in the log, just to report it here.
@eregon
Copy link
Member

eregon commented Aug 3, 2021

Thanks for the report, it seems there is a break escaping:

<no message> (org.truffleruby.language.control.BreakException)

@coorasse
Copy link
Author

coorasse commented Aug 3, 2021

I am not sure what this means 😅

@eregon
Copy link
Member

eregon commented Aug 3, 2021

We'll investigate, probably some unusual use of break.

@coorasse
Copy link
Author

coorasse commented Aug 3, 2021

this is the only break we have right now: https://github.com/CanCanCommunity/cancancan/blob/4254891b8b29a325c7fc9ff5a8d1e31db687a963/lib/cancan/matchers.rb#L16

@eregon
Copy link
Member

eregon commented Aug 3, 2021

Changing BreakException to extends RuntimeException we get a stacktrace:

<no message> (org.truffleruby.language.control.BreakException)
	from org.truffleruby.language.control.BreakNode.execute(BreakNode.java:45)
	from org.truffleruby.language.control.IfNode.execute(IfNode.java:39)
	from org.truffleruby.language.RubyNode.doExecuteVoid(RubyNode.java:70)
	from org.truffleruby.language.control.SequenceNode.execute(SequenceNode.java:33)
	from org.truffleruby.language.control.IfElseNode.execute(IfElseNode.java:43)
	from org.truffleruby.language.control.SequenceNode.execute(SequenceNode.java:36)
	from org.truffleruby.core.module.ModuleNodes$DefineMethodNode$CallMethodWithProcBody.execute(ModuleNodes.java:1370)
	from org.truffleruby.language.RubyMethodRootNode.execute(RubyMethodRootNode.java:63)
	from org.graalvm.truffle/com.oracle.truffle.api.impl.DefaultCallTarget.callDirectOrIndirect(DefaultCallTarget.java:85)
/home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/matchers/dsl.rb:135:in `matches?'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/matchers/dsl.rb:135:in `block (2 levels) in match'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-support-3.10.2/lib/rspec/support.rb:118:in `with_failure_notifier'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/matchers/dsl.rb:133:in `matches?'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:92:in `does_not_match?'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:79:in `block in handle_matcher'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:27:in `with_matcher'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/expectations/handler.rb:76:in `handle_matcher'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-expectations-3.10.1/lib/rspec/expectations/expectation_target.rb:78:in `not_to'
	from /home/eregon/code/cancancan/spec/cancan/matchers_spec.rb:49:in `block (3 levels) in <top (required)>'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:262:in `instance_exec'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:262:in `block in run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:508:in `block in with_around_and_singleton_context_hooks'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:465:in `block in with_around_example_hooks'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:486:in `block in run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:624:in `run_around_example_hooks_for'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:486:in `run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:465:in `with_around_example_hooks'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:508:in `with_around_and_singleton_context_hooks'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:259:in `run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:644:in `block in run_examples'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:640:in `map'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:640:in `run_examples'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:606:in `run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `block in run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `map'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `map'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/configuration.rb:2067:in `with_suite_hooks'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:116:in `block in run_specs'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/reporter.rb:74:in `report'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:115:in `run_specs'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:89:in `run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:71:in `run'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:45:in `invoke'
	from /home/eregon/code/cancancan/vendor/bundle/truffleruby/2.7.3.8/gems/rspec-core-3.10.1/exe/rspec:4:in `<main>'

We're missing the first entry in the Ruby stacktrace, but it is exteremly likely to be https://github.com/CanCanCommunity/cancancan/blob/4254891b8b29a325c7fc9ff5a8d1e31db687a963/lib/cancan/matchers.rb#L16
What's tricky here is RSpec does a lot of meta magic so the block passed to match is actually defined as a method.

So

Kernel.const_get(rspec_module)::Matchers.define :be_able_to do |*args|
  match do |ability|
    actions = args.first
    if actions.is_a? Array
      break false if actions.empty?

the break would normally jump out of the match call, i.e., it unwinds until it's outside the call site with the block.
But the problem is that call site is actually not used since the block is used to define a method with define_method.
So probably the define_method method should catch that specific break and somehow get the information of the call site.
Might be part of the proc->lambda semantic conversion done to convert the block to a method.

@eregon
Copy link
Member

eregon commented Aug 3, 2021

Simpler reproducer:

define_method :foo do
  break 42
end

p foo

return 42 would work, and might make more sense since that block is really a method.
But still break should work, similar to how break works in lambdas.

@eregon eregon added the bug label Aug 3, 2021
@eregon
Copy link
Member

eregon commented Aug 3, 2021

It's a regression from the optimization of moving more behavior into RootNode's.
The issue is define_method takes the body node of a RubyLambdaRootNode and put it in a RubyMethodRootNode

truffleruby/src/main/java/org/truffleruby/core/module/ModuleNodes.java

Lines 1332 to 1334 in 77fe339

final RubyNode body = NodeUtil.cloneNode(rootNode.getBody());
final RubyNode newBody = new CallMethodWithProcBody(proc.declarationFrame, body);
final RubyMethodRootNode newRootNode = new RubyMethodRootNode(

But RubyLambdaRootNode does extra things (redo, break, next, the rest is the same) compared to RubyMethodRootNode and that's why we have this issue.
Probably using a RubyLambdaRootNode even for a method makes sense in this case.
Or alternatively we would have a RubyMethodRootNode that just calls that lambda, but that's visible in the backtrace and does not seem what CRuby does.

@eregon eregon self-assigned this Aug 3, 2021
@eregon eregon added this to the 21.3.0 milestone Aug 3, 2021
@eregon
Copy link
Member

eregon commented Aug 3, 2021 •
edited

@coorasse I have a fix now, it's in the merge queue.

I would recommend to change the upstream code (use a natural return value instead of break) to get CI green on TruffleRuby 21.2.0, which cannot be changed: CanCanCommunity/cancancan#724

eregon added a commit to eregon/cancancan that referenced this issue Aug 3, 2021
@eregon
Avoid using break in RSpec match which uses define_method
8de632d 
* As that causes an issue with TruffleRuby 21.2.0: oracle/truffleruby#2418
@eregon eregon mentioned this issue Aug 3, 2021
Merged
wildmaples pushed a commit to Shopify/truffleruby that referenced this issue Sep 1, 2021
@eregon @wildmaples
Use a RubyLambdaRootNode for define_method(name, &block) methods to p…
ed26018 
…reserve lambda semantics

* Fixes oracle#2418
* Remove RubyRootNode#getBody() as it feels like an anti-pattern
  (it discards semantics in the RootNode and instrumentation nodes might be in between).
* Make Symbol#to_proc use a RubyLambdaRootNode for consistency and simplicity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eregon eregon

Labels
bug
Projects
None yet
Milestone
21.3.0
Development

No branches or pull requests
2 participants
@eregon @coorasse