Test against Ruby 3.2, fix broken things by magni- · Pull Request #134 · sinatra/mustermann

magni- · 2022-07-20T05:43:54Z

My build for a PR in ruby-grape/grape-swagger, which depends on mustermann, failed on Ruby 3.2. Digging into the failure, it seems to be caused by a change concerning ruby2_keywords:

Methods taking a rest parameter (like *args) and wishing to delegate keyword arguments through foo(*args) must now be marked with ruby2_keywords (if not already the case). In other words, all methods wishing to delegate keyword arguments through *args must now be marked with ruby2_keywords, with no exception. This will make it easier to transition to other ways of delegation once a library can require Ruby 3+. Previously, the ruby2_keywords flag was kept if the receiving method took *args, but this was a bug and an inconsistency. A good technique to find the potentially-missing ruby2_keywords is to run the test suite, for where it fails find the last method which must receive keyword arguments, use puts nil, caller, nil there, and check each method/block on the call chain which must delegate keywords is correctly marked as ruby2_keywords. [Bug #18625] [Bug #16466]
def target(**kw)
end

# Accidentally worked without ruby2_keywords in Ruby 2.7-3.1, ruby2_keywords
# needed in 3.2+. Just like (*args, **kwargs) or (...) would be needed on
# both #foo and #bar when migrating away from ruby2_keywords.
ruby2_keywords def bar(*args)
  target(*args)
end

ruby2_keywords def foo(*args)
  bar(*args)
end

foo(k: 1)

There's probably other methods that now need to be marked? I added head to the CI test matrix, so those should be caught there.

dentarg · 2022-07-20T06:33:32Z

@jkowens are you able to add more maintainers here on GitHub? Would be good to have more people being able to approve CI runs

Anyway, I ran the tests in my fork, see https://github.com/dentarg/mustermann/actions/runs/2702649077, looks like this needs more work

dentarg · 2022-07-20T06:34:33Z

Also, @eregon probably knows how to handle this :)

magni- · 2022-07-20T08:11:20Z

Anyway, I ran the tests in my fork, see https://github.com/dentarg/mustermann/actions/runs/2702649077, looks like this needs more work

Thanks, looks like ruby2_keywords is for module/class instance methods, so ruby2_keywords def self.parse(...) won't work.

edit: or not, I took that from the README for the ruby2_keywords gem, which is only used on Ruby < 2.6—which is where tests passed. Tests failed on Ruby > 2.7, which doesn't use ruby2_keywords from the ruby2_keywords gem.

edit2: or not not, moving the ruby2_keywords call to a class << self block, I get the full suite passing locally on Ruby 3.0

eregon · 2022-07-20T08:41:56Z

mustermann/lib/mustermann/ast/node.rb


+      class << self
+        ruby2_keywords :parse
+      end


Could you move it just below the declaration of def self.parse? Otherwise it's hard to find (in both directions).

eregon · 2022-07-20T08:46:05Z

Yes, slightly more ruby2_keywords are needed for Ruby 3.2+.
That changes makes sense, it might be needed in more places.
An alternative here would be to use payload = nil, **options instead of *args for def self.parse's arguments, because those arguments are passed to initialize which takes payload = nil, **options. The advantage is this doesn't need the complex (semantically and a bit of cost) marking of ruby2_keywords.

eregon · 2022-07-20T08:47:01Z

@jkowens are you able to add more maintainers here on GitHub? Would be good to have more people being able to approve CI runs

I've also found there is a way to run actions without needing approvals except for new users on GitHub in the repo's Actions settings, that seems convenient.

magni- · 2022-07-20T13:38:54Z

The build now passes on my fork on 3.2: https://github.com/magni-/mustermann/runs/7430295996?check_suite_focus=true#step:5:15

It turned out the main cause of failures was the removal of Kernel#=~. I went with the ugly easy fix (respond_to?) but maybe we shouldn't be passing in a non-String as a parameter called string ?

While grepping for other places that might be missing a ruby2_keywords, I ran into these two:

diff --git mustermann-contrib/lib/mustermann/flask.rb mustermann-contrib/lib/mustermann/flask.rb
index 4caa3b8..848deab 100644
--- mustermann-contrib/lib/mustermann/flask.rb
+++ mustermann-contrib/lib/mustermann/flask.rb
@@ -179,11 +179,11 @@ def self.register_converter(name, converter = nil, &block)
       converter.node_type = :named_splat
     end
 
-    register_converter(:any) do |converter, *strings|
+    register_converter(:any, &-> (converter, *strings) do
       strings              = strings.map { |s| Regexp.escape(s) unless s == {} }.compact
       converter.qualifier  = ""
       converter.constraint = Regexp.union(*strings)
-    end
+    end.ruby2_keywords)
 
     register_converter(:default, converters['string'])
 
