TypeError Regexp#match?(nil) in Ruby Head #37504

utilum · 2019-10-18T16:45:54Z

Aa of ruby/ruby@2a22a6b calling
Regexp#match?(nil) raises an exception.

This fixes instances instances raised by the test suite.

kaspth · 2019-10-19T00:09:11Z

What would the perf/memory impact be of using to_s for some of these? Or what about swapping the order so we can do nil&.match?(/regexp/)?

utilum · 2019-10-19T04:21:35Z

I'll have a look.

eregon · 2019-10-26T20:22:44Z

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -346,7 +346,7 @@ def check_part(name, part, path_params, hash)
          end

          def split_to(to)
-            if /#/.match?(to)
+            if !to.nil? && /#/.match?(to)


I would simplify to if to && /#/.match?(to) here and other places.
If to was false, it would already raise a TypeError in <=2.6.

eregon · 2019-10-26T20:25:53Z

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -355,7 +355,7 @@ def split_to(to)

          def add_controller_module(controller, modyoule)
            if modyoule && !controller.is_a?(Regexp)
-              if %r{\A/}.match?(controller)
+              if !controller.nil? && %r{\A/}.match?(controller)


if controller && controller.start_with?('/') would be more readable here.

Using start_with? here should have been enforced by RuboCop in #32695, but wasn't due to a bug (rubocop/rubocop#7464).

In any case, I think this patch should only contain changes required for compatibility with ruby/ruby#1506, no matter how tempting adjacent refactorings may be.

Good point, it's probably safer to just directly fix the compatibility with ruby-head in this PR.

eregon · 2019-10-27T10:09:36Z

actionview/lib/action_view/layouts.rb

@@ -280,7 +280,11 @@ def layout(layout, conditions = {})
      def _write_layout_method # :nodoc:
        silence_redefinition_of_method(:_layout)

-        prefixes = /\blayouts/.match?(_implied_layout_name) ? [] : ["layouts"]
+        if _implied_layout_name.nil?
+          ["layouts"]


This is not assigned to prefixes.
It seems better to keep the condition together:

prefixes = if _implied_layout_name && /\blayouts/.match?(_implied_layout_name) [] else ["layouts"] end

eregon · 2019-10-27T10:13:14Z

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -620,7 +620,7 @@ def extract_default_function(default_value, default)
        end

        def has_default_function?(default_value, default)
-          !default_value && %r{\w+\(.*\)|\(.*\)::\w+|CURRENT_DATE|CURRENT_TIMESTAMP}.match?(default)
+          !default.nil? && !default_value && %r{\w+\(.*\)|\(.*\)::\w+|CURRENT_DATE|CURRENT_TIMESTAMP}.match?(default)


Maybe this to keep closer to the original code and avoid double negation:

Suggested change

!default.nil? && !default_value && %r{\w+\(.*\)|\(.*\)::\w+|CURRENT_DATE|CURRENT_TIMESTAMP}.match?(default)

!default_value && default && %r{\w+\(.*\)|\(.*\)::\w+|CURRENT_DATE|CURRENT_TIMESTAMP}.match?(default)

eregon

Looks good to me.

rafaelfranca

In my opinion this is a backward incompatible change in Ruby and should be fixed there.

eregon · 2019-11-01T09:56:30Z

Discussion on the Ruby tracker: https://bugs.ruby-lang.org/issues/13083

It still seems unintentional to me to pass nil to match/match? so I think this would be useful to change in Rails whether that behavior is deprecated or raises.

rafaelfranca · 2019-11-01T16:30:27Z

I'm fine with changing the code to avoid deprecation but I think we can avoid that this change break apps.

eugeneius · 2019-11-02T22:35:16Z

There's one more match? call that needs to be fixed:

rails/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb

Line 165 in 9bfb73a

    
           if type_metadata.type == :datetime && /\ACURRENT_TIMESTAMP(?:\([0-6]?\))?\z/i.match?(default)

It currently causes the Active Record test suite to fail when run with the mysql2 adapter:

$ bundle exec rake test:mysql2
# ...
/Users/eugene/lib/rails/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb:165:in `match?': no implicit conversion of nil into String (TypeError)
rake aborted!

Aa of ruby/ruby@2a22a6b calling `Regexp#match?(nil)` raises an exception. [utilum, eregon, eugeneius]

eugeneius · 2019-11-03T17:59:21Z

@utilum did you explore @kaspth's suggestion of using the safe navigation operator here? It looks like it could be applied uniformly, instead of the current approach that solves each one slightly differently (switching unless to if, replacing ternaries, adding branches to conditionals, etc.)

eugeneius · 2019-11-03T18:04:33Z

ruby/ruby#2637 was merged, so Regexp#match?(nil) will no longer raise in Ruby 2.7, but this patch is still useful to avoid the deprecation warnings.

TypeError Regexp#match?(nil) in Ruby Head

