From 13925f860f1e85d5d4389546891bf9d17eb0668f Mon Sep 17 00:00:00 2001 From: Ilya Bylich Date: Tue, 5 May 2020 03:58:39 +0300 Subject: [PATCH 1/3] * fixed all warnings. test are running in verbose mode now. --- .travis.yml | 1 - Rakefile | 2 +- lib/parser/diagnostic.rb | 2 +- lib/parser/lexer.rl | 7 ++++++ lib/parser/messages.rb | 15 +++++++++++++ lib/parser/source/tree_rewriter.rb | 2 +- test/helper.rb | 8 +++---- test/parse_helper.rb | 10 ++++----- test/test_diagnostic.rb | 2 +- test/test_diagnostic_engine.rb | 13 +++++------ test/test_lexer.rb | 2 +- test/test_parser.rb | 2 +- test/test_runner_parse.rb | 2 +- test/test_runner_rewrite.rb | 2 +- test/test_source_buffer.rb | 2 +- test/test_source_comment_associator.rb | 30 +++++++++++++------------- test/test_source_range.rb | 14 ++++++------ test/test_source_rewriter.rb | 4 ++-- test/test_source_tree_rewriter.rb | 2 +- 19 files changed, 68 insertions(+), 54 deletions(-) diff --git a/.travis.yml b/.travis.yml index 63164edb8..278188c96 100644 --- a/.travis.yml +++ b/.travis.yml @@ -37,6 +37,5 @@ matrix: - rvm: rbx-2 - script: ./ci/run_rubocop_specs before_install: - - gem install bundler -v '< 2' - bundle --version - gem --version diff --git a/Rakefile b/Rakefile index 3ddf24d50..449b91509 100644 --- a/Rakefile +++ b/Rakefile @@ -11,7 +11,7 @@ task :default => [:test] Rake::TestTask.new do |t| t.libs = %w(test/ lib/) t.test_files = FileList["test/**/test_*.rb"] - t.warning = false + t.warning = true end task :test_cov do diff --git a/lib/parser/diagnostic.rb b/lib/parser/diagnostic.rb index 3c8fce20e..535e00e57 100644 --- a/lib/parser/diagnostic.rb +++ b/lib/parser/diagnostic.rb @@ -67,7 +67,7 @@ def initialize(level, reason, arguments, location, highlights=[]) # @return [String] the rendered message. # def message - MESSAGES[@reason] % @arguments + Messages.format(@reason, @arguments) end ## diff --git a/lib/parser/lexer.rl b/lib/parser/lexer.rl index 6af244330..19f43eb9d 100644 --- a/lib/parser/lexer.rl +++ b/lib/parser/lexer.rl @@ -283,6 +283,13 @@ class Parser::Lexer %% write exec; # % + # Ragel creates a local variable called `testEof` but it doesn't use + # it any assignment. This dead code is here to swallow the warning. + # It has no runtime cost because Ruby doesn't produce any instructions from it. + if false + testEof + end + @p = p if @token_queue.any? diff --git a/lib/parser/messages.rb b/lib/parser/messages.rb index e131f92c1..eb1720f16 100644 --- a/lib/parser/messages.rb +++ b/lib/parser/messages.rb @@ -93,4 +93,19 @@ module Parser :crossing_insertions => 'the rewriting action on:', :crossing_insertions_conflict => 'is crossing that on:', }.freeze + + # @api private + module Messages + # Formats the message, returns a raw template if there's nothing to interpolate + # + # Code like `"" % {}` gives a warning, and so this method tries interpolating + # only if `arguments` hash is not empty. + # + # @api private + def self.format(reason, arguments) + template = MESSAGES[reason] + return template if Hash === arguments && arguments.empty? + template % arguments + end + end end diff --git a/lib/parser/source/tree_rewriter.rb b/lib/parser/source/tree_rewriter.rb index 6f61a39b3..1baf7ba09 100644 --- a/lib/parser/source/tree_rewriter.rb +++ b/lib/parser/source/tree_rewriter.rb @@ -221,7 +221,7 @@ def insert_after(range, content) # # @return [String] # - def process + def process source = @source_buffer.source chunks = [] diff --git a/test/helper.rb b/test/helper.rb index 9bb6d9f6d..4d119f66f 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true require 'tempfile' -require 'minitest/test' - require 'simplecov' if ENV.include?('COVERAGE') && SimpleCov.usable? @@ -28,9 +26,9 @@ at_exit { RaccCoverage.stop } SimpleCov.start do - self.formatter = SimpleCov::Formatter::MultiFormatter[ - SimpleCov::Formatter::HTMLFormatter, - ] + self.formatter = SimpleCov::Formatter::MultiFormatter.new( + SimpleCov::Formatter::HTMLFormatter + ) add_group 'Grammars' do |source_file| source_file.filename =~ %r{\.y$} diff --git a/test/parse_helper.rb b/test/parse_helper.rb index b2ea5daa6..85a235ea7 100644 --- a/test/parse_helper.rb +++ b/test/parse_helper.rb @@ -106,8 +106,7 @@ def try_parsing(ast, code, parser, source_maps, version) assert_equal ast, parsed_ast, "(#{version}) AST equality" - parse_source_map_descriptions(source_maps) \ - do |begin_pos, end_pos, map_field, ast_path, line| + parse_source_map_descriptions(source_maps) do |begin_pos, end_pos, map_field, ast_path, line| astlet = traverse_ast(parsed_ast, ast_path) @@ -160,15 +159,14 @@ def assert_diagnoses(diagnostic, code, source_maps='', versions=ALL_VERSIONS) level, reason, arguments = diagnostic arguments ||= {} - message = Parser::MESSAGES[reason] % arguments + message = Parser::Messages.format(reason, arguments) assert_equal level, emitted_diagnostic.level assert_equal reason, emitted_diagnostic.reason assert_equal arguments, emitted_diagnostic.arguments assert_equal message, emitted_diagnostic.message - parse_source_map_descriptions(source_maps) \ - do |begin_pos, end_pos, map_field, ast_path, line| + parse_source_map_descriptions(source_maps) do |begin_pos, end_pos, map_field, ast_path, line| case map_field when 'location' @@ -216,7 +214,7 @@ def assert_diagnoses_many(diagnostics, code, versions=ALL_VERSIONS) diagnostics.zip(@diagnostics) do |expected_diagnostic, actual_diagnostic| level, reason, arguments = expected_diagnostic arguments ||= {} - message = Parser::MESSAGES[reason] % arguments + message = Parser::Messages.format(reason, arguments) assert_equal level, actual_diagnostic.level assert_equal reason, actual_diagnostic.reason diff --git a/test/test_diagnostic.rb b/test/test_diagnostic.rb index 400af1ab1..fa0c0046c 100644 --- a/test/test_diagnostic.rb +++ b/test/test_diagnostic.rb @@ -16,7 +16,7 @@ def test_verifies_levels Parser::Diagnostic.new(:foobar, :escape_eof, {}, @range1) end - assert_match /level/, error.message + assert_match(/level/, error.message) end def test_freezes diff --git a/test/test_diagnostic_engine.rb b/test/test_diagnostic_engine.rb index bc31079d8..afdecc254 100644 --- a/test/test_diagnostic_engine.rb +++ b/test/test_diagnostic_engine.rb @@ -4,9 +4,6 @@ class TestDiagnosticEngine < Minitest::Test def setup - @buffer = Parser::Source::Buffer.new('(source)') - @buffer.source = 'foobar' - @engine = Parser::Diagnostic::Engine.new @queue = [] @@ -14,7 +11,7 @@ def setup end def test_process_warnings - warn = Parser::Diagnostic.new(:warning, :invalid_escape, @buffer, 1..2) + warn = Parser::Diagnostic.new(:warning, :invalid_escape, {}, 1..2) @engine.process(warn) assert_equal [warn], @queue @@ -23,7 +20,7 @@ def test_process_warnings def test_ignore_warnings @engine.ignore_warnings = true - warn = Parser::Diagnostic.new(:warning, :invalid_escape, @buffer, 1..2) + warn = Parser::Diagnostic.new(:warning, :invalid_escape, {}, 1..2) @engine.process(warn) assert_equal [], @queue @@ -32,7 +29,7 @@ def test_ignore_warnings def test_all_errors_are_fatal @engine.all_errors_are_fatal = true - error = Parser::Diagnostic.new(:error, :invalid_escape, @buffer, 1..2) + error = Parser::Diagnostic.new(:error, :invalid_escape, {}, 1..2) err = assert_raises Parser::SyntaxError do @engine.process(error) @@ -44,14 +41,14 @@ def test_all_errors_are_fatal end def test_all_errors_are_collected - error = Parser::Diagnostic.new(:error, :invalid_escape, @buffer, 1..2) + error = Parser::Diagnostic.new(:error, :invalid_escape, {}, 1..2) @engine.process(error) assert_equal [error], @queue end def test_fatal_error - fatal = Parser::Diagnostic.new(:fatal, :invalid_escape, @buffer, 1..2) + fatal = Parser::Diagnostic.new(:fatal, :invalid_escape, {}, 1..2) assert_raises Parser::SyntaxError do @engine.process(fatal) diff --git a/test/test_lexer.rb b/test/test_lexer.rb index 9f98d2639..0cdedf97c 100644 --- a/test/test_lexer.rb +++ b/test/test_lexer.rb @@ -3606,7 +3606,7 @@ def assert_scanned_numbered_parameter(input) def refute_scanned_numbered_parameter(input, message = nil) err = assert_raises Parser::SyntaxError do - lex_token, (lex_value, lex_range) = lex_numbered_parameter(input) + _lex_token, (_lex_value, _lex_range) = lex_numbered_parameter(input) end if message diff --git a/test/test_parser.rb b/test/test_parser.rb index 92f6ddfa5..7b672387e 100644 --- a/test/test_parser.rb +++ b/test/test_parser.rb @@ -9238,7 +9238,7 @@ def assert_pattern_matching_defines_local_variables(match_code, lvar_names, vers before = parser.static_env.instance_variable_get(:@variables).to_a begin - parsed_ast = parser.parse(source_file) + _parsed_ast = parser.parse(source_file) rescue Parser::SyntaxError => exc backtrace = exc.backtrace Exception.instance_method(:initialize).bind(exc). diff --git a/test/test_runner_parse.rb b/test/test_runner_parse.rb index da256471a..f4fbe2733 100644 --- a/test/test_runner_parse.rb +++ b/test/test_runner_parse.rb @@ -7,7 +7,7 @@ class TestRunnerParse < Minitest::Test PATH_TO_RUBY_PARSE = File.expand_path('../bin/ruby-parse', __dir__).freeze def assert_prints(argv, expected_output) - stdout, stderr, status = Open3.capture3(PATH_TO_RUBY_PARSE, *argv) + stdout, _stderr, status = Open3.capture3(PATH_TO_RUBY_PARSE, *argv) assert_equal 0, status.to_i assert_includes(stdout, expected_output) diff --git a/test/test_runner_rewrite.rb b/test/test_runner_rewrite.rb index 46ee7bab3..4a7676628 100644 --- a/test/test_runner_rewrite.rb +++ b/test/test_runner_rewrite.rb @@ -21,7 +21,7 @@ def assert_rewriter_output(path, args, input: 'input.rb', output: 'output.rb', e expected_file = @fixtures_dir + output FileUtils.cp(@fixtures_dir + input, sample_file_expanded) - stdout, stderr, exit_code = Dir.chdir @test_dir do + stdout, stderr, _exit_code = Dir.chdir @test_dir do Open3.capture3 %Q{ #{Shellwords.escape(@ruby_rewrite.to_s)} #{args} \ #{Shellwords.escape(sample_file_expanded.to_s)} diff --git a/test/test_source_buffer.rb b/test/test_source_buffer.rb index 6a6196d63..c75ecb768 100644 --- a/test/test_source_buffer.rb +++ b/test/test_source_buffer.rb @@ -45,7 +45,7 @@ def test_source_setter_encoding_error ].join("\n") end - assert_match /invalid byte sequence in UTF\-8/, error.message + assert_match(/invalid byte sequence in UTF\-8/, error.message) end def test_read diff --git a/test/test_source_comment_associator.rb b/test/test_source_comment_associator.rb index 4e40cc471..db4284822 100644 --- a/test/test_source_comment_associator.rb +++ b/test/test_source_comment_associator.rb @@ -95,7 +95,7 @@ def bar end END - klass_node = ast + _klass_node = ast method_node = ast.children[2] body = method_node.children[2] f1_1_node = body.children[0] @@ -175,7 +175,7 @@ def bar end END - klass_node = ast + _klass_node = ast method_node = ast.children[2] body = method_node.children[2] f1_1_node = body.children[0] @@ -201,12 +201,12 @@ class Foo end def test_associate_empty_tree - ast, associations = associate("") + _ast, associations = associate("") assert_equal 0, associations.size end def test_associate_shebang_only - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) #!ruby class Foo end @@ -216,7 +216,7 @@ class Foo end def test_associate_frozen_string_literal - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # frozen_string_literal: true class Foo end @@ -226,7 +226,7 @@ class Foo end def test_associate_frozen_string_literal_dash_star_dash - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # -*- frozen_string_literal: true -*- class Foo end @@ -236,7 +236,7 @@ class Foo end def test_associate_frozen_string_literal_no_space_after_colon - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # frozen_string_literal:true class Foo end @@ -246,7 +246,7 @@ class Foo end def test_associate_warn_indent - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # warn_indent: true class Foo end @@ -256,7 +256,7 @@ class Foo end def test_associate_warn_indent_dash_star_dash - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # -*- warn_indent: true -*- class Foo end @@ -266,7 +266,7 @@ class Foo end def test_associate_warn_past_scope - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # warn_past_scope: true class Foo end @@ -276,7 +276,7 @@ class Foo end def test_associate_warn_past_scope_dash_star_dash - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # -*- warn_past_scope: true -*- class Foo end @@ -286,7 +286,7 @@ class Foo end def test_associate_multiple - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # frozen_string_literal: true; warn_indent: true class Foo end @@ -296,7 +296,7 @@ class Foo end def test_associate_multiple_dash_star_dash - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) # -*- frozen_string_literal: true; warn_indent: true -*- class Foo end @@ -306,7 +306,7 @@ class Foo end def test_associate_no_comments - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) class Foo end END @@ -315,7 +315,7 @@ class Foo end def test_associate_comments_after_root_node - ast, associations = associate(<<-END) + _ast, associations = associate(<<-END) class Foo end # not associated diff --git a/test/test_source_range.rb b/test/test_source_range.rb index 605b0132e..50e99b53f 100644 --- a/test/test_source_range.rb +++ b/test/test_source_range.rb @@ -95,15 +95,15 @@ def test_containment def test_order assert_equal 0, @sr1_3 <=> @sr1_3 - assert_equal -1, @sr1_3 <=> @sr5_8 - assert_equal -1, @sr2_2 <=> @sr2_6 - assert_equal +1, @sr2_6 <=> @sr2_2 + assert_equal(-1, @sr1_3 <=> @sr5_8) + assert_equal(-1, @sr2_2 <=> @sr2_6) + assert_equal(+1, @sr2_6 <=> @sr2_2) - assert_equal -1, @sr1_3 <=> @sr2_6 + assert_equal(-1, @sr1_3 <=> @sr2_6) - assert_equal +1, @sr2_2 <=> @sr1_3 - assert_equal -1, @sr1_3 <=> @sr2_2 - assert_equal -1, @sr5_7 <=> @sr5_8 + assert_equal(+1, @sr2_2 <=> @sr1_3) + assert_equal(-1, @sr1_3 <=> @sr2_2) + assert_equal(-1, @sr5_7 <=> @sr5_8) assert_nil @sr1_3 <=> Parser::Source::Range.new(@buf.dup, 1, 3) assert_nil @sr1_3 <=> 4 diff --git a/test/test_source_rewriter.rb b/test/test_source_rewriter.rb index 0ef169244..949b7ecfe 100644 --- a/test/test_source_rewriter.rb +++ b/test/test_source_rewriter.rb @@ -522,7 +522,7 @@ def test_nested_transaction_raises_error end end - assert_match /nested/i, error.message + assert_match(/nested/i, error.message) end def test_process_in_transaction_raises_error @@ -532,7 +532,7 @@ def test_process_in_transaction_raises_error end end - assert_match /transaction/, error.message + assert_match(/transaction/, error.message) end def silence_diagnostics diff --git a/test/test_source_tree_rewriter.rb b/test/test_source_tree_rewriter.rb index a423a6bdc..c07d32f4a 100644 --- a/test/test_source_tree_rewriter.rb +++ b/test/test_source_tree_rewriter.rb @@ -36,7 +36,7 @@ def build(actions, **policy) else diags.call end - rescue ::Parser::ClobberingError => e + rescue ::Parser::ClobberingError => _e [::Parser::ClobberingError, diags.call] end From 49f891d3bcf59b657627308d0b19216f1181fec4 Mon Sep 17 00:00:00 2001 From: Ilya Bylich Date: Tue, 5 May 2020 04:06:41 +0300 Subject: [PATCH 2/3] also bump rake to avoid warnings on CI --- parser.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parser.gemspec b/parser.gemspec index 438dcb10a..fe30ebd9e 100644 --- a/parser.gemspec +++ b/parser.gemspec @@ -45,7 +45,7 @@ Gem::Specification.new do |spec| spec.add_dependency 'ast', '~> 2.4.0' spec.add_development_dependency 'bundler', '>= 1.15', '< 3.0.0' - spec.add_development_dependency 'rake', '~> 10.0' + spec.add_development_dependency 'rake', '~> 13.0.1' spec.add_development_dependency 'racc', '= 1.4.15' spec.add_development_dependency 'cliver', '~> 0.3.2' From e9b666c55d76ba7d67ad204f68822ba327fd3342 Mon Sep 17 00:00:00 2001 From: Ilya Bylich Date: Tue, 5 May 2020 12:01:43 +0300 Subject: [PATCH 3/3] use Kernel#format instead of String#%. --- lib/parser/diagnostic.rb | 2 +- lib/parser/lexer.rl | 2 +- lib/parser/messages.rb | 6 +++--- test/parse_helper.rb | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/parser/diagnostic.rb b/lib/parser/diagnostic.rb index 535e00e57..e9f1b0a94 100644 --- a/lib/parser/diagnostic.rb +++ b/lib/parser/diagnostic.rb @@ -67,7 +67,7 @@ def initialize(level, reason, arguments, location, highlights=[]) # @return [String] the rendered message. # def message - Messages.format(@reason, @arguments) + Messages.compile(@reason, @arguments) end ## diff --git a/lib/parser/lexer.rl b/lib/parser/lexer.rl index 19f43eb9d..0be39067f 100644 --- a/lib/parser/lexer.rl +++ b/lib/parser/lexer.rl @@ -284,7 +284,7 @@ class Parser::Lexer # % # Ragel creates a local variable called `testEof` but it doesn't use - # it any assignment. This dead code is here to swallow the warning. + # it in any assignment. This dead code is here to swallow the warning. # It has no runtime cost because Ruby doesn't produce any instructions from it. if false testEof diff --git a/lib/parser/messages.rb b/lib/parser/messages.rb index eb1720f16..a80c7b61d 100644 --- a/lib/parser/messages.rb +++ b/lib/parser/messages.rb @@ -98,14 +98,14 @@ module Parser module Messages # Formats the message, returns a raw template if there's nothing to interpolate # - # Code like `"" % {}` gives a warning, and so this method tries interpolating + # Code like `format("", {})` gives a warning, and so this method tries interpolating # only if `arguments` hash is not empty. # # @api private - def self.format(reason, arguments) + def self.compile(reason, arguments) template = MESSAGES[reason] return template if Hash === arguments && arguments.empty? - template % arguments + format(template, arguments) end end end diff --git a/test/parse_helper.rb b/test/parse_helper.rb index 85a235ea7..2c50eb42d 100644 --- a/test/parse_helper.rb +++ b/test/parse_helper.rb @@ -159,7 +159,7 @@ def assert_diagnoses(diagnostic, code, source_maps='', versions=ALL_VERSIONS) level, reason, arguments = diagnostic arguments ||= {} - message = Parser::Messages.format(reason, arguments) + message = Parser::Messages.compile(reason, arguments) assert_equal level, emitted_diagnostic.level assert_equal reason, emitted_diagnostic.reason @@ -214,7 +214,7 @@ def assert_diagnoses_many(diagnostics, code, versions=ALL_VERSIONS) diagnostics.zip(@diagnostics) do |expected_diagnostic, actual_diagnostic| level, reason, arguments = expected_diagnostic arguments ||= {} - message = Parser::Messages.format(reason, arguments) + message = Parser::Messages.compile(reason, arguments) assert_equal level, actual_diagnostic.level assert_equal reason, actual_diagnostic.reason