Support ruby-head/2.8

[p-mongo]

ruby-head does not pass keyword arguments, making this test fail:

class Foo
  def m(a, b: nil)
    p [a, b]
  end
end

describe 'foo' do
  it 'bars' do
    expect_any_instance_of(Foo).to receive(:m).and_call_original

    Foo.new.m(1, b: 2)
  end
end

Failures:

  1) foo bars
     Failure/Error:
       def m(a, b: nil)
         p [a, b]
       end
     
     ArgumentError:
       wrong number of arguments (given 2, expected 1)
     # ./test.rb:5:in `m'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/message_expectation.rb:102:in `call'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/message_expectation.rb:102:in `block in and_call_original'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/message_expectation.rb:743:in `call'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/message_expectation.rb:574:in `invoke_incrementing_actual_calls_by'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/message_expectation.rb:428:in `invoke'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/proxy.rb:202:in `message_received'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/proxy.rb:348:in `message_received'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/method_double.rb:77:in `proxy_method_invoked'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/method_double.rb:64:in `block (2 levels) in define_proxy_method'
     # /home/me/apps/exp/rspec-mocks/lib/rspec/mocks/any_instance/recorder.rb:262:in `block in observe!'
     # ./test.rb:14:in `block (2 levels) in '
     # /home/me/apps/exp/rspec-core/lib/rspec/core/example.rb:257:in `instance_exec'
     # /home/me/apps/exp/rspec-core/lib/rspec/core/example.rb:257:in `block in run'
     # /home/me/apps/exp/rspec-core/lib/rspec/core/example.rb:503:in `block in with_around_and_singleton_context_hooks'
     # /home/me/apps/exp/rspec-core/lib/rspec/core/example.rb:460:in `block in with_around_example_hooks'
     # /home/me/apps/exp/rspec-core/lib/rspec/core/hooks.rb:481:in `block in run'

Proposed solution is to take *args and **opts separately in all methods.

[JonRowe]

We can't use ** in RSpec currently due to our support of older Rubies. We are working on improved keyword argument support but theres no way this PR can be merged as is currently.

[pirj]

1.8.7 doesn't seem to handle kwargs that well:

SyntaxError:

  /home/travis/build/rspec/rspec-mocks/lib/rspec/mocks/method_double.rb:63: syntax error, unexpected tPOW, expecting tAMPER

            define_method(method_name) do |*args, **opts, &block|

Let's remove Byebug and see what really fails here.

Otherwise, good job, I believe it's the way to go.

You might be interested in this approach to maintain support for both Ruby 1.8.7 and 2.8+.

[pirj]

Wondering if we can pass block as &block and get rid of _block parameter in the notify_received_message method signature.

[pirj]

Leftover.

[pirj]

Maybe pass it as *args, **opts here as well?

[JonRowe]

@pirj, 1.8.7 does not support ** at all, its a syntax error.

[pirj]

That's what I've tried to point out in a gentle manner :D

[p-mongo]

@JonRowe

We are working on improved keyword argument support but theres no way this PR can be merged as is currently.

Is there an ETA for this? I imagine many more projects would be interested in rspec supporting ruby 2.8 than 1.8.

[JonRowe]

Is there an ETA for this? I imagine many more projects would be interested in rspec supporting ruby 2.8 than 1.8.

Sorry, we're all volunteers no ETAs. Its more of a Ruby 2.7 support issue at the moment as thats a released version of Ruby. Ruby 2.8 will be supported when its released which is traditionally in December and by then I hope to have a new major release of RSpec dropping older Ruby support.

[p-mongo]

