Call .value to get gemspec required_ruby_version only if str_type? #12732
Conversation
davidrunger
force-pushed
the
fix-error-getting-target-ruby-from-gemspec
branch
from
c3ba8dc
to
80e4f02
Compare
| @@ -42,39 +42,39 @@ | |||
| content = <<~HEREDOC | |||
| Gem::Specification.new do |s| | |||
| s.name = 'test' | |||
| s.required_ruby_version = '>= 2.7.2' | |||
| s.required_ruby_version = '>= 3.2.2' | |||
There was a problem hiding this comment.
As alluded to in the PR description, most of these changes to Ruby version numbers in this PR are not necessary. The specs continue to pass without these changes. The reason that I am making these changes is because these specs were at risk of passing, even when the relevant functionality has been broken, because the specs previously expected 2.7 to be returned, which also happens to be the current default Ruby version. By changing the specs to expect a non-default Ruby version, they will now fail, if/when the relevant functionality is broken (as, indeed, I did break it, in one of my initial attempts at fixing this bug).
| context 'when file reads the required_ruby_version from another file' do | ||
| it 'uses the default target ruby version' do |
There was a problem hiding this comment.
This is the only new spec actually being added in this PR. All of the other changes in this file are just changing the Ruby version for existing specs.
|
The tests are currently failing. Apart from this your changes look good to me. Excellent analysis of the root cause of the issue, btw! |
This change avoids an exception (NoMethodError: undefined method `value' for an instance of RuboCop::AST::SendNode) that currently occurs in `rubocop` version 1.61.0 when the project has a gemspec file and that gemspec specifies a `required_ruby_version` by reading from another file (such as `.ruby-version`). This bug began occurring in several of my projects as a result of #12645 (d1746be) ("Check gemspec `required_ruby_version` before `.ruby-version` and other sources"). My projects have both a `.ruby-version` file and a gemspec that sets the `required_ruby_version` by reading from that `.ruby-version` file. Prior to #12645, the latent bug that this change fixes was not occurring for me, because the GemspecFile TargetRuby finder was never running, because the RubyVersionFile TargetRuby finder had higher precedence (and a target Ruby version was found using this finder), and so the GemspecFile TargetRubyVersion finder was never invoked. With #12645, because the GemspecFile finder now runs before the RubyVersionFile finder, this exception has started occurring in these projects, when I try to run rubocop 1.61.0. This exception can be fixed/avoided by checking that the `right_hand_side` is a `str_type?` before attempting to call `right_hand_side.value`. In a setup like mentioned in my projects above, `right_hand_side` will be a `RuboCop::AST::SendNode` and `str_type?` will be false, so we avoid the NoMethodError exception that otherwise will occur if we attempt to call `.value` on that node. I also updated several of the specs in the `target_ruby_spec.rb` file. The motivation for this change is that several of these specs were at risk of "falsely passing", because they had spec setups that intended for `2.7` to be returned as the target Ruby version, but Ruby 2.7 is also currently the `RuboCop::TargetRuby::DEFAULT_VERSION`. So, in working on the bug that is the focus of this change, although one of my initial attempts to fix the bug substantially broke some relevant functionality, all of the specs nevertheless still passed, because they expected a target Ruby version of 2.7 to be returned, and it was still being returned, even after I had broken some of the functionality, simply because Ruby 2.7 is also the default. By setting up the specs to expect a target Ruby version other than the default (Ruby 2.7), the specs will now fail, if/when the relevant functionality under test is broken.
This should bust the gems cache and cause us to update from rubocop-rspec 2.27.0 to 2.27.1 for CI runs.
davidrunger
force-pushed
the
fix-error-getting-target-ruby-from-gemspec
branch
from
80e4f02
to
cf03c84
Compare
|
@bbatsov : Thank you for taking a look! I am pretty sure that tests started failing because of a false positive for I have pushed cf03c84 to ensure that we are running |
|
Done. |
|
@bbatsov : Tests passed, with the exception of a failing "CircleCI Pipeline" check (which I am guessing you need to separately approve?) and a number of pending checks (which I am guessing will be run after you approve the "CircleCI Pipeline" check?). Or, if there is something that I can/should change/fix on this PR, in order to get those other checks passing, then I'd appreciate any pointers about how to do so. Thank you again for working with me on this! |
|
Something seems broken with CircleCI in general right now, but I'll look into this separately. Thanks for working on this! |
This fixes an issue mentioned in [this comment](#12579 (comment)) on issue #12579. (I believe that the main/original issue mentioned in issue #12579 has already been fixed by #12732.) This change is a related/similar followup to #12732 (5026393). That PR avoided an exception that was occurring in the `RuboCop::TargetRuby::GemspecFile` target Ruby finder when a project's gemspec determines the value of `required_ruby_version` in some dynamic way (such as by reading from a `.ruby-version` file). However, #12732 did not handle a case where the dynamic value for the gemspec's `required_ruby_version` is wrapped within an array, and, in such a case, currently, an exception like the following will occur: ``` Gem::Requirement::BadRequirementError: Illformed requirement [">= \#{File.read('.ruby-version').rstrip}"] ./lib/rubocop/target_ruby.rb:123:in `new' ./lib/rubocop/target_ruby.rb:123:in `find_default_minimal_known_ruby' ./lib/rubocop/target_ruby.rb:83:in `find_version' ./lib/rubocop/target_ruby.rb:29:in `initialize' ./lib/rubocop/target_ruby.rb:259:in `new' ``` (As with the scenario that _was_ fixed by #12732, I believe that this is essentially a "latent" issue that is now occurring in more projects than previously, due to #12645 having increased the precedence of the `GemspecFile` target Ruby finder in the `RuboCop::TargetRuby::SOURCES` list.) This change avoids this exception by only attempting to parse a target Ruby version from an array value for a gemspec's `required_ruby_version` if every value in the array is a string. When the values within the `required_ruby_version` array are _not_ all strings, now, rather than raising an unhandled exception, the `GemspecFile` finder will return `nil` for the target Ruby version, and so we will continue on to the other TargetRuby finder classes, in order to find a target Ruby version.
fixes #12744
This change avoids an exception (NoMethodError: undefined method `value' for an instance of RuboCop::AST::SendNode) that currently occurs in
rubocopversion 1.61.0 when the project has a gemspec file and that gemspec specifies arequired_ruby_versionby reading from another file (such as.ruby-version).Example of an affected gemspec file:
This bug began occurring in several of my projects as a result of #12645 (d1746be) ("Check gemspec
required_ruby_versionbefore.ruby-versionand other sources"). My projects have both a.ruby-versionfile and also a gemspec that (as in the example above) sets therequired_ruby_versionby reading from that.ruby-versionfile. Prior to #12645, the latent bug that this change fixes was not occurring for me, because the GemspecFile TargetRuby finder was never running, because the RubyVersionFile TargetRuby finder had higher precedence (and a target Ruby version was found using this finder), and so the GemspecFile TargetRubyVersion finder was never invoked.With #12645, because the GemspecFile finder now runs before the RubyVersionFile finder, this exception has started occurring in these projects, when I try to run rubocop 1.61.0.
This change fixes/avoids this exception by checking that the
right_hand_sideis astr_type?before attempting to callright_hand_side.value. In a setup like mentioned in my projects above,right_hand_sidewill be aRuboCop::AST::SendNodeandstr_type?will befalse, so we avoid the NoMethodError exception that otherwise will occur if we attempt to call.valueon that node.With this change, for projects with such a gemspec file that reads from a
.ruby-versionfile, the GemspecFile finder will now returnnilfor the target Ruby version (rather than raising an unhandled exception), and so we will continue through the other TargetRuby finder classes, until the RubyVersionFile finder returns a Ruby version based on the one specified in the.ruby-versionfile.I also updated several of the specs in the
target_ruby_spec.rbfile. The motivation for these spec changes is that several of the specs were at risk of "falsely passing", because they had spec setups that intended for2.7to be returned as the target Ruby version, but Ruby 2.7 is also currently theRuboCop::TargetRuby::DEFAULT_VERSION. So, in working on the bug that is the focus of this change, although one of my initial attempts to fix the bug actually broke some relevant functionality, all of the specs nevertheless still passed, because they expected a target Ruby version of 2.7 to be returned, and it was still being returned, even after I had broken some of the functionality, simply because Ruby 2.7 is also the default. By setting up the specs to expect a target Ruby version other than the default (Ruby 2.7), the specs will now fail, if/when the relevant functionality under test is broken.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.