…of_nil" This reverts commit 4e10538, reversing changes made to 62b4383. The change in Ruby that made those changes required was reverted in https://bugs.ruby-lang.org/projects/ruby-trunk/repository/git/revisions/8852fa876039ed177fd5e867f36177d8a9ff411c

…of_nil" This reverts commit a077f94. That commit in Ruby that required those changes were reverted in https://bugs.ruby-lang.org/projects/ruby-trunk/repository/git/revisions/8852fa876039ed177fd5e867f36177d8a9ff411c

rails-bot bot added actionpack actionview activemodel activerecord labels Oct 18, 2019

This was referenced Oct 19, 2019

TypeError Regexp#match?(nil) in Ruby Head rails/sprockets#646

Closed

TypeError Regexp#match?(nil) in Ruby Head rack/rack#1403

Merged

utilum force-pushed the no_implicit_conversion_of_nil branch 2 times, most recently from 4a4c9f1 to 49af919 Compare October 19, 2019 04:41

eugeneius mentioned this pull request Oct 26, 2019

2.7.0-preview2 triggers 'TypeError: no implicit conversion of nil into String' #37572

Closed

eregon reviewed Oct 26, 2019

View reviewed changes

eugeneius mentioned this pull request Oct 27, 2019

Add forward slash to LITERAL_REGEX rubocop/rubocop#7464

Merged

8 tasks

utilum force-pushed the no_implicit_conversion_of_nil branch from 49af919 to a3ff6a6 Compare October 27, 2019 08:02

eregon reviewed Oct 27, 2019

View reviewed changes

eregon approved these changes Oct 27, 2019

View reviewed changes

utilum force-pushed the no_implicit_conversion_of_nil branch 4 times, most recently from c328207 to 32e4e0c Compare October 28, 2019 06:31

rafaelfranca requested changes Oct 31, 2019

View reviewed changes

TypeError Regexp#match?(nil) in Ruby Head

2ca6830

Aa of ruby/ruby@2a22a6b calling `Regexp#match?(nil)` raises an exception. [utilum, eregon, eugeneius]

utilum force-pushed the no_implicit_conversion_of_nil branch from 32e4e0c to 2ca6830 Compare November 3, 2019 09:26

rafaelfranca merged commit 4e10538 into rails:master Nov 4, 2019

rafaelfranca added a commit that referenced this pull request Nov 4, 2019

Merge pull request #37504 from utilum/no_implicit_conversion_of_nil

a077f94

TypeError Regexp#match?(nil) in Ruby Head

utilum deleted the no_implicit_conversion_of_nil branch November 4, 2019 20:36

eugeneius mentioned this pull request Nov 8, 2019

fix add_controller_module': undefined method start_with?' for :bloc… #37657

Closed

eitoball mentioned this pull request Nov 28, 2019

Add Ruby 2.7.0-preview1 to the test matrix and upgrade older versions rubyconfig/config#226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeError Regexp#match?(nil) in Ruby Head #37504

TypeError Regexp#match?(nil) in Ruby Head #37504

utilum commented Oct 18, 2019

kaspth commented Oct 19, 2019

utilum commented Oct 19, 2019

eregon Oct 26, 2019

eregon Oct 26, 2019

eugeneius Oct 27, 2019

eregon Oct 27, 2019

eregon Oct 27, 2019

eregon Oct 27, 2019

eregon left a comment

rafaelfranca left a comment

eregon commented Nov 1, 2019

rafaelfranca commented Nov 1, 2019

eugeneius commented Nov 2, 2019

eugeneius commented Nov 3, 2019

eugeneius commented Nov 3, 2019

	!default.nil? && !default_value && %r{\w+\(.\)\|\(.\)::\w+\|CURRENT_DATE\|CURRENT_TIMESTAMP}.match?(default)
	!default_value && default && %r{\w+\(.\)\|\(.\)::\w+\|CURRENT_DATE\|CURRENT_TIMESTAMP}.match?(default)

TypeError Regexp#match?(nil) in Ruby Head #37504

TypeError Regexp#match?(nil) in Ruby Head #37504

Conversation

utilum commented Oct 18, 2019

kaspth commented Oct 19, 2019

utilum commented Oct 19, 2019

eregon Oct 26, 2019

Choose a reason for hiding this comment

eregon Oct 26, 2019

Choose a reason for hiding this comment

eugeneius Oct 27, 2019

Choose a reason for hiding this comment

eregon Oct 27, 2019

Choose a reason for hiding this comment

eregon Oct 27, 2019

Choose a reason for hiding this comment

eregon Oct 27, 2019

Choose a reason for hiding this comment

eregon left a comment

Choose a reason for hiding this comment

rafaelfranca left a comment

Choose a reason for hiding this comment

eregon commented Nov 1, 2019

rafaelfranca commented Nov 1, 2019

eugeneius commented Nov 2, 2019

eugeneius commented Nov 3, 2019

eugeneius commented Nov 3, 2019