Well, you also have volunteers offering to contribute ruby 2.8 support (I'm not the only one from what I am gathering) but what you are saying - correct me if I'm wrong - is this work is not wanted because you wish to support long-EOLed ruby 1.8/1.9.

For us (Ruby driver for MongoDB and Mongoid) lack of Ruby 2.8 support in rspec is effectively a regression - we test all projects on ruby-head and as of Ruby 2.7 release the ruby-head tests are failing because of rspec.

[JonRowe]

Well, you also have volunteers offering to contribute ruby 2.8 support (I'm not the only one from what I am gathering) but what you are saying - correct me if I'm wrong - is this work is not wanted because you wish to support long-EOLed ruby 1.8/1.9.

Not at all, it just needs to support ruby 1.8 and 1.9 too if you'd like it merged into master / released.

[p-mongo]

I am open to doing a reasonable amount of work to achieve that.

If https://github.com/rspec/rspec-support/pull/394/files#diff-6f77530d5756f8c02ca078ddecaa891cL120 is the suggested approach, what this looks like to me is is replacing every one line of code with 10 lines of eval with code duplicated and a major loss of readability. Given the number of lines affected (this PR is only a part of the full change set) I think this is not a realistic approach.

[JonRowe]

I'm open to an improved approach, the difficulty is that older rubies cannot even parse ** thus it may not be in source code for this files. I with extreme hesitance use eval, but its proven to be the only solution so far bar conditional file inclusion which I view as worse because it splits the source tree and is completely non obvious whats happening.

Speaking as the author of the diff you've highlighted I accept that it may look less directly readable due to the string eval (I'm personally ok with it because it has no directly eval'd parts, its literally the code that would exist if it could be parsed by Ruby), but the conditional code definition (its not duplication, the methods are different implementation) is something we do all the time in RSpec because it is much more performant than the alternative of using if at runtime.

[p-mongo]

Is my assessment that 1 line of existing code will be replaced by 10 lines of 1.8+2.8 compatible code correct?

If yes, can the 10 lines be programmatically generated from 1 line (potentially a revised one)? If so I would say this is a smarter path to pursue than writing 1000-2000 lines of code now (based on this PR being +35 -35, and being incomplete) which will need to be removed in a year. If the 10 lines of code cannot be programmatically generated (i.e. they have important differences) I doubt writing them by hand and getting them right is something I personally see myself doing.

I think it will be much less effort overall to a version like 3.9 to be a "long term support" one which ruby 1.8/1.9 projects can use, with bug fixes only. I suspect there aren't too many bugs reported against 3.x branch of rspec after 9 minor releases? rspec 3.10 can then require ruby 2.0.

[JonRowe]

Is my assessment that 1 line of existing code will be replaced by 10 lines of 1.8+2.8 compatible code correct?

No it's not, what happened was the original implementation was inlined then wrapped in an if to expand with the evald Ruby 2.7 definition. This caused an N+3 addition of code. 1 new line for every existing line, plus three to wrap it in an if / else / end.

If yes, can the 10 lines be programmatically generated from 1 line (potentially a revised one)?

I did it in vim so, probably, except for the new code addition a vim macro could quite easily add the if and conditional case the old code allowing you to concentrate then on the code you wish to change.

If so I would say this is a smarter path to pursue than writing 1000-2000 lines of code now (based on this PR being +35 -35, and being incomplete)

It wont be thousands of lines of code, its just a few, you likely have most of them in this PR, most of RSpec does not need keyword argument support in this fashion because we don't use keyword arguments except for supporting user code in mock definitions.

I think it will be much less effort overall to a version like 3.9 to be a "long term support" one which ruby 1.8/1.9 projects can use, with bug fixes only. I suspect there aren't too many bugs reported against 3.x branch of rspec after 9 minor releases? rspec 3.10 can then require ruby 2.0.

Semantic versioning requires us to support all the older rubies until RSpec 4. There isn't likely to be a 3.10.

[p-mongo]

It wont be thousands of lines of code, its just a few, you likely have most of them in this PR

OK, in this case would you mind taking one/a few of the hunks in this PR and altering it to be functionally correct and mergeable?

I did it in vim so, probably, except for the new code addition a vim macro

What I meant was, the source code would contain 1 line. This line will either be expanded at runtime or in some kind of a pre-processing transformation but if one needs to make changes to that line in the future they would be working with this 1 line instead of the expanded 10 lines.

[JonRowe]

OK, in this case would you mind taking one/a few of the hunks in this PR and altering it to be functionally correct and mergeable?

I'll see if I can find time later in the week, it's late here and I have a busy work schedule tomorrow.

What I meant was, the source code would contain 1 line. This line will either be expanded at runtime or in some kind of a pre-processing transformation but if one needs to make changes to that line in the future they would be working with this 1 line instead of the expanded 10 lines.

That sounds overly complicated, as indicated we prefer solutions that are implemented at parse time so that user performance isn't effected. In practise we've never needed to modify the fallback "old code" because its worked for so long previously, it tends to be the new solutions which introduce bugs.

[pirj]

you also have volunteers offering to contribute ruby 2.8 support (I'm not the only one from what I am gathering)

Volunteers are always welcome!

You seem to have experience with ruby-head, wondering if the way taken by Rails works fine:

def foo
  # ...
end
ruby2_keywords(:foo) if respond_to?(:ruby2_keywords, true)

Does it look like a reasonable approach to you?

the ruby-head tests are failing because of rspec

I've checked some of the recent builds, jobs are green except ruby-head ones.
The reasons, however, seem unrelated to RSpec:

1, 2

(ArgumentError: Delegation needs a target. Supply an options hash with a :to key as the last argument (e.g. delegate :hello, to: :greeter).
# ./lib/mongoid.rb:112:in `<module:Mongoid>'

3

/data/mci/efda4d40e5148eff650691b83536fe2d/data/mci/efda4d40e5148eff650691b83536fe2d/drivers-tools/.evergreen/orchestration/server.log: no such file or directory

I apologise in advance for not checking further builds for failure reasons, and I completely understand your point that lack of proper ruby-head support in RSpec is a blocker for you since you can't be certain that the projects you're working on are ruby-head-ready.

I am open to doing a reasonable amount of work
I doubt writing them by hand and getting them right is something I personally see myself doing.

Keeping in mind your tight schedule and lack of even formal ETA this raises the risks of not being it ready by the moment Ruby 2.8 is released in December.

Leaving it up to you to decide what is a reasonable amount of work that you would like to dedicate to improve or completely fix RSpec support for ruby-head, this effort is very appreciated.

What I meant was, the source code would contain 1 line. This line will either be expanded at runtime or in some kind of a pre-processing transformation but if one needs to make changes to that line in the future they would be working with this 1 line instead of the expanded 10 lines.

Do you have some examples of such generator/pre-processing, preferably for real code, ideally in this pull request? I'd really love to see this in action, might be the most viable approach to the problem.

[p-mongo]

wondering if the way taken by Rails works fine:

It seems fine to me but https://github.com/ruby/ruby2_keywords does not explain much as far as what it does, when one would use it, what the caveats are, etc. If it works I say it's a fine solution.

The reasons, however, seem unrelated to RSpec:

These are fixed in mongodb/mongoid#4720. The current build log is https://evergreen.mongodb.com/task_log_raw/mongoid_rubies__mongodb_version~4.0_topology~replica_set_ruby~ruby_head_driver~current_test_patch_044e3c9751851638ef4a85a845fc16ecb82831b3_5e28c5f83627e075e55518da_20_01_22_22_00_37/0?type=T, in which the first 5 failures are caused by the issue described in this PR. I don't know if the subsequent failures are issues in Mongoid that are yet to be addressed or failures resulting from the first 5 failures in some way.

Do you have some examples of such generator/pre-processing, preferably for real code, ideally in this pull request?

No, this was just a general remark that in Ruby I typically do not expect to type out a lot of boilerplate. I use Ruby because it is a concise language.

I'd really love to see this in action, might be the most viable approach to the problem.

I had another thought as to how to fix the overall problem. I figure most methods changed in this PR are internal to rspec and thus don't need to use **opts syntax - they can pass opts as an argument. The only methods that must deal with keyword arguments are the opposite ends of the entire stack (the method called by applications and the method in rspec that ultimately invokes original implementation). So, the awkwardness may only need to exist in a relatively small number of places.

On the topic of 1.8/1.9 support, we stopped supporting them in part because building those rubies seems to be running into problems with current openssl releases. Which modern linux distributions do 1.8/1.9 build on out of the box?

[pirj]

The only methods that must deal with keyword arguments are the opposite ends of the entire stack (the method called by applications and the method in rspec that ultimately invokes original implementation). So, the awkwardness may only need to exist in a relatively small number of places.

It's fine with me.

[JonRowe]

Anywhere we defined the method, we should be able to add ruby2_keywords :method_name if respond_to?(ruby2_keywords) to help here, it currently doesn't work for and_call_original but I'm working on it, we also need specs to trigger the warning to fix it etc

[JonRowe]

#1324 is currently warning free on ruby-head (the build fails due to rspec-rails which will always be a bit more convulted) so if you have some more examples feel feel to report them but I'm closing this in preference of that for now.