From b57c88ac95a68d0b3a9a3a4f4988058dda690ffb Mon Sep 17 00:00:00 2001 From: Alexandre Terrasa Date: Fri, 9 Jun 2023 09:57:49 -0400 Subject: [PATCH 1/6] Make kind optional when deleting dead code at a specific location Signed-off-by: Alexandre Terrasa --- lib/spoom/deadcode/remover.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/spoom/deadcode/remover.rb b/lib/spoom/deadcode/remover.rb index dae28c0d..ff0ebdda 100644 --- a/lib/spoom/deadcode/remover.rb +++ b/lib/spoom/deadcode/remover.rb @@ -13,7 +13,7 @@ def initialize(context) @context = context end - sig { params(kind: Definition::Kind, location: Location).void } + sig { params(kind: T.nilable(Definition::Kind), location: Location).void } def remove_location(kind, location) file = location.file @@ -32,7 +32,7 @@ class NodeRemover sig { returns(String) } attr_reader :new_source - sig { params(source: String, kind: Definition::Kind, location: Location).void } + sig { params(source: String, kind: T.nilable(Definition::Kind), location: Location).void } def initialize(source, kind, location) @old_source = source @new_source = T.let(source.dup, String) @@ -296,7 +296,13 @@ def replace_chars(start_char, end_char, replacement) @new_source[start_char...end_char] = replacement end - sig { params(node: SyntaxTree::MethodAddBlock, name: String, kind: Definition::Kind).returns(String) } + sig do + params( + node: SyntaxTree::MethodAddBlock, + name: String, + kind: T.nilable(Definition::Kind), + ).returns(String) + end def transform_sig(node, name:, kind:) type = T.let(nil, T.nilable(String)) @@ -493,7 +499,7 @@ class NodeFinder < SyntaxTree::Visitor class << self extend T::Sig - sig { params(source: String, location: Location, kind: Definition::Kind).returns(NodeContext) } + sig { params(source: String, location: Location, kind: T.nilable(Definition::Kind)).returns(NodeContext) } def find(source, location, kind) tree = SyntaxTree.parse(source) @@ -505,7 +511,7 @@ def find(source, location, kind) raise Error, "Can't find node at #{location}" end - unless node_match_kind?(node, kind) + if kind && !node_match_kind?(node, kind) raise Error, "Can't find node at #{location}, expected #{kind} but got #{node.class}" end From 6fc2b3cfcb83e430f885fe7913caaf980c69645c Mon Sep 17 00:00:00 2001 From: Alexandre Terrasa Date: Fri, 1 Mar 2024 12:07:04 -0500 Subject: [PATCH 2/6] Do not write the file in `Deadcode::Remover` just return the new string Signed-off-by: Alexandre Terrasa --- lib/spoom/deadcode/remover.rb | 4 ++-- test/spoom/deadcode/remover_test.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/spoom/deadcode/remover.rb b/lib/spoom/deadcode/remover.rb index ff0ebdda..93a66dcf 100644 --- a/lib/spoom/deadcode/remover.rb +++ b/lib/spoom/deadcode/remover.rb @@ -13,7 +13,7 @@ def initialize(context) @context = context end - sig { params(kind: T.nilable(Definition::Kind), location: Location).void } + sig { params(kind: T.nilable(Definition::Kind), location: Location).returns(String) } def remove_location(kind, location) file = location.file @@ -23,7 +23,7 @@ def remove_location(kind, location) node_remover = NodeRemover.new(@context.read(file), kind, location) node_remover.apply_edit - @context.write!(file, node_remover.new_source) + node_remover.new_source end class NodeRemover diff --git a/test/spoom/deadcode/remover_test.rb b/test/spoom/deadcode/remover_test.rb index bd524fc8..812f938d 100644 --- a/test/spoom/deadcode/remover_test.rb +++ b/test/spoom/deadcode/remover_test.rb @@ -1584,11 +1584,11 @@ def remove(ruby_string, def_name) definition = T.must(definitions.first) remover = Remover.new(context) - remover.remove_location(definition.kind, definition.location) - res = context.read("file.rb") + new_source = remover.remove_location(definition.kind, definition.location) + context.destroy! - res + new_source end end end From 2b12014f12688a2ee1dede8f207337780fd26935 Mon Sep 17 00:00:00 2001 From: Alexandre Terrasa Date: Fri, 1 Mar 2024 12:07:24 -0500 Subject: [PATCH 3/6] Make it easier to json dump the dead code candidates Signed-off-by: Alexandre Terrasa --- lib/spoom/deadcode/definition.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/spoom/deadcode/definition.rb b/lib/spoom/deadcode/definition.rb index 0e0b560a..0fb84a25 100644 --- a/lib/spoom/deadcode/definition.rb +++ b/lib/spoom/deadcode/definition.rb @@ -93,6 +93,17 @@ def ignored? def ignored! @status = Status::IGNORED end + + # Utils + + sig { params(args: T.untyped).returns(String) } + def to_json(*args) + { + kind: kind, + name: name, + location: location.to_s, + }.to_json + end end end end From 9c9aed2bf53f1c9271fa1d5f12e2c6e0d82dd85f Mon Sep 17 00:00:00 2001 From: Alexandre Terrasa Date: Fri, 1 Mar 2024 12:08:36 -0500 Subject: [PATCH 4/6] Separate parsing and indexing Signed-off-by: Alexandre Terrasa --- lib/spoom/deadcode.rb | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/lib/spoom/deadcode.rb b/lib/spoom/deadcode.rb index a863a6db..abee7c7d 100644 --- a/lib/spoom/deadcode.rb +++ b/lib/spoom/deadcode.rb @@ -36,17 +36,35 @@ class IndexerError < Error; end class << self extend T::Sig - sig { params(index: Index, ruby: String, file: String, plugins: T::Array[Deadcode::Plugins::Base]).void } - def index_ruby(index, ruby, file:, plugins: []) - node = SyntaxTree.parse(ruby) - visitor = Spoom::Deadcode::Indexer.new(file, ruby, index, plugins: plugins) - visitor.visit(node) + sig { params(ruby: String, file: String).returns(SyntaxTree::Node) } + def parse_ruby(ruby, file:) + SyntaxTree.parse(ruby) rescue SyntaxTree::Parser::ParseError => e raise ParserError.new("Error while parsing #{file} (#{e.message} at #{e.lineno}:#{e.column})", parent: e) + end + + sig do + params( + index: Index, + node: SyntaxTree::Node, + ruby: String, + file: String, + plugins: T::Array[Deadcode::Plugins::Base], + ).void + end + def index_node(index, node, ruby, file:, plugins: []) + visitor = Spoom::Deadcode::Indexer.new(file, ruby, index, plugins: plugins) + visitor.visit(node) rescue => e raise IndexerError.new("Error while indexing #{file} (#{e.message})", parent: e) end + sig { params(index: Index, ruby: String, file: String, plugins: T::Array[Deadcode::Plugins::Base]).void } + def index_ruby(index, ruby, file:, plugins: []) + node = parse_ruby(ruby, file: file) + index_node(index, node, ruby, file: file, plugins: plugins) + end + sig { params(index: Index, erb: String, file: String, plugins: T::Array[Deadcode::Plugins::Base]).void } def index_erb(index, erb, file:, plugins: []) ruby = ERB.new(erb).src From 638b45b1d2ac92393a4f72eb736f826df10e9525 Mon Sep 17 00:00:00 2001 From: Alexandre Terrasa Date: Fri, 1 Mar 2024 12:08:57 -0500 Subject: [PATCH 5/6] Add CLI commands for dead code detection and removal Signed-off-by: Alexandre Terrasa --- lib/spoom/cli.rb | 4 + lib/spoom/cli/deadcode.rb | 172 ++++++++++++++++++++++++++++++ test/spoom/cli/cli_test.rb | 1 + test/spoom/cli/deadcode_test.rb | 181 ++++++++++++++++++++++++++++++++ 4 files changed, 358 insertions(+) create mode 100644 lib/spoom/cli/deadcode.rb create mode 100644 test/spoom/cli/deadcode_test.rb diff --git a/lib/spoom/cli.rb b/lib/spoom/cli.rb index 00f32f5e..07d45f0e 100644 --- a/lib/spoom/cli.rb +++ b/lib/spoom/cli.rb @@ -6,6 +6,7 @@ require_relative "cli/helper" require_relative "cli/bump" +require_relative "cli/deadcode" require_relative "cli/lsp" require_relative "cli/coverage" require_relative "cli/run" @@ -27,6 +28,9 @@ class Main < Thor desc "coverage", "Collect metrics related to Sorbet coverage" subcommand "coverage", Spoom::Cli::Coverage + desc "deadcode", "Analyze code to find deadcode" + subcommand "deadcode", Spoom::Cli::Deadcode + desc "lsp", "Send LSP requests to Sorbet" subcommand "lsp", Spoom::Cli::LSP diff --git a/lib/spoom/cli/deadcode.rb b/lib/spoom/cli/deadcode.rb new file mode 100644 index 00000000..4901ee6d --- /dev/null +++ b/lib/spoom/cli/deadcode.rb @@ -0,0 +1,172 @@ +# typed: true +# frozen_string_literal: true + +require_relative "../deadcode" + +module Spoom + module Cli + class Deadcode < Thor + extend T::Sig + include Helper + + default_task :deadcode + + desc "deadcode PATH...", "Analyze PATHS to find dead code" + option :allowed_extensions, + type: :array, + default: [".rb", ".erb", ".gemspec"], + aliases: :e, + desc: "Allowed extensions" + option :allowed_mime_types, + type: :array, + default: ["text/x-ruby", "text/x-ruby-script"], + aliases: :m, + desc: "Allowed mime types" + option :exclude, + type: :array, + default: ["vendor/", "sorbet/"], + aliases: :x, + desc: "Exclude paths" + option :show_files, + type: :boolean, + default: false, + desc: "Show the files that will be analyzed" + option :show_plugins, + type: :boolean, + default: false, + desc: "Show the loaded plugins" + option :show_defs, + type: :boolean, + default: false, + desc: "Show the indexed definitions" + option :show_refs, + type: :boolean, + default: false, + desc: "Show the indexed references" + option :sort, + type: :string, + default: "name", + enum: ["name", "location"], + desc: "Sort the output by name or location" + sig { params(paths: String).void } + def deadcode(*paths) + context = self.context + + paths << exec_path if paths.empty? + + $stderr.puts "Collecting files..." + collector = FileCollector.new( + allow_extensions: options[:allowed_extensions], + allow_mime_types: options[:allowed_mime_types], + exclude_patterns: options[:exclude].map { |p| Pathname.new(File.join(exec_path, p, "**")).cleanpath.to_s }, + ) + collector.visit_paths(paths) + files = collector.files.sort + + if options[:show_files] + $stderr.puts "\nCollected #{blue(files.size.to_s)} files for analysis\n" + files.each do |file| + $stderr.puts " #{gray(file)}" + end + $stderr.puts + end + + plugins = Spoom::Deadcode.plugins_from_gemfile_lock(context) + if options[:show_plugins] + $stderr.puts "\nLoaded #{blue(plugins.size.to_s)} plugins\n" + plugins.each do |plugin| + $stderr.puts " #{gray(plugin.class.to_s)}" + end + $stderr.puts + end + + index = Spoom::Deadcode::Index.new + + $stderr.puts "Indexing #{blue(files.size.to_s)} files..." + files.each do |file| + content = File.read(file) + content = ERB.new(content).src if file.end_with?(".erb") + + tree = Spoom::Deadcode.parse_ruby(content, file: file) + Spoom::Deadcode.index_node(index, tree, content, file: file, plugins: plugins) + rescue Spoom::Deadcode::ParserError => e + say_error("Error parsing #{file}: #{e.message}") + next + rescue Spoom::Deadcode::IndexerError => e + say_error("Error indexing #{file}: #{e.message}") + next + end + + if options[:show_defs] + $stderr.puts "\nDefinitions:" + index.definitions.each do |name, definitions| + $stderr.puts " #{blue(name)}" + definitions.each do |definition| + $stderr.puts " #{yellow(definition.kind.serialize)} #{gray(definition.location.to_s)}" + end + end + $stderr.puts + end + + if options[:show_refs] + $stderr.puts "\nReferences:" + index.references.values.flatten.sort_by(&:name).each do |references| + name = references.name + kind = references.kind.serialize + loc = references.location.to_s + $stderr.puts " #{blue(name)} #{yellow(kind)} #{gray(loc)}" + end + $stderr.puts + end + + definitions_count = index.definitions.size.to_s + references_count = index.references.size.to_s + $stderr.puts "Analyzing #{blue(definitions_count)} definitions against #{blue(references_count)} references..." + + index.finalize! + dead = index.definitions.values.flatten.select(&:dead?) + + if options[:sort] == "name" + dead.sort_by!(&:name) + else + dead.sort_by!(&:location) + end + + if dead.empty? + $stderr.puts "\n#{green("No dead code found!")}" + else + $stderr.puts "\nCandidates:" + dead.each do |definition| + $stderr.puts " #{red(definition.full_name)} #{gray(definition.location.to_s)}" + end + $stderr.puts "\n" + $stderr.puts red(" Found #{dead.size} dead candidates") + + exit(1) + end + end + + desc "remove LOCATION", "Remove dead code at LOCATION" + def remove(location_string) + location = Spoom::Deadcode::Location.from_string(location_string) + context = self.context + remover = Spoom::Deadcode::Remover.new(context) + + new_source = remover.remove_location(nil, location) + context.write!("PATCH", new_source) + + diff = context.exec("diff -u #{location.file} PATCH") + $stderr.puts T.must(diff.out.lines[2..-1]).join + context.remove!("PATCH") + + context.write!(location.file, new_source) + rescue Spoom::Deadcode::Remover::Error => e + say_error("Can't remove code at #{location_string}: #{e.message}") + exit(1) + rescue Spoom::Deadcode::Location::LocationError => e + say_error(e.message) + exit(1) + end + end + end +end diff --git a/test/spoom/cli/cli_test.rb b/test/spoom/cli/cli_test.rb index 9ed32900..e09e913e 100644 --- a/test/spoom/cli/cli_test.rb +++ b/test/spoom/cli/cli_test.rb @@ -27,6 +27,7 @@ def test_display_help_long_option spoom --version # Show version spoom bump # Bump Sorbet sigils from `false` to `true` when no errors spoom coverage # Collect metrics related to Sorbet coverage + spoom deadcode # Analyze code to find deadcode spoom help [COMMAND] # Describe available commands or one specific command spoom lsp # Send LSP requests to Sorbet spoom tc # Run Sorbet and parses its output diff --git a/test/spoom/cli/deadcode_test.rb b/test/spoom/cli/deadcode_test.rb new file mode 100644 index 00000000..27025c06 --- /dev/null +++ b/test/spoom/cli/deadcode_test.rb @@ -0,0 +1,181 @@ +# typed: true +# frozen_string_literal: true + +require "test_with_project" + +module Spoom + module Cli + class DeadcodeTest < TestWithProject + def test_deadcode_without_deadcode + @project.write!("lib/foo.rb", <<~RUBY) + def foo; end + def bar; end + def baz; end + + foo; bar; baz + RUBY + + result = @project.spoom("deadcode --no-color") + assert_equal(<<~ERR, result.err) + Collecting files... + Indexing 1 files... + Analyzing 3 definitions against 3 references... + + No dead code found! + ERR + assert_empty(result.out) + assert(result.status) + end + + def test_deadcode_with_deadcode + @project.write!("lib/foo.rb", <<~RUBY) + def foo; end + def bar; end + def baz; end + foo; bar + RUBY + + result = @project.spoom("deadcode --no-color") + assert_equal(<<~ERR, result.err) + Collecting files... + Indexing 1 files... + Analyzing 3 definitions against 2 references... + + Candidates: + baz lib/foo.rb:3:0-3:12 + + Found 1 dead candidates + ERR + assert_empty(result.out) + refute(result.status) + end + + def test_deadcode_show_files + @project.write!("lib/foo.rb", <<~RUBY) + def foo; end + def bar; end + def baz; end + RUBY + + @project.write!("lib/bar.rb", <<~RUBY) + foo; bar; baz + RUBY + + result = @project.spoom("deadcode --show-files --no-color") + assert_equal(<<~ERR, result.err) + Collecting files... + + Collected 2 files for analysis + lib/bar.rb + lib/foo.rb + + Indexing 2 files... + Analyzing 3 definitions against 3 references... + + No dead code found! + ERR + assert_empty(result.out) + assert(result.status) + end + + def test_deadcode_show_defs + @project.write!("lib/foo.rb", <<~RUBY) + def foo; end + def bar; end + def baz; end + + foo; bar; baz + RUBY + + result = @project.spoom("deadcode --show-defs --no-color") + assert_equal(<<~ERR, result.err) + Collecting files... + Indexing 1 files... + + Definitions: + foo + method lib/foo.rb:1:0-1:12 + bar + method lib/foo.rb:2:0-2:12 + baz + method lib/foo.rb:3:0-3:12 + + Analyzing 3 definitions against 3 references... + + No dead code found! + ERR + assert_empty(result.out) + assert(result.status) + end + + def test_deadcode_show_refs + @project.write!("lib/foo.rb", <<~RUBY) + def foo; end + def bar; end + def baz; end + + foo; bar; baz + RUBY + + result = @project.spoom("deadcode --show-refs --no-color") + assert_equal(<<~ERR, result.err) + Collecting files... + Indexing 1 files... + + References: + bar method lib/foo.rb:5:5-5:8 + baz method lib/foo.rb:5:10-5:13 + foo method lib/foo.rb:5:0-5:3 + + Analyzing 3 definitions against 3 references... + + No dead code found! + ERR + assert_empty(result.out) + assert(result.status) + end + + def test_deadcode_show_plugins_default + result = @project.spoom("deadcode --show-plugins --no-color") + assert_equal(<<~ERR, result.err) + Collecting files... + + Loaded 6 plugins + Spoom::Deadcode::Plugins::Namespaces + Spoom::Deadcode::Plugins::Ruby + Spoom::Deadcode::Plugins::Minitest + Spoom::Deadcode::Plugins::Rake + Spoom::Deadcode::Plugins::Sorbet + Spoom::Deadcode::Plugins::Thor + + Indexing 0 files... + Analyzing 0 definitions against 0 references... + + No dead code found! + ERR + assert_empty(result.out) + assert(result.status) + end + + def test_remove + @project.write!("lib/foo.rb", <<~RUBY) + def foo; end + def bar; end + def baz; end + RUBY + + result = @project.spoom("deadcode remove lib/foo.rb:2:0-2:12 --no-color") + + assert_equal(<<~ERR, result.err) + @@ -1,3 +1,2 @@ + def foo; end + -def bar; end + def baz; end + ERR + + assert_empty(result.out) + assert(result.status) + end + end + end +end From 67b42e896bef1499e6e6f80f19eaa023018cf661 Mon Sep 17 00:00:00 2001 From: Alexandre Terrasa Date: Fri, 1 Mar 2024 12:13:17 -0500 Subject: [PATCH 6/6] Add some documentation Signed-off-by: Alexandre Terrasa --- README.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/README.md b/README.md index 2bdc67cb..fd1d732f 100644 --- a/README.md +++ b/README.md @@ -341,6 +341,22 @@ require "spoom/backtrace_filter/minitest" Minitest.backtrace_filter = Spoom::BacktraceFilter::Minitest.new ``` +### Dead code removal + +Run dead code detection in your project with: + +``` +$ spoom deadcode +``` + +This will list all the methods and constants that do not appear to be used in your project. + +You can remove them with Spoom: + +``` +$ spoom deadcode remove path/to/file.rb:42:18-47:23 +``` + ## Development After checking out the repo, run `bin/setup` to install dependencies. Then, run `bin/test` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. Don't forget to run `bin/sanity` before pushing your changes.