[Fix #12823] Fix uninitialized constant error #12826

amomchilov · 2024-04-09T01:45:09Z

The bug originates from #12186, which changed the defined? check in a way that didn't cover this broken reference to Bundler on line 66:

rubocop/lib/rubocop/lockfile.rb

Lines 59 to 68 in a4447ea

    
           # @return [Bundler::LockfileParser, nil] 
        
           def parser 
        
             return @parser if defined?(@parser) 
        
             return unless @lockfile_path 
        
             lockfile = Bundler.read_file(@lockfile_path) 
        
             @parser = lockfile ? Bundler::LockfileParser.new(lockfile) : nil 
        
           rescue Bundler::BundlerError 
        
             nil 
        
           end

This wasn't caught in CI, because none of the cops used by Rubocop (to check its own source code in CI) use Rails cops, so RuboCop::Config#target_rails_version_from_bundler_lock_file was never called in CI.

Validation

An easy way to reproduce this is to just modify a gem used by Rubocop itself (e.g. RuboCop::Cop::Layout::IndentationStyle), adding requires_gem 'rubocop' to its body. This triggers the target_rails_version_from_bundler_lock_file code path when you run exe/rubycop . to run rubocop on its own codebase.

code_to_reproduce.patch

diff --git a/lib/rubocop/cop/layout/indentation_style.rb b/lib/rubocop/cop/layout/indentation_style.rb
index e2b936f73..5e4a152b7 100644
--- a/lib/rubocop/cop/layout/indentation_style.rb
+++ b/lib/rubocop/cop/layout/indentation_style.rb
@@ -37,6 +37,8 @@ module RuboCop
         include RangeHelp
         extend AutoCorrector

+        requires_gem 'rubocop'
+
         MSG = '%<type>s detected in indentation.'

         def on_new_investigation

It fails on 1.63.0, and passes with this PR's changes.

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
~~Added tests.~~
- I don't think we have an easy way to test this, since CI will always have Bundler defined, given that it returns tests in the bundle, that's the whole point).
  
  There's probably some way to test it by spawning a fresh process, but I'm not smart enough to figure that out quickly enough to get this bugfix out :)
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

Fixes the broken reference to `Bundler` on line 66

amomchilov · 2024-04-09T01:46:55Z

lib/rubocop/lockfile.rb

-      @parser = lockfile ? Bundler::LockfileParser.new(lockfile) : nil
-    rescue Bundler::BundlerError
-      nil
+      @parser = if defined?(::Bundler) && @lockfile_path


This should do it: the rescue ::Bundler::BundlerError clause will only be evaluated if the defined?(::Bundler) check passes.

Better yet, this check is effectively memoized, so it won't re-evaluated on every call to the parser (which is used more now than before).

amomchilov

Apologies for the bug, but here's the fix :)

@koic @bbatsov

amomchilov · 2024-04-09T01:49:54Z

lib/rubocop/lockfile.rb

+begin
+  require 'bundler'
+rescue LoadError
+  nil
+end
+


This should have been here all along, but feel free to pull it out of this PR, if you want to keep it super-focused on just fixing the exact bug that's reported by #12823

I put this as a second commit, so feel free to drop it if you'd like.

This does not look right. The question with your checks in the other commit in this PR is not "is bundler available on the system at all" which this require does, but "is this project using bundler". By forcefully loading bundler here, you are also introducing it in cases where bundler is not used (e.g. in a simple ruby script file). Consequently, rubocop now fails because it cannot find a Gemfile.

Consequently, rubocop now fails because it cannot find a Gemfile.

Can you elaborate on that? I don't think that using bundler as a library imposes the requirement for a Gemfile to be present.

I saw failures related to it and effectively the defined?(Bundler) check later is now always true given that normally it should always be possible to require bundler. I'll try to create a proper test case to verify and will open a separate issue for it if I can reproduce it properly.

This issue is expected to be resolved in #12847, which aims to solve #12846.

