diff --git a/README.md b/README.md index c492019..087264b 100644 --- a/README.md +++ b/README.md @@ -665,6 +665,10 @@ def autocorrect(_processed_source, offense) end ``` +If your linter's offenses depend on anything besides its configuration and the file being linted, such as another +config file or a gem's version, define a `cache_key` method that returns a string that changes whenever those inputs +do. Otherwise `--cache` keeps returning results computed with the old inputs. + ## Output formats You can change the output format of ERB Lint by specifying formatters with the `-f/--format` option. @@ -766,7 +770,8 @@ No errors were found in ERB files ``` Cached lint results are stored in the `.erb_lint_cache` directory by default, though a custom directory can be provided -via the `--cache-dir` option. Cache filenames are computed with a hash of information about the file and `erb_lint` settings. +via the `--cache-dir` option. Cache filenames are computed with a hash of information about the file, `erb_lint` settings, +and other inputs that linters depend on, such as the RuboCop configuration and version. These files store instance attributes of the `CachedOffense` object, which only contain the `Offense` attributes necessary to restore the results of running `erb_lint` for output. The cache also automatically prunes outdated files each time it's run. diff --git a/lib/erb_lint/cache.rb b/lib/erb_lint/cache.rb index 7746765..2b27314 100644 --- a/lib/erb_lint/cache.rb +++ b/lib/erb_lint/cache.rb @@ -67,6 +67,8 @@ def clear FileUtils.rm_r(@cache_dir) end + attr_writer :runner_cache_key + private attr_reader :config, :hits, :new_results @@ -76,7 +78,7 @@ def checksum(filename, file_content) mode = File.stat(filename).mode digester.update( - "#{mode}#{config.to_hash}#{ERBLint::VERSION}#{file_content}", + "#{mode}#{config.to_hash}#{ERBLint::VERSION}#{@runner_cache_key}#{file_content}", ) digester.hexdigest rescue Errno::ENOENT diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 2be7c92..1366dea 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -79,6 +79,9 @@ def run(args = ARGV) reporter.preview runner = ERBLint::Runner.new(file_loader, @config, @options[:disable_inline_configs]) + + @cache.runner_cache_key = runner.cache_key if cache? + file_content = nil lint_files.each do |filename| diff --git a/lib/erb_lint/linter.rb b/lib/erb_lint/linter.rb index 0c8ceea..9426563 100644 --- a/lib/erb_lint/linter.rb +++ b/lib/erb_lint/linter.rb @@ -70,6 +70,13 @@ def clear_offenses @offenses = [] end + # Returns a string that changes whenever something besides the linter's + # config and the linted file affects its offenses, such as another config + # file or a gem version, so that `--cache` doesn't reuse stale results. + def cache_key + nil + end + private def update_offense_status(processed_source) diff --git a/lib/erb_lint/linters/erb_safety.rb b/lib/erb_lint/linters/erb_safety.rb index 8bebe77..e984c82 100644 --- a/lib/erb_lint/linters/erb_safety.rb +++ b/lib/erb_lint/linters/erb_safety.rb @@ -32,6 +32,10 @@ def run(processed_source) end end + def cache_key + @file_loader.yaml(@config_filename).to_s unless @config_filename.nil? + end + private def tester_classes diff --git a/lib/erb_lint/linters/rubocop.rb b/lib/erb_lint/linters/rubocop.rb index abe8e3b..c578c52 100644 --- a/lib/erb_lint/linters/rubocop.rb +++ b/lib/erb_lint/linters/rubocop.rb @@ -46,8 +46,40 @@ def autocorrect(_processed_source, offense) end end + def cache_key + Digest::SHA1.hexdigest("#{rubocop_source_checksum}#{portable_rubocop_config}") + end + private + # RuboCop's own result cache hashes the contents of every loaded file, so + # this covers plugins, gems, and project-local cops loaded through + # `require:`. It's memoized, which is fine because the runner only asks for + # cache keys once every linter has loaded its config. Before RuboCop 1.82.1 + # it stays nil unless RuboCop's own cache computed it, so fall back to the + # versions of `rubocop*` gems. + def rubocop_source_checksum + ::RuboCop::ResultCache.source_checksum || rubocop_gem_versions.join(",") + end + + def rubocop_gem_versions + Gem.loaded_specs.each_value.filter_map { |spec| spec.full_name if spec.name.start_with?("rubocop") }.sort + end + + # RuboCop expands `Exclude` patterns to absolute paths, which would tie + # cached results to the directory the project is checked out in. + def portable_rubocop_config + base_path = File.expand_path(@file_loader.base_path) + @rubocop_config.to_hash.to_h do |name, config| + next [name, config] unless config.is_a?(Hash) && config["Exclude"].is_a?(Array) + + excludes = config["Exclude"].map do |pattern| + pattern.is_a?(String) ? ::RuboCop::PathUtil.relative_path(pattern, base_path) : pattern + end + [name, config.merge("Exclude" => excludes)] + end + end + def descendant_nodes(processed_source) processed_source.ast.descendants(:erb) end diff --git a/lib/erb_lint/runner.rb b/lib/erb_lint/runner.rb index ba399ee..cbd3ba6 100644 --- a/lib/erb_lint/runner.rb +++ b/lib/erb_lint/runner.rb @@ -42,6 +42,15 @@ def restore_offenses(offenses) @offenses.concat(offenses) end + def cache_key + digester = Digest::SHA1.new + @linters.each do |linter| + linter_cache_key = linter.cache_key + digester.update(linter_cache_key) if linter_cache_key + end + digester.hexdigest + end + private def enable_inline_configs? diff --git a/spec/erb_lint/cache_spec.rb b/spec/erb_lint/cache_spec.rb index 3a5eaab..fd93aec 100644 --- a/spec/erb_lint/cache_spec.rb +++ b/spec/erb_lint/cache_spec.rb @@ -70,6 +70,15 @@ end end + describe "#runner_cache_key=" do + it "is part of the checksum of cached results" do + cache.runner_cache_key = "runner-cache-key" + cache.get(linted_file_path, linted_file_content) + + expect(Digest::SHA1.new).to(have_received(:update).with(a_string_including("runner-cache-key"))) + end + end + describe "#cache_dir_exists?" do it "returns true if the cache dir exists" do expect(cache.cache_dir_exists?).to(be(true)) diff --git a/spec/erb_lint/cli_spec.rb b/spec/erb_lint/cli_spec.rb index 7c40a89..d81d36b 100644 --- a/spec/erb_lint/cli_spec.rb +++ b/spec/erb_lint/cli_spec.rb @@ -332,6 +332,16 @@ def run(processed_source) expect(Dir[ERBLint::Cache::CACHE_DIRECTORY].length).to(be(1)) end + + it "does not reuse cached results once a linter's cache key changes" do + allow_any_instance_of(ERBLint::Runner).to(receive(:cache_key).and_return("before")) + expect { described_class.new.run(args) }.to(output(/Cache mode is on/).to_stdout) + + allow_any_instance_of(ERBLint::Runner).to(receive(:cache_key).and_return("after")) + expect { described_class.new.run(args) }.to(output(/Cache being created for the first time/).to_stdout) + + expect(Dir.children(ERBLint::Cache::CACHE_DIRECTORY).size).to(eq(2)) + end end end end diff --git a/spec/erb_lint/linter_spec.rb b/spec/erb_lint/linter_spec.rb index 8f1f9b7..18a2e34 100644 --- a/spec/erb_lint/linter_spec.rb +++ b/spec/erb_lint/linter_spec.rb @@ -31,5 +31,11 @@ class Fake < ERBLint::Linter expect(linter.offenses).to(eq([])) end end + + describe "#cache_key" do + it "is nil by default" do + expect(linter.cache_key).to(be_nil) + end + end end end diff --git a/spec/erb_lint/linters/erb_safety_spec.rb b/spec/erb_lint/linters/erb_safety_spec.rb index 632f632..bd715cc 100644 --- a/spec/erb_lint/linters/erb_safety_spec.rb +++ b/spec/erb_lint/linters/erb_safety_spec.rb @@ -155,6 +155,24 @@ def yaml(_filename) end end + describe "#cache_key" do + let(:file) { "" } + + it "is nil without a better-html config file" do + expect(linter.cache_key).to(be_nil) + end + + context "with a better-html config file" do + let(:linter_config) { described_class.config_schema.new("better_html_config" => ".better-html.yml") } + + it "changes when the config file changes" do + changed_file_loader = MockFileLoader.new(javascript_safe_methods: ["foobar"]) + + expect(described_class.new(changed_file_loader, linter_config).cache_key).not_to(eq(linter.cache_key)) + end + end + end + private def unsafe_interpolate(range) diff --git a/spec/erb_lint/linters/rubocop_spec.rb b/spec/erb_lint/linters/rubocop_spec.rb index f969e81..669ff5d 100644 --- a/spec/erb_lint/linters/rubocop_spec.rb +++ b/spec/erb_lint/linters/rubocop_spec.rb @@ -335,6 +335,112 @@ it { expect(corrected_content).to(eq("<% dont_auto_correct_me(safe_method(dont_auto_correct_me)) %>\n")) } end + describe "#cache_key" do + let(:linter_config) do + described_class.config_schema.new(rubocop_config: { inherit_from: inherit_from_filename }) + end + let(:nested_config) { { "Layout/LineLength" => { "Max" => 100 } } } + let(:file) { "" } + + it "changes when an inherited RuboCop config changes" do + cache_key = linter.cache_key + allow(RuboCop::ConfigLoader).to( + receive(:load_file) + .with(a_string_ending_with(inherit_from_filename)) + .and_return({ "Layout/LineLength" => { "Max" => 120 } }), + ) + + expect(described_class.new(file_loader, linter_config).cache_key).not_to(eq(cache_key)) + end + + it "changes with RuboCop's source checksum" do + allow(RuboCop::ResultCache).to(receive(:source_checksum).and_return("one")) + cache_key = linter.cache_key + allow(RuboCop::ResultCache).to(receive(:source_checksum).and_return("two")) + + expect(linter.cache_key).not_to(eq(cache_key)) + end + + context "when RuboCop doesn't compute a source checksum" do + before { allow(RuboCop::ResultCache).to(receive(:source_checksum).and_return(nil)) } + + def stub_loaded_gems(versions) + loaded_specs = versions.to_h { |name, version| [name, Gem::Specification.new(name, version)] } + allow(Gem).to(receive(:loaded_specs).and_return(loaded_specs)) + end + + it "changes when a RuboCop gem is upgraded" do + stub_loaded_gems("rubocop" => "1.0.0", "rubocop-rails" => "2.0.0") + cache_key = linter.cache_key + stub_loaded_gems("rubocop" => "1.0.0", "rubocop-rails" => "2.0.1") + + expect(linter.cache_key).not_to(eq(cache_key)) + end + + it "does not change when other gems are upgraded" do + stub_loaded_gems("rubocop" => "1.0.0", "rake" => "13.0.0") + cache_key = linter.cache_key + stub_loaded_gems("rubocop" => "1.0.0", "rake" => "13.0.1") + + expect(linter.cache_key).to(eq(cache_key)) + end + end + + # `ResultCache.source_checksum` is memoized for the life of the process, so + # these compute each key in a fresh one, the way a CLI run does. + context "in a new process" do + def cache_key_in_new_process(required_file) + script = <<~RUBY + require "erb_lint/all" + config = ERBLint::Linters::Rubocop.config_schema.new(rubocop_config: { "require" => [#{required_file.inspect}] }) + linter = ERBLint::Linters::Rubocop.new(ERBLint::FileLoader.new(Dir.pwd), config) + puts "cache_key=\#{linter.cache_key}" + RUBY + lib_dir = File.expand_path("../../../lib", __dir__) + output = IO.popen([RbConfig.ruby, "-I", lib_dir, "-e", script], err: [:child, :out], &:read) + + cache_key = output[/^cache_key=(\h+)$/, 1] + expect(cache_key).not_to(be_nil, output) + cache_key + end + + def write_required_file(dir, content) + File.join(dir, "required_by_rubocop_config.rb").tap { |path| File.write(path, content) } + end + + it "is the same for identical required files at different paths" do + cache_keys = Array.new(2) do + Dir.mktmpdir { |dir| cache_key_in_new_process(write_required_file(dir, "ErbLintCacheKeySpec = 1\n")) } + end + + expect(cache_keys.first).to(eq(cache_keys.last)) + end + + it "changes when a file loaded through `require:` changes" do + if Gem::Version.new(RuboCop::Version::STRING) < Gem::Version.new("1.82.1") + skip("RuboCop #{RuboCop::Version::STRING} doesn't compute `ResultCache.source_checksum`") + end + + Dir.mktmpdir do |dir| + cache_key = cache_key_in_new_process(write_required_file(dir, "ErbLintCacheKeySpec = 1\n")) + + expect(cache_key_in_new_process(write_required_file(dir, "ErbLintCacheKeySpec = 2\n"))).not_to(eq(cache_key)) + end + end + end + + it "does not depend on where the project is checked out" do + linter_config = described_class.config_schema.new(rubocop_config: { AllCops: { Exclude: ["vendor/**/*"] } }) + cache_keys = Array.new(2) do + Dir.mktmpdir do |dir| + Dir.chdir(dir) { described_class.new(ERBLint::FileLoader.new(Dir.pwd), linter_config).cache_key } + end + end + + expect(cache_keys.first).to(eq(cache_keys.last)) + end + end + private def arbitrary_error_message(range) diff --git a/spec/erb_lint/runner_spec.rb b/spec/erb_lint/runner_spec.rb index a2a01e6..b1b085b 100644 --- a/spec/erb_lint/runner_spec.rb +++ b/spec/erb_lint/runner_spec.rb @@ -219,4 +219,37 @@ class FakeLinter4 < FakeLinter3; end end end end + + describe "#cache_key" do + module ERBLint + module Linters + class FakeLinterWithCacheKey < FakeLinter1 + def cache_key + "fake cache key" + end + end + end + end + + before do + allow(ERBLint::LinterRegistry).to(receive(:linters) + .and_return([ + ERBLint::Linters::FakeLinter1, + ERBLint::Linters::FakeLinterWithCacheKey, + ])) + end + + def cache_key_with(linters) + config = ERBLint::RunnerConfig.new(linters: linters.to_h { |name| [name, { "enabled" => true }] }) + described_class.new(file_loader, config).cache_key + end + + it "ignores linters without a cache key" do + expect(cache_key_with(["FakeLinter1"])).to(eq(cache_key_with([]))) + end + + it "includes the cache keys of enabled linters" do + expect(cache_key_with(["FakeLinter1", "FakeLinterWithCacheKey"])).not_to(eq(cache_key_with(["FakeLinter1"]))) + end + end end