diff --git mustermann/lib/mustermann/ast/transformer.rb mustermann/lib/mustermann/ast/transformer.rb
index f21f8f3..bdfe93f 100644
--- mustermann/lib/mustermann/ast/transformer.rb
+++ mustermann/lib/mustermann/ast/transformer.rb
@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+require 'ruby2_keywords'
 require 'mustermann/ast/translator'
 
 module Mustermann
@@ -139,7 +140,7 @@ def track(element)
 
         # turn look ahead buffer into look ahead node
         # @!visibility private
-        def create_lookahead(elements, *args)
+        ruby2_keywords def create_lookahead(elements, *args)
           return elements unless elements.size > 1
           [Node[:with_look_ahead].new(elements, *args, start: elements.first.start, stop: elements.last.stop)]
         end

But the build passes without these changes and I'm really unfamiliar with both this codebase and the ruby2_keywords method, so I'm not sure if those changes are actually needed/correct.

An alternative would be to mark the method with `ruby2_keywords`: From the Ruby 3.2 changelog: > Methods taking a rest parameter (like `*args`) and wishing to delegate keyword arguments through `foo(*args)` must now be marked with `ruby2_keywords` (if not already the case). In other words, all methods wishing to delegate keyword arguments through `*args` must now be marked with `ruby2_keywords`, with no exception. This will make it easier to transition to other ways of delegation once a library can require Ruby 3+. Previously, the `ruby2_keywords` flag was kept if the receiving method took `*args`, but this was a bug and an inconsistency. A good technique to find the potentially-missing `ruby2_keywords` is to run the test suite, for where it fails find the last method which must receive keyword arguments, use `puts nil, caller, nil` there, and check each method/block on the call chain which must delegate keywords is correctly marked as `ruby2_keywords`. [[Bug #18625](https://bugs.ruby-lang.org/issues/18625)] [[Bug #16466](https://bugs.ruby-lang.org/issues/16466)] `Node.parse` takes `*args` and delegates them to `Node.new`, therefore it needs to be marked with `ruby2_keywords`.

`Kernel` defined `#=~` up until Ruby 3.2, but it just returned `nil` when the method wasn't redefined by a child class.

jkowens · 2022-07-20T13:50:50Z

@jkowens are you able to add more maintainers here on GitHub? Would be good to have more people being able to approve CI runs

Anyway, I ran the tests in my fork, see https://github.com/dentarg/mustermann/actions/runs/2702649077, looks like this needs more work

@dentarg I wish I could, but I don't have access to do that. @zzak could you help us out with that?

magni- · 2022-07-20T14:07:35Z

Also, while the specs pass, the coverage checks blows up because of the removal of the deprecated File.exists? method, which is referenced by coveralls:0.7.2. coveralls fixed this in 0.7.3 to use File.exist? instead, but we can't upgrade to that (or to a newer version) because of our dependency on simplecov (see lemurheavy/coveralls-ruby#172)

Fetching gem metadata from https://rubygems.org/..........
Resolving dependencies...
Bundler could not find compatible versions for gem "simplecov":
  In Gemfile:
    support was resolved to 0.0.1, which depends on
      coveralls (~> 0.8.23) was resolved to 0.8.23, which depends on
        simplecov (~> 0.16.1)

    support was resolved to 0.0.1, which depends on
      simplecov (~> 0.17.0)

jkowens · 2022-07-20T14:07:48Z

Looks like is passing as expected. Good to merge? What release version should this go out as? Is releasing this in 3.0 ok, or do we need a patch release in the 2x series?

magni- · 2022-07-20T14:13:57Z

Good to merge?

I'd welcome @eregon's eyes on this again since I gather he knows far more about these Ruby 3.2 changes than I do 🙇🏼

What release version should this go out as? Is releasing this in 3.0 ok, or do we need a patch release in the 2x series?

Since 3.0 isn't out yet, might as well release it on 2.x first? Looking at #133, there should be just the small conflict in .github/workflows/test.yml.

dentarg · 2022-07-20T14:24:25Z

the coverage checks blows up

Looks like it is the sending to Coveralls that doesn't work but that's already the case for the other jobs too (Coveralls encountered an exception: RestClient::UnprocessableEntity 422 Unprocessable Entity), thankfully it doesn't fail the job so we can ignore that for now. :)

eregon · 2022-07-20T17:29:34Z

mustermann/lib/mustermann/ast/expander.rb

      # @see Mustermann::AST::Translator#expand
      # @!visibility private
      ruby2_keywords def escape(string, *args)
+        return super unless string.respond_to?(:=~)


If it's just nil/false, I'd simplify to return super unless string or integrate it in the ternary below.
Or are other types seen?

From the build failures, it's Integers: https://github.com/magni-/mustermann/runs/7425121555?check_suite_focus=true#step:5:14

e.g.

9) Mustermann::Expander supports combining different pattern styles Failure/Error: expander.expand(bar: 23).should be == "/23" NoMethodError: undefined method `=~' for 23:Integer # ./mustermann/lib/mustermann/ast/expander.rb:128:in `escape' # ./mustermann/lib/mustermann/ast/expander.rb:99:in `block in expand' # ./mustermann/lib/mustermann/ast/expander.rb:99:in `each' # ./mustermann/lib/mustermann/ast/expander.rb:99:in `expand' # ./mustermann/lib/mustermann/expander.rb:150:in `expand' # ./mustermann/spec/expander_spec.rb:26:in `block (2 levels) in <top (required)>'

eregon · 2022-07-20T17:37:49Z

@magni- The first change looks wrong to me, because *strings isn't used for delegating keyword arguments.
Regexp.union does not accept kwargs AFAIK.
This (existing) line seems dodgy:

strings              = strings.map { |s| Regexp.escape(s) unless s == {} }.compact

What passes a empty hash to that method? That seems wrong. I think such callers should be fixed to not pass any kwargs or hashes.
But also because of this line the ruby2_keywords shouldn't be needed.

The second change also looks strange to me, because AFAIK one cannot use a ruby2_keywords Hash + explicit kwargs like in:

[Node[:with_look_ahead].new(elements, *args, start: elements.first.start, stop: elements.last.stop)]

An example which show it does not work:

$ ruby -vwe 'def r(*a,**kw); p [a, kw]; end; ruby2_keywords def m(e, *args); r(e, *args, start: 1); end; m(1, 2, 3, kw: 2)'
ruby 3.2.0dev (2022-06-26T06:36:14Z master c2e37c8ff7) [x86_64-linux]
[[1, 2, 3, {:kw=>2}], {:start=>1}]

Do you actually have more warnings or errors without these 2 ruby2_keywords calls?

eregon · 2022-07-20T17:39:29Z

Ah I missed the end of your message:

But the build passes without these changes and I'm really unfamiliar with both this codebase and the ruby2_keywords method, so I'm not sure if those changes are actually needed/correct.

From what I can see they are not needed and not correct (well, simply not needed/no effect and might cause warnings).
There is some weird code there with the strings = strings.map { |s| Regexp.escape(s) unless s == {} }.compact, but already preexisting to this PR and probably should be cleaned up separately.

eregon · 2022-07-20T17:40:56Z

mustermann/lib/mustermann/ast/node.rb

+      def self.parse(payload = nil, **options, &block)
+        new(payload, **options).tap { |n| n.parse(&block) }


Looks good 👍

magni- · 2022-07-21T23:05:22Z

Anything else needed for this PR? 🙇🏼

eregon

IMHO good to merge

Test on Ruby@head as well

55c86ce

magni- mentioned this pull request Jul 20, 2022

Allow using nicknames for body definitions ruby-grape/grape-swagger#862

Merged

magni- force-pushed the pp/ruby-3.2-fix branch from 08f5ee8 to b37b266 Compare July 20, 2022 08:23

eregon reviewed Jul 20, 2022

View reviewed changes

magni- force-pushed the pp/ruby-3.2-fix branch from b37b266 to da64be6 Compare July 20, 2022 13:33

magni- added 2 commits July 20, 2022 22:39

Don't call #=~ on objects that don't respond to it

8be5bd4

`Kernel` defined `#=~` up until Ruby 3.2, but it just returned `nil` when the method wasn't redefined by a child class.

magni- force-pushed the pp/ruby-3.2-fix branch from da64be6 to 8be5bd4 Compare July 20, 2022 13:39

eregon reviewed Jul 20, 2022

View reviewed changes

magni- changed the title ~~Test against Ruby 3.2, properly mark methods with ruby2_keywords~~ Test against Ruby 3.2, fix broken things Jul 21, 2022

eregon approved these changes Jul 22, 2022

View reviewed changes

jkowens merged commit 99b4b46 into sinatra:master Jul 22, 2022

magni- deleted the pp/ruby-3.2-fix branch July 22, 2022 14:40

dentarg mentioned this pull request Nov 14, 2022

Ruby 3.2 compat of Mustermann::Rails#expand #135

Closed

		def self.parse(payload = nil, **options, &block)
		new(payload, **options).tap { \|n\| n.parse(&block) }

Conversation

magni- commented Jul 20, 2022

Uh oh!

dentarg commented Jul 20, 2022

Uh oh!

dentarg commented Jul 20, 2022

Uh oh!

magni- commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

eregon commented Jul 20, 2022

Uh oh!

eregon commented Jul 20, 2022

Uh oh!

magni- commented Jul 20, 2022

Uh oh!

jkowens commented Jul 20, 2022

Uh oh!

magni- commented Jul 20, 2022

Uh oh!

jkowens commented Jul 20, 2022

Uh oh!

magni- commented Jul 20, 2022

Uh oh!

dentarg commented Jul 20, 2022

Uh oh!

eregon Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

magni- Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

eregon commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eregon Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

magni- commented Jul 21, 2022

Uh oh!

eregon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

magni- commented Jul 20, 2022 •

edited

Loading

eregon commented Jul 20, 2022 •

edited

Loading

eregon commented Jul 20, 2022 •

edited

Loading