Yesterday, all our `pronto` actions started [failing like this](https://github.com/getlago/lago-api/actions/runs/8600901566/job/23566843917?pr=1854). It turned out that there was a bug introduced in rubocop, which made it incompatible with `rubocop-rails`. A [fix](rubocop/rubocop#12826) seems on the way. rubocop/rubocop#12823 One way to mitigate this is to use `bundle exec rubocop` instead of global `rubocop`. We don't have pronto in our Gemfile so let's move to regular rubocop for now. Ideally, we'd like to move to checks like this: #1856 Note: It's necessary to exit if there is no files to run because you can run rubocop on the entire codebase today.

bbatsov · 2024-04-09T19:35:03Z

The proposed fix looks good to me. Thanks!

bbatsov · 2024-04-16T08:31:35Z

https://github.com/rubocop/rubocop/releases/tag/v1.63.2 is out

Yesterday, all our `pronto` actions started [failing like this](https://github.com/getlago/lago-api/actions/runs/8600901566/job/23566843917?pr=1854). It turned out that there was a bug introduced in rubocop, which made it incompatible with `rubocop-rails`. A [fix](rubocop/rubocop#12826) seems on the way. rubocop/rubocop#12823 One way to mitigate this is to use `bundle exec rubocop` instead of global `rubocop`. We don't have pronto in our Gemfile so let's move to regular rubocop for now. Ideally, we'd like to move to checks like this: getlago#1856 Note: It's necessary to exit if there is no files to run because you can run rubocop on the entire codebase today.

amomchilov added 2 commits April 8, 2024 21:44

[Fix rubocop#12823] Fix uninitialized constant error

17624d9

Fixes the broken reference to `Bundler` on line 66

Try to use Bundler, if available

5d69642

amomchilov commented Apr 9, 2024

View reviewed changes

amomchilov changed the title ~~[ #12823] Fix uninitialized constant error~~ [Fix #12823] Fix uninitialized constant error Apr 9, 2024

amomchilov commented Apr 9, 2024

View reviewed changes

amomchilov mentioned this pull request Apr 9, 2024

bundle exec required for running rubocop 1.63.0 and rubocop-rails #12823

Closed

amomchilov referenced this pull request Apr 9, 2024

Parameterize lockfile path of RuboCop::Lockfile

caac002

julienbourdeau mentioned this pull request Apr 9, 2024

chore(ci): tentative fix pronto getlago/lago-api#1857

Merged

bbatsov merged commit b0c508a into rubocop:master Apr 9, 2024
31 of 32 checks passed

asottile mentioned this pull request Apr 16, 2024

regression in 1.63.1 -- "Could not locate Gemfile" #12846

Closed

amomchilov mentioned this pull request Apr 24, 2024

[Fix #12865] require 'bundler' if possible and rescue Bundler::GemfileNotFound #12866

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #12823] Fix uninitialized constant error #12826

[Fix #12823] Fix uninitialized constant error #12826

amomchilov commented Apr 9, 2024 •

edited

amomchilov Apr 9, 2024

amomchilov left a comment

amomchilov Apr 9, 2024

graaff Apr 11, 2024

bbatsov Apr 11, 2024

graaff Apr 11, 2024

koic Apr 16, 2024

bbatsov commented Apr 9, 2024

bbatsov commented Apr 16, 2024

	# @return [Bundler::LockfileParser, nil]
	def parser
	return @parser if defined?(@parser)
	return unless @lockfile_path

	lockfile = Bundler.read_file(@lockfile_path)
	@parser = lockfile ? Bundler::LockfileParser.new(lockfile) : nil
	rescue Bundler::BundlerError
	nil
	end

[Fix #12823] Fix uninitialized constant error #12826

[Fix #12823] Fix uninitialized constant error #12826

Conversation

amomchilov commented Apr 9, 2024 • edited

Validation

amomchilov Apr 9, 2024

Choose a reason for hiding this comment

amomchilov left a comment

Choose a reason for hiding this comment

amomchilov Apr 9, 2024

Choose a reason for hiding this comment

graaff Apr 11, 2024

Choose a reason for hiding this comment

bbatsov Apr 11, 2024

Choose a reason for hiding this comment

graaff Apr 11, 2024

Choose a reason for hiding this comment

koic Apr 16, 2024

Choose a reason for hiding this comment

bbatsov commented Apr 9, 2024

bbatsov commented Apr 16, 2024

amomchilov commented Apr 9, 2024 •

edited