From e7afb9f4578d18e9326d68e8f6e7d5d2189b6f1e Mon Sep 17 00:00:00 2001 From: schneems Date: Sun, 31 Oct 2021 20:50:08 -0500 Subject: [PATCH 01/10] Lex based banner Described in https://github.com/zombocom/dead_end/issues/68#issuecomment-955575369. This implements a method of checking the source code for logically missing component pairs: - {} - [] - || - () - keyword/end It seems to be much more robust than using a regex on the parser errors. It also seems to provide more confident results than the parser errors. For cases where we are not confident about why code is invalid, we still fall back on the parser error messages. That allows this functionality to act as a progressive enhancement rather than a wholesale replacement. We may be able to specialize other checks in the future as well. The most common one for me that comes up is missing a trailing comma in a hash/array/method-call: ``` query = Cutlass::FunctionQuery.new( port: port # <== here body: body ).call ``` --- README.md | 110 +++++++-- lib/dead_end.rb | 5 +- lib/dead_end/display_invalid_blocks.rb | 23 +- lib/dead_end/explain_syntax.rb | 105 ++++++++ lib/dead_end/left_right_lex_count.rb | 159 ++++++++++++ spec/integration/exe_cli_spec.rb | 2 + .../improvement_regression_spec.rb | 6 +- spec/spec_helper.rb | 9 + spec/unit/explain_syntax_spec.rb | 230 ++++++++++++++++++ 9 files changed, 616 insertions(+), 33 deletions(-) create mode 100644 lib/dead_end/explain_syntax.rb create mode 100644 lib/dead_end/left_right_lex_count.rb create mode 100644 spec/unit/explain_syntax_spec.rb diff --git a/README.md b/README.md index 8cf2252..d1f670e 100644 --- a/README.md +++ b/README.md @@ -2,19 +2,14 @@ An error in your code forces you to stop. DeadEnd helps you find those errors to get you back on your way faster. - DeadEnd: Unmatched `end` detected - - This code has an unmatched `end`. Ensure that all `end` lines - in your code have a matching syntax keyword (`def`, `do`, etc.) - and that you don't have any extra `end` lines. - - file: path/to/dog.rb - simplified: +``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? - 3 class Dog - ❯ 5 defbark - ❯ 7 end - 12 end + 1 class Dog +❯ 2 defbark +❯ 4 end + 5 end +``` ## Installation in your codebase @@ -52,34 +47,99 @@ This gives you the CLI command `$ dead_end` for more info run `$ dead_end --help ## What syntax errors does it handle? +Dead end will fire against all syntax errors and can isolate any syntax error. In addition, dead_end attempts to produce human readable descriptions of what needs to be done to resolve the issue. For example: + - Missing `end`: + -- Unexpected `end` +``` +Unmatched keyword, missing `end' ? +❯ 1 class Dog +❯ 2 def bark +❯ 4 end +``` + +- Missing keyword + -As well as unmatched `|` and unmatched `}`. These errors can be time consuming to debug because Ruby often only tells you the last line in the file. The command `ruby -wc path/to/file.rb` can narrow it down a little bit, but this library does a better job. +``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 1 class Dog + 2 def speak +❯ 3 @sounds.each |sound| +❯ 5 end + 6 end + 7 end +``` + +- Missing pair characters (like `{}`, `[]`, `()` , or `||`) + + +``` +Unmatched `(', missing `)' ? + + 1 class Dog +❯ 2 def speak(sound +❯ 4 end + 5 end +``` + +- Any ambiguous or unknown errors will be annotated by the original ripper error output: + + + +``` +syntax error, unexpected end-of-input + + 1 class Dog + 2 def meals_last_month +❯ 3 puts 3 * + 4 end + 5 end +``` + +## How is it better than `ruby -wc`? + +Ruby allows you to syntax check a file with warnings using `ruby -wc`. This emits a parser error instead of a human focused error. Ruby's parse errors attempt to narrow down the location and can tell you if there is a glaring indentation error involving `end`. + +The `dead_end` algorithm doesn't just guess at the location of syntax errors, it re-parses the document to prove that it captured them. + +This library focuses on the human side of syntax errors. It cares less about why the document could not be parsed (computer problem) and more on what the programmer needs (human problem) to fix the problem. ## Sounds cool, but why isn't this baked into Ruby directly? @@ -105,6 +165,14 @@ After checking out the repo, run `bin/setup` to install dependencies. Then, run To install this gem onto your local machine, run `bundle exec rake install`. To release a new version, update the version number in `version.rb`, and then run `bundle exec rake release`, which will create a git tag for the version, push git commits and tags, and push the `.gem` file to [rubygems.org](https://rubygems.org). +### How to debug changes to output display + +You can see changes to output against a variety of invalid code by running specs and using the `DEBUG_DISPLAY=1` environment variable. For example: + +``` +$ DEBUG_DISPLAY=1 be rspec spec/ --format=failures +``` + ## Contributing Bug reports and pull requests are welcome on GitHub at https://github.com/zombocom/dead_end. This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the [code of conduct](https://github.com/zombocom/dead_end/blob/master/CODE_OF_CONDUCT.md). diff --git a/lib/dead_end.rb b/lib/dead_end.rb index b30aa4a..e8b285b 100644 --- a/lib/dead_end.rb +++ b/lib/dead_end.rb @@ -28,8 +28,9 @@ def self.handle_error(e) raise e end - def self.call(source:, filename:, terminal: DEFAULT_VALUE, record_dir: nil, timeout: TIMEOUT_DEFAULT, io: $stderr) + def self.call(source:, filename: DEFAULT_VALUE, terminal: DEFAULT_VALUE, record_dir: nil, timeout: TIMEOUT_DEFAULT, io: $stderr) search = nil + filename = nil if filename == DEFAULT_VALUE Timeout.timeout(timeout) do record_dir ||= ENV["DEBUG"] ? "tmp" : nil search = CodeSearch.new(source, record_dir: record_dir).call @@ -141,4 +142,6 @@ def self.valid?(source) require_relative "dead_end/display_invalid_blocks" require_relative "dead_end/parse_blocks_from_indent_line" +require_relative "dead_end/explain_syntax" + require_relative "dead_end/auto" diff --git a/lib/dead_end/display_invalid_blocks.rb b/lib/dead_end/display_invalid_blocks.rb index c840f63..e510f8c 100644 --- a/lib/dead_end/display_invalid_blocks.rb +++ b/lib/dead_end/display_invalid_blocks.rb @@ -27,8 +27,10 @@ def call return self end - @io.puts("--> #{filename}") if filename - @io.puts + if filename + @io.puts("--> #{filename}") + @io.puts + end @blocks.each do |block| display_block(block) end @@ -37,6 +39,15 @@ def call end private def display_block(block) + # Output explanations + ExplainSyntax.new( + code_lines: block.lines + ).call.errors.each do |e| + @io.puts e + end + @io.puts + + ## Output source code lines = CaptureCodeContext.new( blocks: block, code_lines: @code_lines @@ -48,16 +59,8 @@ def call highlight_lines: block.lines ).call - RipperErrors.new(block.lines.map(&:original).join).call.errors.each do |e| - @io.puts e - end - @io.puts - @io.puts(document) - end - private def banner - Banner.new(invalid_obj: @invalid_obj).call end private def code_with_context diff --git a/lib/dead_end/explain_syntax.rb b/lib/dead_end/explain_syntax.rb new file mode 100644 index 0000000..05f38a3 --- /dev/null +++ b/lib/dead_end/explain_syntax.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +require_relative "left_right_lex_count" + +module DeadEnd + # Explains syntax errors based on their source + # + # example: + # + # source = "def foo; puts 'lol'" # Note missing end + # explain ExplainSyntax.new( + # code_lines: CodeLine.from_source(source) + # ).call + # explain.errors.first + # # => "Unmatched keyword, missing `end' ?" + # + # When the error cannot be determined by lexical counting + # then ripper is run against the input and the raw ripper + # errors returned. + # + # Example: + # + # source = "1 * " # Note missing a second number + # explain ExplainSyntax.new( + # code_lines: CodeLine.from_source(source) + # ).call + # explain.errors.first + # # => "syntax error, unexpected end-of-input" + class ExplainSyntax + INVERSE = { + "{" => "}", + "}" => "{", + "[" => "]", + "]" => "[", + "(" => ")", + ")" => "(", + "|" => "|" + }.freeze + + def initialize(code_lines: ) + @code_lines = code_lines + @left_right = LeftRightLexCount.new + @missing = nil + end + + def call + @code_lines.each do |line| + line.lex.each do |lex| + @left_right.count_lex(lex) + end + end + + self + end + + # Returns an array of missing elements + # + # For example this: + # + # ExplainSyntax.new(code_lines: lines).missing + # # => ["}"] + # + # Would indicate that the source is missing + # a `}` character in the source code + def missing + @missing ||= @left_right.missing + end + + # Converts a missing string to + # an human understandable explanation. + # + # Example: + # + # explain.why("}") + # # => "Unmatched `{', missing `}' ?" + # + def why(miss) + case miss + when "keyword" + "Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?" + when "end" + "Unmatched keyword, missing `end' ?" + when "{" + "Unmatched `}', missing `{' or `\#{' ?" + else + inverse = INVERSE.fetch(miss) { + raise "Unknown explain syntax char or key: #{miss.inspect}" + } + "Unmatched `#{inverse}', missing `#{miss}' ?" + end + end + + # Returns an array of syntax error messages + # + # If no missing pairs are found it falls back + # on the original ripper error messages + def errors + if missing.empty? + return RipperErrors.new(@code_lines.map(&:original).join).call.errors + end + + missing.map {|miss| why(miss) } + end + end +end diff --git a/lib/dead_end/left_right_lex_count.rb b/lib/dead_end/left_right_lex_count.rb new file mode 100644 index 0000000..a7fec87 --- /dev/null +++ b/lib/dead_end/left_right_lex_count.rb @@ -0,0 +1,159 @@ +# frozen_string_literal: true + +module DeadEnd + # Find mis-matched syntax based on lexical count + # + # Used for detecting missing pairs of elements + # each keyword needs an end, each '{' needs a '}' + # etc. + # + # Example: + # + # left_right = LeftRightLexCount.new + # left_right.count_kw + # left_right.missing.first + # # => "end" + # + # left_right = LeftRightLexCount.new + # source = "{ a: b, c: d" # Note missing '}' + # LexAll.new(source: source).each do |lex| + # left_right.count_lex(lex) + # end + # left_right.missing.first + # # => "}" + class LeftRightLexCount + def initialize + @kw_count = 0 + @end_count = 0 + + @count_for_char = { + "{" => 0, + "}" => 0, + "[" => 0, + "]" => 0, + "(" => 0, + ")" => 0, + "|" => 0 + } + end + + def count_kw + @kw_count += 1 + end + + def count_end + @end_count += 1 + end + + # Count source code characters + # + # Example: + # + # left_right = LeftRightLexCount.new + # left_right.count_lex(LexValue.new(1, :on_lbrace, "{", Ripper::EXPR_BEG)) + # left_right.count_for_char("{") + # # => 1 + # left_right.count_for_char("}") + # # => 0 + def count_lex(lex) + case lex.type + when :on_tstring_content + # ^^^ + # Means it's a string or a symbol `"{"` rather than being + # part of a data structure (like a hash) `{ a: b }` + when :on_embexpr_beg + # ^^^ + # Embedded string expressions like `"#{foo} <-embed"` + # are parsed with chars: + # + # `#{` as :on_embexpr_beg + # `}` as :on_embexpr_end + # + # We cannot ignore both :on_emb_expr_beg and :on_embexpr_end + # because sometimes the lexer thinks something is an embed + # string end, when it is not like `lol = }` (no clue why). + # + # When we see `#{` count it as a `{` or we will + # have a mis-match count. + # + case lex.token + when "\#{" + @count_for_char["{"] += 1 + end + else + @end_count += 1 if lex.is_end? + @kw_count += 1 if lex.is_kw? + @count_for_char[lex.token] += 1 if @count_for_char.key?(lex.token) + end + end + + def count_for_char(char) + @count_for_char[char] + end + + # Returns an array of missing syntax characters + # or `"end"` or `"keyword"` + # + # left_right.missing + # # => ["}"] + def missing + out = missing_pairs + out << missing_pipe + out << missing_keyword_end + out.compact! + out + end + + PAIRS = { + "{" => "}", + "[" => "]", + "(" => ")" + }.freeze + + # Opening characters like `{` need closing characters # like `}`. + # + # When a mis-match count is detected, suggest the + # missing member. + # + # For example if there are 3 `}` and only two `{` + # return `"{"` + private def missing_pairs + PAIRS.map do |(left, right)| + case @count_for_char[left] <=> @count_for_char[right] + when 1 + right + when 0 + nil + when -1 + left + end + end + end + + # Keywords need ends and ends need keywords + # + # If we have more keywords, there's a missing `end` + # if we have more `end`-s, there's a missing keyword + private def missing_keyword_end + case @kw_count <=> @end_count + when 1 + "end" + when 0 + nil + when -1 + "keyword" + end + end + + # Pipes come in pairs. + # If there's an odd number of pipes then we + # are missing one + private def missing_pipe + if @count_for_char["|"].odd? + "|" + else + nil + end + end + end +end diff --git a/spec/integration/exe_cli_spec.rb b/spec/integration/exe_cli_spec.rb index a6a891f..2696a5f 100644 --- a/spec/integration/exe_cli_spec.rb +++ b/spec/integration/exe_cli_spec.rb @@ -24,6 +24,7 @@ def exe(cmd) it "parses invalid code" do ruby_file = fixtures_dir.join("this_project_extra_def.rb.txt") out = exe(ruby_file) + debug_display(out) expect(out.strip).to include("❯ 36 def filename") expect($?.success?).to be_falsey @@ -37,6 +38,7 @@ def exe(cmd) Pathname(file.path).write(lines.join) out = exe(file.path) + debug_display(out) expect(out).to include(<<~EOM) 16 class Rexe diff --git a/spec/integration/improvement_regression_spec.rb b/spec/integration/improvement_regression_spec.rb index fc7c796..b2c15c4 100644 --- a/spec/integration/improvement_regression_spec.rb +++ b/spec/integration/improvement_regression_spec.rb @@ -3,7 +3,7 @@ require_relative "../spec_helper" module DeadEnd - RSpec.describe "Library only integration to test regressions and improvements" do + RSpec.describe "Integration tests that don't spawn a process (like using the cli)" do it "returns good results on routes.rb" do source = fixtures_dir.join("routes.rb.txt").read @@ -13,6 +13,7 @@ module DeadEnd source: source, filename: "none" ) + debug_display(io.string) expect(io.string).to include(<<~'EOM') 1 Rails.application.routes.draw do @@ -32,6 +33,7 @@ module DeadEnd source: source, filename: "none" ) + debug_display(io.string) expect(io.string).to include(<<~'EOM') 1 describe "webmock tests" do @@ -55,6 +57,8 @@ module DeadEnd filename: "none" ) + debug_display(io.string) + expect(io.string).to include(<<~'EOM') 5 module DerailedBenchmarks 6 class RequireTree diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 39f3b9e..e01a6ac 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -17,6 +17,15 @@ end end +# Used for debugging modifications to +# display output +def debug_display(output) + return unless ENV["DEBUG_DISPLAY"] + puts + puts output + puts +end + def spec_dir Pathname(__dir__) end diff --git a/spec/unit/explain_syntax_spec.rb b/spec/unit/explain_syntax_spec.rb new file mode 100644 index 0000000..a9ea9af --- /dev/null +++ b/spec/unit/explain_syntax_spec.rb @@ -0,0 +1,230 @@ +# frozen_string_literal: true + +require_relative "../spec_helper" + +module DeadEnd + RSpec.describe "ExplainSyntax" do + it "doesn't falsely identify strings or symbols as critical chars" do + source = <<~EOM + a = ['(', '{', '[', '|'] + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq([]) + + source = <<~EOM + a = [:'(', :'{', :'[', :'|'] + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq([]) + end + + it "finds missing |" do + source = <<~EOM + Foo.call do | + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["|"]) + expect(explain.errors).to eq([explain.why("|")]) + end + + it "finds missing {" do + source = <<~EOM + class Cat + lol = { + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["}"]) + expect(explain.errors).to eq([explain.why("}")]) + end + + it "finds missing }" do + source = <<~EOM + def foo + lol = "foo" => :bar } + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["{"]) + expect(explain.errors).to eq([explain.why("{")]) + end + + it "finds missing [" do + source = <<~EOM + class Cat + lol = [ + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["]"]) + expect(explain.errors).to eq([explain.why("]")]) + end + + it "finds missing ]" do + source = <<~EOM + def foo + lol = ] + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["["]) + expect(explain.errors).to eq([explain.why("[")]) + end + + it "finds missing (" do + source = "def initialize; ); end" + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["("]) + expect(explain.errors).to eq([explain.why("(")]) + end + + it "finds missing )" do + source = "def initialize; (; end" + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq([")"]) + expect(explain.errors).to eq([explain.why(")")]) + end + + it "finds missing keyword" do + source = <<~EOM + class Cat + end + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["keyword"]) + expect(explain.errors).to eq([explain.why("keyword")]) + end + + it "finds missing end" do + source = <<~EOM + class Cat + def meow + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["end"]) + expect(explain.errors).to eq([explain.why("end")]) + end + + it "falls back to ripper on unknown errors" do + source = <<~EOM + class Cat + def meow + 1 * + end + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq([]) + expect(explain.errors).to eq(RipperErrors.new(source).call.errors) + end + + it "handles an unexpected rescue" do + source = <<~EOM + def foo + if bar + "baz" + else + "foo" + rescue FooBar + nil + end + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["end"]) + end + + # String embeds are `"#{foo} <-- here` + # + # We need to count a `#{` as a `{` + # otherwise it will report that we are + # missing a curly when we are using valid + # string embed syntax + it "is not confused by valid string embed" do + source = <<~'EOM' + foo = "#{hello}" + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + expect(explain.missing).to eq([]) + end + + # Missing string embed beginnings are not a + # syntax error. i.e. `"foo}"` or `"{foo}` or "#foo}" + # would just be strings with extra characters. + # + # However missing the end curly will trigger + # an error: i.e. `"#{foo` + # + # String embed beginning is a `#{` rather than + # a `{`, make sure we handle that case and + # report the correct missing `}` diagnosis + it "finds missing string embed end" do + source = <<~'EOM' + "#{foo + EOM + + explain = ExplainSyntax.new( + code_lines: CodeLine.from_source(source) + ).call + + expect(explain.missing).to eq(["}"]) + end + end +end From 628ef3510025b8a57eb5c82778f1e8a1d2bb7d6e Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 12:22:27 -0500 Subject: [PATCH 02/10] Allow DeadEnd.call to fire without a filename --- spec/integration/improvement_regression_spec.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/spec/integration/improvement_regression_spec.rb b/spec/integration/improvement_regression_spec.rb index b2c15c4..b23d8f5 100644 --- a/spec/integration/improvement_regression_spec.rb +++ b/spec/integration/improvement_regression_spec.rb @@ -11,7 +11,6 @@ module DeadEnd DeadEnd.call( io: io, source: source, - filename: "none" ) debug_display(io.string) @@ -31,7 +30,6 @@ module DeadEnd DeadEnd.call( io: io, source: source, - filename: "none" ) debug_display(io.string) @@ -54,9 +52,7 @@ module DeadEnd DeadEnd.call( io: io, source: source, - filename: "none" ) - debug_display(io.string) expect(io.string).to include(<<~'EOM') From 0528dc0249702a95d579401793eb2e16c471651c Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 14:14:44 -0500 Subject: [PATCH 03/10] Rename integration test The integration test was poorly named. It's really integration cases for when we're not using a different process. --- .../{improvement_regression_spec.rb => dead_end_spec.rb} | 1 + spec/unit/dead_end_spec.rb | 8 -------- 2 files changed, 1 insertion(+), 8 deletions(-) rename spec/integration/{improvement_regression_spec.rb => dead_end_spec.rb} (99%) delete mode 100644 spec/unit/dead_end_spec.rb diff --git a/spec/integration/improvement_regression_spec.rb b/spec/integration/dead_end_spec.rb similarity index 99% rename from spec/integration/improvement_regression_spec.rb rename to spec/integration/dead_end_spec.rb index b23d8f5..34e0f9f 100644 --- a/spec/integration/improvement_regression_spec.rb +++ b/spec/integration/dead_end_spec.rb @@ -4,6 +4,7 @@ module DeadEnd RSpec.describe "Integration tests that don't spawn a process (like using the cli)" do + it "returns good results on routes.rb" do source = fixtures_dir.join("routes.rb.txt").read diff --git a/spec/unit/dead_end_spec.rb b/spec/unit/dead_end_spec.rb deleted file mode 100644 index 09ff0cf..0000000 --- a/spec/unit/dead_end_spec.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -require_relative "../spec_helper" - -module DeadEnd - RSpec.describe DeadEnd do - end -end From ff41ec109c93a33e3b69a35cb2334d8f5f7a90cc Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 14:45:45 -0500 Subject: [PATCH 04/10] Fix incorrect code removal As listed in #95 there are other cases where we have valid code nested in a way we didn't previously handle (via `CleanDocument`). The specific case is: ```ruby def ruby_install_binstub_path(ruby_layer_path = ".") @ruby_install_binstub_path ||= if ruby_version.build? "#{build_ruby_path}/bin" elsif ruby_version "#{ruby_layer_path}/#{slug_vendor_ruby}/bin" else "" end end ``` As it's being through being validated/hidden, the inside is removed first, which generates an invalid code block. This fix is to re-check all code in a block even if it is "hidden". This conveniently drastically improves the results of #88 beyond just that small case, the whole output becomes very focused close #95 close #88 --- lib/dead_end/code_block.rb | 7 +- spec/fixtures/ruby_buildpack.rb.txt | 1344 +++++++++++++++++++++++++++ spec/integration/dead_end_spec.rb | 20 + 3 files changed, 1369 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/ruby_buildpack.rb.txt diff --git a/lib/dead_end/code_block.rb b/lib/dead_end/code_block.rb index 1fbefb1..ab81987 100644 --- a/lib/dead_end/code_block.rb +++ b/lib/dead_end/code_block.rb @@ -70,8 +70,11 @@ def invalid? end def valid? - return @valid if @valid != UNSET - @valid = DeadEnd.valid?(to_s) + if @valid == UNSET + @valid = DeadEnd.valid?(lines.map(&:original).join) + else + @valid + end end def to_s diff --git a/spec/fixtures/ruby_buildpack.rb.txt b/spec/fixtures/ruby_buildpack.rb.txt new file mode 100644 index 0000000..9acdbf3 --- /dev/null +++ b/spec/fixtures/ruby_buildpack.rb.txt @@ -0,0 +1,1344 @@ +require "tmpdir" +require "digest/md5" +require "benchmark" +require "rubygems" +require "language_pack" +require "language_pack/base" +require "language_pack/ruby_version" +require "language_pack/helpers/nodebin" +require "language_pack/helpers/node_installer" +require "language_pack/helpers/yarn_installer" +require "language_pack/helpers/layer" +require "language_pack/helpers/binstub_check" +require "language_pack/version" + +# base Ruby Language Pack. This is for any base ruby app. +class LanguagePack::Ruby < LanguagePack::Base + NAME = "ruby" + LIBYAML_VERSION = "0.1.7" + LIBYAML_PATH = "libyaml-#{LIBYAML_VERSION}" + RBX_BASE_URL = "http://binaries.rubini.us/heroku" + NODE_BP_PATH = "vendor/node/bin" + + Layer = LanguagePack::Helpers::Layer + + # detects if this is a valid Ruby app + # @return [Boolean] true if it's a Ruby app + def self.use? + instrument "ruby.use" do + File.exist?("Gemfile") + end + end + + def self.bundler + @@bundler ||= LanguagePack::Helpers::BundlerWrapper.new.install + end + + def bundler + self.class.bundler + end + + def initialize(*args) + super(*args) + @fetchers[:mri] = LanguagePack::Fetcher.new(VENDOR_URL, @stack) + @fetchers[:rbx] = LanguagePack::Fetcher.new(RBX_BASE_URL, @stack) + @node_installer = LanguagePack::Helpers::NodeInstaller.new + @yarn_installer = LanguagePack::Helpers::YarnInstaller.new + end + + def name + "Ruby" + end + + def default_addons + instrument "ruby.default_addons" do + add_dev_database_addon + end + end + + def default_config_vars + instrument "ruby.default_config_vars" do + vars = { + "LANG" => env("LANG") || "en_US.UTF-8", + } + + ruby_version.jruby? ? vars.merge({ + "JRUBY_OPTS" => default_jruby_opts + }) : vars + end + end + + def default_process_types + instrument "ruby.default_process_types" do + { + "rake" => "bundle exec rake", + "console" => "bundle exec irb" + } + end + end + + def best_practice_warnings + if bundler.has_gem?("asset_sync") + warn(<<-WARNING) +You are using the `asset_sync` gem. +This is not recommended. +See https://devcenter.heroku.com/articles/please-do-not-use-asset-sync for more information. +WARNING + end + end + + def compile + instrument 'ruby.compile' do + # check for new app at the beginning of the compile + new_app? + Dir.chdir(build_path) + remove_vendor_bundle + warn_bundler_upgrade + warn_bad_binstubs + install_ruby(slug_vendor_ruby, build_ruby_path) + setup_language_pack_environment( + ruby_layer_path: File.expand_path("."), + gem_layer_path: File.expand_path("."), + bundle_path: "vendor/bundle", + bundle_default_without: "development:test" + ) + allow_git do + install_bundler_in_app(slug_vendor_base) + load_bundler_cache + build_bundler + post_bundler + create_database_yml + install_binaries + run_assets_precompile_rake_task + end + config_detect + best_practice_warnings + warn_outdated_ruby + setup_profiled(ruby_layer_path: "$HOME", gem_layer_path: "$HOME") # $HOME is set to /app at run time + setup_export + cleanup + super + end + rescue => e + warn_outdated_ruby + raise e + end + + + def build + new_app? + remove_vendor_bundle + warn_bad_binstubs + ruby_layer = Layer.new(@layer_dir, "ruby", launch: true) + install_ruby("#{ruby_layer.path}/#{slug_vendor_ruby}") + ruby_layer.metadata[:version] = ruby_version.version + ruby_layer.metadata[:patchlevel] = ruby_version.patchlevel if ruby_version.patchlevel + ruby_layer.metadata[:engine] = ruby_version.engine.to_s + ruby_layer.metadata[:engine_version] = ruby_version.engine_version + ruby_layer.write + + gem_layer = Layer.new(@layer_dir, "gems", launch: true, cache: true, build: true) + setup_language_pack_environment( + ruby_layer_path: ruby_layer.path, + gem_layer_path: gem_layer.path, + bundle_path: "#{gem_layer.path}/vendor/bundle", + bundle_default_without: "development:test" + ) + allow_git do + # TODO install bundler in separate layer + topic "Loading Bundler Cache" + gem_layer.validate! do |metadata| + valid_bundler_cache?(gem_layer.path, gem_layer.metadata) + end + install_bundler_in_app("#{gem_layer.path}/#{slug_vendor_base}") + build_bundler + # TODO post_bundler might need to be done in a new layer + bundler.clean + gem_layer.metadata[:gems] = Digest::SHA2.hexdigest(File.read("Gemfile.lock")) + gem_layer.metadata[:stack] = @stack + gem_layer.metadata[:ruby_version] = run_stdout(%q(ruby -v)).strip + gem_layer.metadata[:rubygems_version] = run_stdout(%q(gem -v)).strip + gem_layer.metadata[:buildpack_version] = BUILDPACK_VERSION + gem_layer.write + + create_database_yml + # TODO replace this with multibuildpack stuff? put binaries in their own layer? + install_binaries + run_assets_precompile_rake_task + end + setup_profiled(ruby_layer_path: ruby_layer.path, gem_layer_path: gem_layer.path) + setup_export(gem_layer) + config_detect + best_practice_warnings + cleanup + + super + end + + def cleanup + end + + def config_detect + end + +private + + # A bad shebang line looks like this: + # + # ``` + # #!/usr/bin/env ruby2.5 + # ``` + # + # Since `ruby2.5` is not a valid binary name + # + def warn_bad_binstubs + check = LanguagePack::Helpers::BinstubCheck.new(app_root_dir: Dir.pwd, warn_object: self) + check.call + end + + def default_malloc_arena_max? + return true if @metadata.exists?("default_malloc_arena_max") + return @metadata.touch("default_malloc_arena_max") if new_app? + + return false + end + + def warn_bundler_upgrade + old_bundler_version = @metadata.read("bundler_version").strip if @metadata.exists?("bundler_version") + + if old_bundler_version && old_bundler_version != bundler.version + warn(<<-WARNING, inline: true) +Your app was upgraded to bundler #{ bundler.version }. +Previously you had a successful deploy with bundler #{ old_bundler_version }. + +If you see problems related to the bundler version please refer to: +https://devcenter.heroku.com/articles/bundler-version#known-upgrade-issues + +WARNING + end + end + + # For example "vendor/bundle/ruby/2.6.0" + def self.slug_vendor_base + @slug_vendor_base ||= begin + command = %q(ruby -e "require 'rbconfig';puts \"vendor/bundle/#{RUBY_ENGINE}/#{RbConfig::CONFIG['ruby_version']}\"") + out = run_no_pipe(command, user_env: true).strip + error "Problem detecting bundler vendor directory: #{out}" unless $?.success? + out + end + end + + # the relative path to the bundler directory of gems + # @return [String] resulting path + def slug_vendor_base + instrument 'ruby.slug_vendor_base' do + @slug_vendor_base ||= self.class.slug_vendor_base + end + end + + # the relative path to the vendored ruby directory + # @return [String] resulting path + def slug_vendor_ruby + "vendor/#{ruby_version.version_without_patchlevel}" + end + + # the absolute path of the build ruby to use during the buildpack + # @return [String] resulting path + def build_ruby_path + "/tmp/#{ruby_version.version_without_patchlevel}" + end + + # fetch the ruby version from bundler + # @return [String, nil] returns the ruby version if detected or nil if none is detected + def ruby_version + instrument 'ruby.ruby_version' do + return @ruby_version if @ruby_version + new_app = !File.exist?("vendor/heroku") + last_version_file = "buildpack_ruby_version" + last_version = nil + last_version = @metadata.read(last_version_file).strip if @metadata.exists?(last_version_file) + + @ruby_version = LanguagePack::RubyVersion.new(bundler.ruby_version, + is_new: new_app, + last_version: last_version) + return @ruby_version + end + end + + def set_default_web_concurrency + <<-EOF +case $(ulimit -u) in +256) + export HEROKU_RAM_LIMIT_MB=${HEROKU_RAM_LIMIT_MB:-512} + export WEB_CONCURRENCY=${WEB_CONCURRENCY:-2} + ;; +512) + export HEROKU_RAM_LIMIT_MB=${HEROKU_RAM_LIMIT_MB:-1024} + export WEB_CONCURRENCY=${WEB_CONCURRENCY:-4} + ;; +16384) + export HEROKU_RAM_LIMIT_MB=${HEROKU_RAM_LIMIT_MB:-2560} + export WEB_CONCURRENCY=${WEB_CONCURRENCY:-8} + ;; +32768) + export HEROKU_RAM_LIMIT_MB=${HEROKU_RAM_LIMIT_MB:-6144} + export WEB_CONCURRENCY=${WEB_CONCURRENCY:-16} + ;; +*) + ;; +esac +EOF + end + + # default JRUBY_OPTS + # return [String] string of JRUBY_OPTS + def default_jruby_opts + "-Xcompile.invokedynamic=false" + end + + # sets up the environment variables for the build process + def setup_language_pack_environment(ruby_layer_path:, gem_layer_path:, bundle_path:, bundle_default_without:) + instrument 'ruby.setup_language_pack_environment' do + if ruby_version.jruby? + ENV["PATH"] += ":bin" + ENV["JRUBY_OPTS"] = env('JRUBY_BUILD_OPTS') || env('JRUBY_OPTS') + end + setup_ruby_install_env(ruby_layer_path) + + # By default Node can address 1.5GB of memory, a limitation it inherits from + # the underlying v8 engine. This can occasionally cause issues during frontend + # builds where memory use can exceed this threshold. + # + # This passes an argument to all Node processes during the build, so that they + # can take advantage of all available memory on the build dynos. + ENV["NODE_OPTIONS"] ||= "--max_old_space_size=2560" + + # TODO when buildpack-env-args rolls out, we can get rid of + # ||= and the manual setting below + default_config_vars.each do |key, value| + ENV[key] ||= value + end + + paths = [] + gem_path = "#{gem_layer_path}/#{slug_vendor_base}" + ENV["GEM_PATH"] = gem_path + ENV["GEM_HOME"] = gem_path + + ENV["DISABLE_SPRING"] = "1" + + # Rails has a binstub for yarn that doesn't work for all applications + # we need to ensure that yarn comes before local bin dir for that case + paths << yarn_preinstall_bin_path if yarn_preinstalled? + + # Need to remove `./bin` folder since it links to the wrong --prefix ruby binstubs breaking require in Ruby 1.9.2 and 1.8.7. + # Because for 1.9.2 and 1.8.7 there is a "build" ruby and a non-"build" Ruby + paths << "#{File.expand_path(".")}/bin" unless ruby_version.ruby_192_or_lower? + + paths << "#{gem_layer_path}/#{bundler_binstubs_path}" # Binstubs from bundler, eg. vendor/bundle/bin + paths << "#{gem_layer_path}/#{slug_vendor_base}/bin" # Binstubs from rubygems, eg. vendor/bundle/ruby/2.6.0/bin + paths << ENV["PATH"] + + ENV["PATH"] = paths.join(":") + + ENV["BUNDLE_WITHOUT"] = env("BUNDLE_WITHOUT") || bundle_default_without + if ENV["BUNDLE_WITHOUT"].include?(' ') + ENV["BUNDLE_WITHOUT"] = ENV["BUNDLE_WITHOUT"].tr(' ', ':') + + warn("Your BUNDLE_WITHOUT contains a space, we are converting it to a colon `:` BUNDLE_WITHOUT=#{ENV["BUNDLE_WITHOUT"]}", inline: true) + end + ENV["BUNDLE_PATH"] = bundle_path + ENV["BUNDLE_BIN"] = bundler_binstubs_path + ENV["BUNDLE_DEPLOYMENT"] = "1" + ENV["BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE"] = "1" if bundler.needs_ruby_global_append_path? + end + end + + # Sets up the environment variables for subsequent processes run by + # muiltibuildpack. We can't use profile.d because $HOME isn't set up + def setup_export(layer = nil) + instrument 'ruby.setup_export' do + if layer + paths = ENV["PATH"] + else + paths = ENV["PATH"].split(":").map do |path| + /^\/.*/ !~ path ? "#{build_path}/#{path}" : path + end.join(":") + end + + # TODO ensure path exported is correct + set_export_path "PATH", paths, layer + + if layer + gem_path = "#{layer.path}/#{slug_vendor_base}" + else + gem_path = "#{build_path}/#{slug_vendor_base}" + end + set_export_path "GEM_PATH", gem_path, layer + set_export_default "LANG", "en_US.UTF-8", layer + + # TODO handle jruby + if ruby_version.jruby? + set_export_default "JRUBY_OPTS", default_jruby_opts + end + + set_export_default "BUNDLE_PATH", ENV["BUNDLE_PATH"], layer + set_export_default "BUNDLE_WITHOUT", ENV["BUNDLE_WITHOUT"], layer + set_export_default "BUNDLE_BIN", ENV["BUNDLE_BIN"], layer + set_export_default "BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE", ENV["BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE"], layer if bundler.needs_ruby_global_append_path? + set_export_default "BUNDLE_DEPLOYMENT", ENV["BUNDLE_DEPLOYMENT"], layer if ENV["BUNDLE_DEPLOYMENT"] # Unset on windows since we delete the Gemfile.lock + end + end + + # sets up the profile.d script for this buildpack + def setup_profiled(ruby_layer_path: , gem_layer_path: ) + instrument 'setup_profiled' do + profiled_path = [] + + # Rails has a binstub for yarn that doesn't work for all applications + # we need to ensure that yarn comes before local bin dir for that case + if yarn_preinstalled? + profiled_path << yarn_preinstall_bin_path.gsub(File.expand_path("."), "$HOME") + elsif has_yarn_binary? + profiled_path << "#{ruby_layer_path}/vendor/#{@yarn_installer.binary_path}" + end + profiled_path << "$HOME/bin" # /app in production + profiled_path << "#{gem_layer_path}/#{bundler_binstubs_path}" # Binstubs from bundler, eg. vendor/bundle/bin + profiled_path << "#{gem_layer_path}/#{slug_vendor_base}/bin" # Binstubs from rubygems, eg. vendor/bundle/ruby/2.6.0/bin + profiled_path << "$PATH" + + set_env_default "LANG", "en_US.UTF-8" + set_env_override "GEM_PATH", "#{gem_layer_path}/#{slug_vendor_base}:$GEM_PATH" + set_env_override "PATH", profiled_path.join(":") + set_env_override "DISABLE_SPRING", "1" + + set_env_default "MALLOC_ARENA_MAX", "2" if default_malloc_arena_max? + + web_concurrency = env("SENSIBLE_DEFAULTS") ? set_default_web_concurrency : "" + add_to_profiled(web_concurrency, filename: "WEB_CONCURRENCY.sh", mode: "w") # always write that file, even if its empty (meaning no defaults apply), for interop with other buildpacks - and we overwrite the file rather than appending (which is the default) + + # TODO handle JRUBY + if ruby_version.jruby? + set_env_default "JRUBY_OPTS", default_jruby_opts + end + + set_env_default "BUNDLE_PATH", ENV["BUNDLE_PATH"] + set_env_default "BUNDLE_WITHOUT", ENV["BUNDLE_WITHOUT"] + set_env_default "BUNDLE_BIN", ENV["BUNDLE_BIN"] + set_env_default "BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE", ENV["BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE"] if bundler.needs_ruby_global_append_path? + set_env_default "BUNDLE_DEPLOYMENT", ENV["BUNDLE_DEPLOYMENT"] if ENV["BUNDLE_DEPLOYMENT"] # Unset on windows since we delete the Gemfile.lock + end + end + + def warn_outdated_ruby + return unless defined?(@outdated_version_check) + + @warn_outdated ||= begin + @outdated_version_check.join + + warn_outdated_minor + warn_outdated_eol + warn_stack_upgrade + true + end + end + + def warn_stack_upgrade + return unless defined?(@ruby_download_check) + return unless @ruby_download_check.next_stack(current_stack: stack) + return if @ruby_download_check.exists_on_next_stack?(current_stack: stack) + + warn(<<~WARNING) + Your Ruby version is not present on the next stack + + You are currently using #{ruby_version.version_for_download} on #{stack} stack. + This version does not exist on #{@ruby_download_check.next_stack(current_stack: stack)}. In order to upgrade your stack you will + need to upgrade to a supported Ruby version. + + For a list of supported Ruby versions see: + https://devcenter.heroku.com/articles/ruby-support#supported-runtimes + + For a list of the oldest Ruby versions present on a given stack see: + https://devcenter.heroku.com/articles/ruby-support#oldest-available-runtimes + WARNING + end + + def warn_outdated_eol + return unless @outdated_version_check.maybe_eol? + + if @outdated_version_check.eol? + warn(<<~WARNING) + EOL Ruby Version + + You are using a Ruby version that has reached its End of Life (EOL) + + We strongly suggest you upgrade to Ruby #{@outdated_version_check.suggest_ruby_eol_version} or later + + Your current Ruby version no longer receives security updates from + Ruby Core and may have serious vulnerabilities. While you will continue + to be able to deploy on Heroku with this Ruby version you must upgrade + to a non-EOL version to be eligible to receive support. + + Upgrade your Ruby version as soon as possible. + + For a list of supported Ruby versions see: + https://devcenter.heroku.com/articles/ruby-support#supported-runtimes + WARNING + else + # Maybe EOL + warn(<<~WARNING) + Potential EOL Ruby Version + + You are using a Ruby version that has either reached its End of Life (EOL) + or will reach its End of Life on December 25th of this year. + + We suggest you upgrade to Ruby #{@outdated_version_check.suggest_ruby_eol_version} or later + + Once a Ruby version becomes EOL, it will no longer receive + security updates from Ruby core and may have serious vulnerabilities. + + Please upgrade your Ruby version. + + For a list of supported Ruby versions see: + https://devcenter.heroku.com/articles/ruby-support#supported-runtimes + WARNING + end + end + + def warn_outdated_minor + return if @outdated_version_check.latest_minor_version? + + warn(<<~WARNING) + There is a more recent Ruby version available for you to use: + + #{@outdated_version_check.suggested_ruby_minor_version} + + The latest version will include security and bug fixes. We always recommend + running the latest version of your minor release. + + Please upgrade your Ruby version. + + For all available Ruby versions see: + https://devcenter.heroku.com/articles/ruby-support#supported-runtimes + WARNING + end + + # install the vendored ruby + # @return [Boolean] true if it installs the vendored ruby and false otherwise + def install_ruby(install_path, build_ruby_path = nil) + instrument 'ruby.install_ruby' do + # Could do a compare operation to avoid re-downloading ruby + return false unless ruby_version + installer = LanguagePack::Installers::RubyInstaller.installer(ruby_version).new(@stack) + + @ruby_download_check = LanguagePack::Helpers::DownloadPresence.new(ruby_version.file_name) + @ruby_download_check.call + + if ruby_version.build? + installer.fetch_unpack(ruby_version, build_ruby_path, true) + end + + installer.install(ruby_version, install_path) + + @outdated_version_check = LanguagePack::Helpers::OutdatedRubyVersion.new( + current_ruby_version: ruby_version, + fetcher: installer.fetcher + ) + @outdated_version_check.call + + @metadata.write("buildpack_ruby_version", ruby_version.version_for_download) + + topic "Using Ruby version: #{ruby_version.version_for_download}" + if !ruby_version.set + warn(<<~WARNING) + You have not declared a Ruby version in your Gemfile. + + To declare a Ruby version add this line to your Gemfile: + + ``` + ruby "#{LanguagePack::RubyVersion::DEFAULT_VERSION_NUMBER}" + ``` + + For more information see: + https://devcenter.heroku.com/articles/ruby-versions + WARNING + end + + if ruby_version.warn_ruby_26_bundler? + warn(<<~WARNING, inline: true) + There is a known bundler bug with your version of Ruby + + Your version of Ruby contains a problem with the built-in integration of bundler. If + you encounter a bundler error you need to upgrade your Ruby version. We suggest you upgrade to: + + #{@outdated_version_check.suggested_ruby_minor_version} + + For more information see: + https://devcenter.heroku.com/articles/bundler-version#known-upgrade-issues + WARNING + end + end + + true + rescue LanguagePack::Fetcher::FetchError + if @ruby_download_check.does_not_exist? + message = <<~ERROR + The Ruby version you are trying to install does not exist: #{ruby_version.version_for_download} + ERROR + else + message = <<~ERROR + The Ruby version you are trying to install does not exist on this stack. + + You are trying to install #{ruby_version.version_for_download} on #{stack}. + + Ruby #{ruby_version.version_for_download} is present on the following stacks: + + - #{@ruby_download_check.valid_stack_list.join("\n - ")} + ERROR + + if env("CI") + message << <<~ERROR + + On Heroku CI you can set your stack in the `app.json`. For example: + + ``` + "stack": "heroku-20" + ``` + ERROR + end + end + + message << <<~ERROR + + Heroku recommends you use the latest supported Ruby version listed here: + https://devcenter.heroku.com/articles/ruby-support#supported-runtimes + + For more information on syntax for declaring a Ruby version see: + https://devcenter.heroku.com/articles/ruby-versions + ERROR + + error message + end + + # TODO make this compatible with CNB + def new_app? + @new_app ||= !File.exist?("vendor/heroku") + end + + # find the ruby install path for its binstubs during build + # @return [String] resulting path or empty string if ruby is not vendored + def ruby_install_binstub_path(ruby_layer_path = ".") + @ruby_install_binstub_path ||= + if ruby_version.build? + "#{build_ruby_path}/bin" + elsif ruby_version + "#{ruby_layer_path}/#{slug_vendor_ruby}/bin" + else + "" + end + end + + # setup the environment so we can use the vendored ruby + def setup_ruby_install_env(ruby_layer_path = ".") + instrument 'ruby.setup_ruby_install_env' do + ENV["PATH"] = "#{File.expand_path(ruby_install_binstub_path(ruby_layer_path))}:#{ENV["PATH"]}" + end + end + + # installs vendored gems into the slug + def install_bundler_in_app(bundler_dir) + instrument 'ruby.install_language_pack_gems' do + FileUtils.mkdir_p(bundler_dir) + Dir.chdir(bundler_dir) do |dir| + `cp -R #{bundler.bundler_path}/. .` + end + + # write bundler shim, so we can control the version bundler used + # Ruby 2.6.0 started vendoring bundler + write_bundler_shim("vendor/bundle/bin") if ruby_version.vendored_bundler? + end + end + + # default set of binaries to install + # @return [Array] resulting list + def binaries + add_node_js_binary + add_yarn_binary + end + + # vendors binaries into the slug + def install_binaries + instrument 'ruby.install_binaries' do + binaries.each {|binary| install_binary(binary) } + Dir["bin/*"].each {|path| run("chmod +x #{path}") } + end + end + + # vendors individual binary into the slug + # @param [String] name of the binary package from S3. + # Example: https://s3.amazonaws.com/language-pack-ruby/node-0.4.7.tgz, where name is "node-0.4.7" + def install_binary(name) + topic "Installing #{name}" + bin_dir = "bin" + FileUtils.mkdir_p bin_dir + Dir.chdir(bin_dir) do |dir| + if name.match(/^node\-/) + @node_installer.install + # need to set PATH here b/c `node-gyp` can change the CWD, but still depends on executing node. + # the current PATH is relative, but it needs to be absolute for this. + # doing this here also prevents it from being exported during runtime + node_bin_path = File.absolute_path(".") + # this needs to be set after so other binaries in bin/ don't take precedence" + ENV["PATH"] = "#{ENV["PATH"]}:#{node_bin_path}" + elsif name.match(/^yarn\-/) + FileUtils.mkdir_p("../vendor") + Dir.chdir("../vendor") do |vendor_dir| + @yarn_installer.install + yarn_path = File.absolute_path("#{vendor_dir}/#{@yarn_installer.binary_path}") + ENV["PATH"] = "#{yarn_path}:#{ENV["PATH"]}" + end + else + @fetchers[:buildpack].fetch_untar("#{name}.tgz") + end + end + end + + # removes a binary from the slug + # @param [String] relative path of the binary on the slug + def uninstall_binary(path) + FileUtils.rm File.join('bin', File.basename(path)), :force => true + end + + def load_default_cache? + new_app? && ruby_version.default? + end + + # loads a default bundler cache for new apps to speed up initial bundle installs + def load_default_cache + instrument "ruby.load_default_cache" do + if false # load_default_cache? + puts "New app detected loading default bundler cache" + patchlevel = run("ruby -e 'puts RUBY_PATCHLEVEL'").strip + cache_name = "#{LanguagePack::RubyVersion::DEFAULT_VERSION}-p#{patchlevel}-default-cache" + @fetchers[:buildpack].fetch_untar("#{cache_name}.tgz") + end + end + end + + # remove `vendor/bundle` that comes from the git repo + # in case there are native ext. + # users should be using `bundle pack` instead. + # https://github.com/heroku/heroku-buildpack-ruby/issues/21 + def remove_vendor_bundle + if File.exists?("vendor/bundle") + warn(<<-WARNING) +Removing `vendor/bundle`. +Checking in `vendor/bundle` is not supported. Please remove this directory +and add it to your .gitignore. To vendor your gems with Bundler, use +`bundle pack` instead. +WARNING + FileUtils.rm_rf("vendor/bundle") + end + end + + def bundler_binstubs_path + "vendor/bundle/bin" + end + + def bundler_path + @bundler_path ||= "#{slug_vendor_base}/gems/#{bundler.dir_name}" + end + + def write_bundler_shim(path) + FileUtils.mkdir_p(path) + shim_path = "#{path}/bundle" + File.open(shim_path, "w") do |file| + file.print <<-BUNDLE +#!/usr/bin/env ruby +require 'rubygems' + +version = "#{bundler.version}" + +if ARGV.first + str = ARGV.first + str = str.dup.force_encoding("BINARY") if str.respond_to? :force_encoding + if str =~ /\A_(.*)_\z/ and Gem::Version.correct?($1) then + version = $1 + ARGV.shift + end +end + +if Gem.respond_to?(:activate_bin_path) +load Gem.activate_bin_path('bundler', 'bundle', version) +else +gem "bundler", version +load Gem.bin_path("bundler", "bundle", version) +end +BUNDLE + end + FileUtils.chmod(0755, shim_path) + end + + # runs bundler to install the dependencies + def build_bundler + instrument 'ruby.build_bundler' do + log("bundle") do + if File.exist?("#{Dir.pwd}/.bundle/config") + warn(<<~WARNING, inline: true) + You have the `.bundle/config` file checked into your repository + It contains local state like the location of the installed bundle + as well as configured git local gems, and other settings that should + not be shared between multiple checkouts of a single repo. Please + remove the `.bundle/` folder from your repo and add it to your `.gitignore` file. + + https://devcenter.heroku.com/articles/bundler-configuration + WARNING + end + + if bundler.windows_gemfile_lock? + log("bundle", "has_windows_gemfile_lock") + + File.unlink("Gemfile.lock") + ENV.delete("BUNDLE_DEPLOYMENT") + + warn(<<~WARNING, inline: true) + Removing `Gemfile.lock` because it was generated on Windows. + Bundler will do a full resolve so native gems are handled properly. + This may result in unexpected gem versions being used in your app. + In rare occasions Bundler may not be able to resolve your dependencies at all. + + https://devcenter.heroku.com/articles/bundler-windows-gemfile + WARNING + end + + bundle_command = String.new("") + bundle_command << "BUNDLE_WITHOUT='#{ENV["BUNDLE_WITHOUT"]}' " + bundle_command << "BUNDLE_PATH=#{ENV["BUNDLE_PATH"]} " + bundle_command << "BUNDLE_BIN=#{ENV["BUNDLE_BIN"]} " + bundle_command << "BUNDLE_DEPLOYMENT=#{ENV["BUNDLE_DEPLOYMENT"]} " if ENV["BUNDLE_DEPLOYMENT"] # Unset on windows since we delete the Gemfile.lock + bundle_command << "BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE=#{ENV["BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE"]} " if bundler.needs_ruby_global_append_path? + bundle_command << "bundle install -j4" + + topic("Installing dependencies using bundler #{bundler.version}") + + bundler_output = String.new("") + bundle_time = nil + env_vars = {} + Dir.mktmpdir("libyaml-") do |tmpdir| + libyaml_dir = "#{tmpdir}/#{LIBYAML_PATH}" + + # need to setup compile environment for the psych gem + yaml_include = File.expand_path("#{libyaml_dir}/include").shellescape + yaml_lib = File.expand_path("#{libyaml_dir}/lib").shellescape + pwd = Dir.pwd + bundler_path = "#{pwd}/#{slug_vendor_base}/gems/#{bundler.dir_name}/lib" + + # we need to set BUNDLE_CONFIG and BUNDLE_GEMFILE for + # codon since it uses bundler. + env_vars["BUNDLE_GEMFILE"] = "#{pwd}/Gemfile" + env_vars["BUNDLE_CONFIG"] = "#{pwd}/.bundle/config" + env_vars["CPATH"] = noshellescape("#{yaml_include}:$CPATH") + env_vars["CPPATH"] = noshellescape("#{yaml_include}:$CPPATH") + env_vars["LIBRARY_PATH"] = noshellescape("#{yaml_lib}:$LIBRARY_PATH") + env_vars["RUBYOPT"] = syck_hack + env_vars["NOKOGIRI_USE_SYSTEM_LIBRARIES"] = "true" + env_vars["BUNDLE_DISABLE_VERSION_CHECK"] = "true" + env_vars["BUNDLER_LIB_PATH"] = "#{bundler_path}" if ruby_version.ruby_version == "1.8.7" + env_vars["BUNDLE_DISABLE_VERSION_CHECK"] = "true" + + puts "Running: #{bundle_command}" + instrument "ruby.bundle_install" do + bundle_time = Benchmark.realtime do + bundler_output << pipe("#{bundle_command} --no-clean", out: "2>&1", env: env_vars, user_env: true) + end + end + end + + if $?.success? + puts "Bundle completed (#{"%.2f" % bundle_time}s)" + log "bundle", :status => "success" + puts "Cleaning up the bundler cache." + instrument "ruby.bundle_clean" do + # Only show bundle clean output when not using default cache + if load_default_cache? + run("bundle clean > /dev/null", user_env: true, env: env_vars) + else + pipe("bundle clean", out: "2> /dev/null", user_env: true, env: env_vars) + end + end + @bundler_cache.store + + # Keep gem cache out of the slug + FileUtils.rm_rf("#{slug_vendor_base}/cache") + else + mcount "fail.bundle.install" + log "bundle", :status => "failure" + error_message = "Failed to install gems via Bundler." + puts "Bundler Output: #{bundler_output}" + if bundler_output.match(/An error occurred while installing sqlite3/) + mcount "fail.sqlite3" + error_message += <<~ERROR + + Detected sqlite3 gem which is not supported on Heroku: + https://devcenter.heroku.com/articles/sqlite3 + ERROR + end + + if bundler_output.match(/but your Gemfile specified/) + mcount "fail.ruby_version_mismatch" + error_message += <<~ERROR + + Detected a mismatch between your Ruby version installed and + Ruby version specified in Gemfile or Gemfile.lock. You can + correct this by running: + + $ bundle update --ruby + $ git add Gemfile.lock + $ git commit -m "update ruby version" + + If this does not solve the issue please see this documentation: + + https://devcenter.heroku.com/articles/ruby-versions#your-ruby-version-is-x-but-your-gemfile-specified-y + ERROR + end + + error error_message + end + end + end + end + + def post_bundler + instrument "ruby.post_bundler" do + Dir[File.join(slug_vendor_base, "**", ".git")].each do |dir| + FileUtils.rm_rf(dir) + end + bundler.clean + end + end + + # RUBYOPT line that requires syck_hack file + # @return [String] require string if needed or else an empty string + def syck_hack + instrument "ruby.syck_hack" do + syck_hack_file = File.expand_path(File.join(File.dirname(__FILE__), "../../vendor/syck_hack")) + rv = run_stdout('ruby -e "puts RUBY_VERSION"').strip + # < 1.9.3 includes syck, so we need to use the syck hack + if Gem::Version.new(rv) < Gem::Version.new("1.9.3") + "-r#{syck_hack_file}" + else + "" + end + end + end + + # writes ERB based database.yml for Rails. The database.yml uses the DATABASE_URL from the environment during runtime. + def create_database_yml + instrument 'ruby.create_database_yml' do + return false unless File.directory?("config") + return false if bundler.has_gem?('activerecord') && bundler.gem_version('activerecord') >= Gem::Version.new('4.1.0.beta1') + + log("create_database_yml") do + topic("Writing config/database.yml to read from DATABASE_URL") + File.open("config/database.yml", "w") do |file| + file.puts <<-DATABASE_YML +<% + +require 'cgi' +require 'uri' + +begin + uri = URI.parse(ENV["DATABASE_URL"]) +rescue URI::InvalidURIError + raise "Invalid DATABASE_URL" +end + +raise "No RACK_ENV or RAILS_ENV found" unless ENV["RAILS_ENV"] || ENV["RACK_ENV"] + +def attribute(name, value, force_string = false) + if value + value_string = + if force_string + '"' + value + '"' + else + value + end + "\#{name}: \#{value_string}" + else + "" + end +end + +adapter = uri.scheme +adapter = "postgresql" if adapter == "postgres" + +database = (uri.path || "").split("/")[1] + +username = uri.user +password = uri.password + +host = uri.host +port = uri.port + +params = CGI.parse(uri.query || "") + +%> + +<%= ENV["RAILS_ENV"] || ENV["RACK_ENV"] %>: + <%= attribute "adapter", adapter %> + <%= attribute "database", database %> + <%= attribute "username", username %> + <%= attribute "password", password, true %> + <%= attribute "host", host %> + <%= attribute "port", port %> + +<% params.each do |key, value| %> + <%= key %>: <%= value.first %> +<% end %> + DATABASE_YML + end + end + end + end + + def rake + @rake ||= begin + rake_gem_available = bundler.has_gem?("rake") || ruby_version.rake_is_vendored? + raise_on_fail = bundler.gem_version('railties') && bundler.gem_version('railties') > Gem::Version.new('3.x') + + topic "Detecting rake tasks" + rake = LanguagePack::Helpers::RakeRunner.new(rake_gem_available) + rake.load_rake_tasks!({ env: rake_env }, raise_on_fail) + rake + end + end + + def rake_env + if database_url + { "DATABASE_URL" => database_url } + else + {} + end.merge(user_env_hash) + end + + def database_url + env("DATABASE_URL") if env("DATABASE_URL") + end + + # executes the block with GIT_DIR environment variable removed since it can mess with the current working directory git thinks it's in + # @param [block] block to be executed in the GIT_DIR free context + def allow_git(&blk) + git_dir = ENV.delete("GIT_DIR") # can mess with bundler + blk.call + ENV["GIT_DIR"] = git_dir + end + + # decides if we need to enable the dev database addon + # @return [Array] the database addon if the pg gem is detected or an empty Array if it isn't. + def add_dev_database_addon + pg_adapters.any? {|a| bundler.has_gem?(a) } ? ['heroku-postgresql'] : [] + end + + def pg_adapters + [ + "pg", + "activerecord-jdbcpostgresql-adapter", + "jdbc-postgres", + "jdbc-postgresql", + "jruby-pg", + "rjack-jdbc-postgres", + "tgbyte-activerecord-jdbcpostgresql-adapter" + ] + end + + # decides if we need to install the node.js binary + # @note execjs will blow up if no JS RUNTIME is detected and is loaded. + # @return [Array] the node.js binary path if we need it or an empty Array + def add_node_js_binary + return [] if node_js_preinstalled? + + if Pathname(build_path).join("package.json").exist? || + bundler.has_gem?('execjs') || + bundler.has_gem?('webpacker') + [@node_installer.binary_path] + else + [] + end + end + + def add_yarn_binary + return [] if yarn_preinstalled? +| + if Pathname(build_path).join("yarn.lock").exist? || bundler.has_gem?('webpacker') + [@yarn_installer.name] + else + [] + end + end + + def has_yarn_binary? + add_yarn_binary.any? + end + + # checks if node.js is installed via the official heroku-buildpack-nodejs using multibuildpack + # @return String if it's detected and false if it isn't + def node_preinstall_bin_path + return @node_preinstall_bin_path if defined?(@node_preinstall_bin_path) + + legacy_path = "#{Dir.pwd}/#{NODE_BP_PATH}" + path = run("which node").strip + if path && $?.success? + @node_preinstall_bin_path = path + elsif run("#{legacy_path}/node -v") && $?.success? + @node_preinstall_bin_path = legacy_path + else + @node_preinstall_bin_path = false + end + end + alias :node_js_preinstalled? :node_preinstall_bin_path + + def node_not_preinstalled? + !node_js_preinstalled? + end + + # Example: tmp/build_8523f77fb96a956101d00988dfeed9d4/.heroku/yarn/bin/ (without the `yarn` at the end) + def yarn_preinstall_bin_path + (yarn_preinstall_binary_path || "").chomp("/yarn") + end + + # Example `tmp/build_8523f77fb96a956101d00988dfeed9d4/.heroku/yarn/bin/yarn` + def yarn_preinstall_binary_path + return @yarn_preinstall_binary_path if defined?(@yarn_preinstall_binary_path) + + path = run("which yarn").strip + if path && $?.success? + @yarn_preinstall_binary_path = path + else + @yarn_preinstall_binary_path = false + end + end + + def yarn_preinstalled? + yarn_preinstall_binary_path + end + + def yarn_not_preinstalled? + !yarn_preinstalled? + end + + def run_assets_precompile_rake_task + instrument 'ruby.run_assets_precompile_rake_task' do + + precompile = rake.task("assets:precompile") + return true unless precompile.is_defined? + + topic "Precompiling assets" + precompile.invoke(env: rake_env) + if precompile.success? + puts "Asset precompilation completed (#{"%.2f" % precompile.time}s)" + else + precompile_fail(precompile.output) + end + end + end + + def precompile_fail(output) + mcount "fail.assets_precompile" + log "assets_precompile", :status => "failure" + msg = "Precompiling assets failed.\n" + if output.match(/(127\.0\.0\.1)|(org\.postgresql\.util)/) + msg << "Attempted to access a nonexistent database:\n" + msg << "https://devcenter.heroku.com/articles/pre-provision-database\n" + end + + sprockets_version = bundler.gem_version('sprockets') + if output.match(/Sprockets::FileNotFound/) && (sprockets_version < Gem::Version.new('4.0.0.beta7') && sprockets_version > Gem::Version.new('4.0.0.beta4')) + mcount "fail.assets_precompile.file_not_found_beta" + msg << "If you have this file in your project\n" + msg << "try upgrading to Sprockets 4.0.0.beta7 or later:\n" + msg << "https://github.com/rails/sprockets/pull/547\n" + end + + error msg + end + + def bundler_cache + "vendor/bundle" + end + + def valid_bundler_cache?(path, metadata) + full_ruby_version = run_stdout(%q(ruby -v)).strip + rubygems_version = run_stdout(%q(gem -v)).strip + old_rubygems_version = nil + + old_rubygems_version = metadata[:ruby_version] + old_stack = metadata[:stack] + old_stack ||= DEFAULT_LEGACY_STACK + + stack_change = old_stack != @stack + if !new_app? && stack_change + return [false, "Purging Cache. Changing stack from #{old_stack} to #{@stack}"] + end + + # fix bug from v37 deploy + if File.exists?("#{path}/vendor/ruby_version") + puts "Broken cache detected. Purging build cache." + cache.clear("vendor") + FileUtils.rm_rf("#{path}/vendor/ruby_version") + return [false, "Broken cache detected. Purging build cache."] + # fix bug introduced in v38 + elsif !metadata.include?(:buildpack_version) && metadata.include?(:ruby_version) + puts "Broken cache detected. Purging build cache." + return [false, "Broken cache detected. Purging build cache."] + elsif (@bundler_cache.exists? || @bundler_cache.old?) && full_ruby_version != metadata[:ruby_version] + return [false, <<-MESSAGE] +Ruby version change detected. Clearing bundler cache. +Old: #{metadata[:ruby_version]} +New: #{full_ruby_version} +MESSAGE + end + + # fix git gemspec bug from Bundler 1.3.0+ upgrade + if File.exists?(bundler_cache) && !metadata.include?(:bundler_version) && !run("find #{path}/vendor/bundle/*/*/bundler/gems/*/ -name *.gemspec").include?("No such file or directory") + return [false, "Old bundler cache detected. Clearing bundler cache."] + end + + # fix for https://github.com/heroku/heroku-buildpack-ruby/issues/86 + if (!metadata.include?(:rubygems_version) || + (old_rubygems_version == "2.0.0" && old_rubygems_version != rubygems_version)) && + metadata.include?(:ruby_version) && metadata[:ruby_version].strip.include?("ruby 2.0.0p0") + return [false, "Updating to rubygems #{rubygems_version}. Clearing bundler cache."] + end + + # fix for https://github.com/sparklemotion/nokogiri/issues/923 + if metadata.include?(:buildpack_version) && (bv = metadata[:buildpack_version].sub('v', '').to_i) && bv != 0 && bv <= 76 + return [false, <<-MESSAGE] +Fixing nokogiri install. Clearing bundler cache. +See https://github.com/sparklemotion/nokogiri/issues/923. +MESSAGE + end + + # recompile nokogiri to use new libyaml + if metadata.include?(:buildpack_version) && (bv = metadata[:buildpack_version].sub('v', '').to_i) && bv != 0 && bv <= 99 && bundler.has_gem?("psych") + return [false, <<-MESSAGE] +Need to recompile psych for CVE-2013-6393. Clearing bundler cache. +See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=737076. +MESSAGE + end + + # recompile gems for libyaml 0.1.7 update + if metadata.include?(:buildpack_version) && (bv = metadata[:buildpack_version].sub('v', '').to_i) && bv != 0 && bv <= 147 && + (metadata.include?(:ruby_version) && metadata[:ruby_version].match(/ruby 2\.1\.(9|10)/) || + bundler.has_gem?("psych") + ) + return [false, <<-MESSAGE] +Need to recompile gems for CVE-2014-2014-9130. Clearing bundler cache. +See https://devcenter.heroku.com/changelog-items/1016. +MESSAGE + end + + true + end + + def load_bundler_cache + instrument "ruby.load_bundler_cache" do + cache.load "vendor" + + full_ruby_version = run_stdout(%q(ruby -v)).strip + rubygems_version = run_stdout(%q(gem -v)).strip + heroku_metadata = "vendor/heroku" + old_rubygems_version = nil + ruby_version_cache = "ruby_version" + buildpack_version_cache = "buildpack_version" + bundler_version_cache = "bundler_version" + rubygems_version_cache = "rubygems_version" + stack_cache = "stack" + + # bundle clean does not remove binstubs + FileUtils.rm_rf("vendor/bundler/bin") + + old_rubygems_version = @metadata.read(ruby_version_cache).strip if @metadata.exists?(ruby_version_cache) + old_stack = @metadata.read(stack_cache).strip if @metadata.exists?(stack_cache) + old_stack ||= DEFAULT_LEGACY_STACK + + stack_change = old_stack != @stack + convert_stack = @bundler_cache.old? + @bundler_cache.convert_stack(stack_change) if convert_stack + if !new_app? && stack_change + puts "Purging Cache. Changing stack from #{old_stack} to #{@stack}" + purge_bundler_cache(old_stack) + elsif !new_app? && !convert_stack + @bundler_cache.load + end + + # fix bug from v37 deploy + if File.exists?("vendor/ruby_version") + puts "Broken cache detected. Purging build cache." + cache.clear("vendor") + FileUtils.rm_rf("vendor/ruby_version") + purge_bundler_cache + # fix bug introduced in v38 + elsif !@metadata.include?(buildpack_version_cache) && @metadata.exists?(ruby_version_cache) + puts "Broken cache detected. Purging build cache." + purge_bundler_cache + elsif (@bundler_cache.exists? || @bundler_cache.old?) && @metadata.exists?(ruby_version_cache) && full_ruby_version != @metadata.read(ruby_version_cache).strip + puts "Ruby version change detected. Clearing bundler cache." + puts "Old: #{@metadata.read(ruby_version_cache).strip}" + puts "New: #{full_ruby_version}" + purge_bundler_cache + end + + # fix git gemspec bug from Bundler 1.3.0+ upgrade + if File.exists?(bundler_cache) && !@metadata.exists?(bundler_version_cache) && !run("find vendor/bundle/*/*/bundler/gems/*/ -name *.gemspec").include?("No such file or directory") + puts "Old bundler cache detected. Clearing bundler cache." + purge_bundler_cache + end + + # fix for https://github.com/heroku/heroku-buildpack-ruby/issues/86 + if (!@metadata.exists?(rubygems_version_cache) || + (old_rubygems_version == "2.0.0" && old_rubygems_version != rubygems_version)) && + @metadata.exists?(ruby_version_cache) && @metadata.read(ruby_version_cache).strip.include?("ruby 2.0.0p0") + puts "Updating to rubygems #{rubygems_version}. Clearing bundler cache." + purge_bundler_cache + end + + # fix for https://github.com/sparklemotion/nokogiri/issues/923 + if @metadata.exists?(buildpack_version_cache) && (bv = @metadata.read(buildpack_version_cache).sub('v', '').to_i) && bv != 0 && bv <= 76 + puts "Fixing nokogiri install. Clearing bundler cache." + puts "See https://github.com/sparklemotion/nokogiri/issues/923." + purge_bundler_cache + end + + # recompile nokogiri to use new libyaml + if @metadata.exists?(buildpack_version_cache) && (bv = @metadata.read(buildpack_version_cache).sub('v', '').to_i) && bv != 0 && bv <= 99 && bundler.has_gem?("psych") + puts "Need to recompile psych for CVE-2013-6393. Clearing bundler cache." + puts "See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=737076." + purge_bundler_cache + end + + # recompile gems for libyaml 0.1.7 update + if @metadata.exists?(buildpack_version_cache) && (bv = @metadata.read(buildpack_version_cache).sub('v', '').to_i) && bv != 0 && bv <= 147 && + (@metadata.exists?(ruby_version_cache) && @metadata.read(ruby_version_cache).strip.match(/ruby 2\.1\.(9|10)/) || + bundler.has_gem?("psych") + ) + puts "Need to recompile gems for CVE-2014-2014-9130. Clearing bundler cache." + puts "See https://devcenter.heroku.com/changelog-items/1016." + purge_bundler_cache + end + + FileUtils.mkdir_p(heroku_metadata) + @metadata.write(ruby_version_cache, full_ruby_version, false) + @metadata.write(buildpack_version_cache, BUILDPACK_VERSION, false) + @metadata.write(bundler_version_cache, bundler.version, false) + @metadata.write(rubygems_version_cache, rubygems_version, false) + @metadata.write(stack_cache, @stack, false) + @metadata.save + end + end + + def purge_bundler_cache(stack = nil) + instrument "ruby.purge_bundler_cache" do + @bundler_cache.clear(stack) + # need to reinstall language pack gems + install_bundler_in_app(slug_vendor_base) + end + end +end diff --git a/spec/integration/dead_end_spec.rb b/spec/integration/dead_end_spec.rb index 34e0f9f..a116121 100644 --- a/spec/integration/dead_end_spec.rb +++ b/spec/integration/dead_end_spec.rb @@ -70,5 +70,25 @@ module DeadEnd 74 end EOM end + + it "re-checks all block code, not just what's visible issues/95" do + file = fixtures_dir.join("ruby_buildpack.rb.txt") + io = StringIO.new + DeadEnd.call( + io: io, + source: file.read, + filename: file + ) + debug_display(io.string) + + expect(io.string).to_not include("def ruby_install_binstub_path") + expect(io.string).to include(<<~'EOM') + ❯ 1067 def add_yarn_binary + ❯ 1068 return [] if yarn_preinstalled? + ❯ 1069 | + ❯ 1075 end + EOM + end + end end From 93f98ce25e51f2c3b160e10f78f6fe6be20b6d2b Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 15:04:28 -0500 Subject: [PATCH 05/10] 28x faster empty line checks on CodeBlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Benchmark: ``` require 'ripper' require 'benchmark/ips' @array = 20.times.map do obj = "puts a + b" def obj.empty? true end def obj.hidden? true end obj end def invalid?(source) source = source.join if source.is_a?(Array) source = source.to_s Ripper.new(source).tap(&:parse).error? end def valid?(source) !invalid?(source) end b = Benchmark.ips do |b| b.report("empty") { @array.all? {|x| x.empty? || x.hidden? } } b.report("ripper") { valid?(@array) } b.compare! end ``` Gives us: ``` Warming up -------------------------------------- empty 59.015k i/100ms ripper 2.064k i/100ms Calculating ------------------------------------- empty 594.981k (± 3.6%) i/s - 3.010M in 5.065759s ripper 20.694k (± 3.3%) i/s - 105.264k in 5.092576s Comparison: empty: 594981.2 i/s ripper: 20693.8 i/s - 28.75x (± 0.00) slower ``` This reduces overall test run time from ~5.3 seconds to 5.1 seconds. It's not a huge win, but it is something. We could try to also use this same strategy in the CodeFrontier where we're likely doing a bunch of redundant parsing (i.e. adding empty lines won't ever change the parse results, but it is expensive to re-parse everything. --- lib/dead_end/code_block.rb | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/dead_end/code_block.rb b/lib/dead_end/code_block.rb index ab81987..6e34cef 100644 --- a/lib/dead_end/code_block.rb +++ b/lib/dead_end/code_block.rb @@ -71,7 +71,20 @@ def invalid? def valid? if @valid == UNSET - @valid = DeadEnd.valid?(lines.map(&:original).join) + # Performance optimization + # + # If all the lines were previously hidden + # and we expand to capture additional empty + # lines then the result cannot be invalid + # + # That means there's no reason to re-check all + # lines with ripper (which is expensive). + # Benchmark in commit message + if lines.all? {|l| l.hidden? || l.empty? } + @valid = true + else + @valid = DeadEnd.valid?(lines.map(&:original).join) + end else @valid end From 1500be50dc18a859b06f526369a95d69326f10c3 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 15:07:31 -0500 Subject: [PATCH 06/10] Re-order and document class calls --- lib/dead_end/display_invalid_blocks.rb | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/dead_end/display_invalid_blocks.rb b/lib/dead_end/display_invalid_blocks.rb index e510f8c..fff6023 100644 --- a/lib/dead_end/display_invalid_blocks.rb +++ b/lib/dead_end/display_invalid_blocks.rb @@ -39,28 +39,33 @@ def call end private def display_block(block) - # Output explanations - ExplainSyntax.new( + # Build explanation + explain = ExplainSyntax.new( code_lines: block.lines - ).call.errors.each do |e| - @io.puts e - end - @io.puts + ).call - ## Output source code + # Enhance code output + # Also handles several ambiguious cases lines = CaptureCodeContext.new( blocks: block, code_lines: @code_lines ).call + # Build code output document = DisplayCodeWithLineNumbers.new( lines: lines, terminal: @terminal, highlight_lines: block.lines ).call - @io.puts(document) + # Output syntax error explanation + explain.errors.each do |e| + @io.puts e + end + @io.puts + # Output code + @io.puts(document) end private def code_with_context From 50f1c51961b3f26180e01f6a0f6923186cc7966b Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 15:25:37 -0500 Subject: [PATCH 07/10] CHANGELOG --- CHANGELOG.md | 5 +++-- spec/integration/dead_end_spec.rb | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78dc793..93ac0b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,11 @@ ## HEAD (unreleased) +- Fix bug causing poor results (fix #95, fix #88) () - [Breaking] Remove previously deprecated `require "dead_end/fyi"` interface (https://github.com/zombocom/dead_end/pull/94) - DeadEnd is now fired on EVERY syntax error (https://github.com/zombocom/dead_end/pull/94) -- Output format changes - - The "banner" is removed in favor of original parse error messages (https://github.com/zombocom/dead_end/pull/94) +- Output format changes: - Parse errors emitted per-block rather than for the whole document (https://github.com/zombocom/dead_end/pull/94) + - The "banner" is now based on lexical analysis rather than parser regex (fix #68, fix #87) () ## 2.0.2 diff --git a/spec/integration/dead_end_spec.rb b/spec/integration/dead_end_spec.rb index a116121..8786c22 100644 --- a/spec/integration/dead_end_spec.rb +++ b/spec/integration/dead_end_spec.rb @@ -89,6 +89,5 @@ module DeadEnd ❯ 1075 end EOM end - end end From 903d434cbf759cb94b26a9a78f733897fa4a0204 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 15:41:27 -0500 Subject: [PATCH 08/10] Standardrb --fix --- lib/dead_end/code_block.rb | 6 +++--- lib/dead_end/explain_syntax.rb | 4 ++-- lib/dead_end/left_right_lex_count.rb | 2 -- spec/integration/dead_end_spec.rb | 7 +++---- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/lib/dead_end/code_block.rb b/lib/dead_end/code_block.rb index 6e34cef..1a197b2 100644 --- a/lib/dead_end/code_block.rb +++ b/lib/dead_end/code_block.rb @@ -80,10 +80,10 @@ def valid? # That means there's no reason to re-check all # lines with ripper (which is expensive). # Benchmark in commit message - if lines.all? {|l| l.hidden? || l.empty? } - @valid = true + @valid = if lines.all? { |l| l.hidden? || l.empty? } + true else - @valid = DeadEnd.valid?(lines.map(&:original).join) + DeadEnd.valid?(lines.map(&:original).join) end else @valid diff --git a/lib/dead_end/explain_syntax.rb b/lib/dead_end/explain_syntax.rb index 05f38a3..7996f84 100644 --- a/lib/dead_end/explain_syntax.rb +++ b/lib/dead_end/explain_syntax.rb @@ -37,7 +37,7 @@ class ExplainSyntax "|" => "|" }.freeze - def initialize(code_lines: ) + def initialize(code_lines:) @code_lines = code_lines @left_right = LeftRightLexCount.new @missing = nil @@ -99,7 +99,7 @@ def errors return RipperErrors.new(@code_lines.map(&:original).join).call.errors end - missing.map {|miss| why(miss) } + missing.map { |miss| why(miss) } end end end diff --git a/lib/dead_end/left_right_lex_count.rb b/lib/dead_end/left_right_lex_count.rb index a7fec87..1430ac8 100644 --- a/lib/dead_end/left_right_lex_count.rb +++ b/lib/dead_end/left_right_lex_count.rb @@ -151,8 +151,6 @@ def missing private def missing_pipe if @count_for_char["|"].odd? "|" - else - nil end end end diff --git a/spec/integration/dead_end_spec.rb b/spec/integration/dead_end_spec.rb index 8786c22..19959d3 100644 --- a/spec/integration/dead_end_spec.rb +++ b/spec/integration/dead_end_spec.rb @@ -4,14 +4,13 @@ module DeadEnd RSpec.describe "Integration tests that don't spawn a process (like using the cli)" do - it "returns good results on routes.rb" do source = fixtures_dir.join("routes.rb.txt").read io = StringIO.new DeadEnd.call( io: io, - source: source, + source: source ) debug_display(io.string) @@ -30,7 +29,7 @@ module DeadEnd io = StringIO.new DeadEnd.call( io: io, - source: source, + source: source ) debug_display(io.string) @@ -52,7 +51,7 @@ module DeadEnd io = StringIO.new DeadEnd.call( io: io, - source: source, + source: source ) debug_display(io.string) From 6196431bd205e42f723d8ee873a028b8939163a8 Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 15:41:27 -0500 Subject: [PATCH 09/10] Faster frontier check Before: ``` $ git co 903d434 $ time be rspec ./spec/ --format=f bundle exec rspec ./spec/ --format=f 4.52s user 1.38s system 97% cpu 6.032 total ``` After: ``` $ git co - $ time be rspec ./spec/ --format=f bundle exec rspec ./spec/ --format=f 4.07s user 1.39s system 97% cpu 5.593 total ``` Benchmarking just that test: Before: ``` 0.899206 0.011891 0.911097 ( 0.911433) ``` After: ``` 0.485635 0.008435 0.494070 ( 0.495051) ``` --- lib/dead_end/code_frontier.rb | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/lib/dead_end/code_frontier.rb b/lib/dead_end/code_frontier.rb index f9e0792..ababae9 100644 --- a/lib/dead_end/code_frontier.rb +++ b/lib/dead_end/code_frontier.rb @@ -54,18 +54,45 @@ def initialize(code_lines:) @code_lines = code_lines @frontier = [] @unvisited_lines = @code_lines.sort_by(&:indent_index) + @has_run = false end def count @frontier.count end + # Performance optimization + # + # Parsing with ripper is expensive + # If we know we don't have any blocks with invalid + # syntax, then we know we cannot have found + # the incorrect syntax yet. + # + # We need to short-circuit this logic for the + # first pass, since passing in valid document + # would result in an infinite loop + # + # This logic gets re-used as well so we have to + # check for when the default value is also the frontier + private def can_skip_check?(block_array) + if block_array == @frontier + if @has_run && !block_array.any?(&:invalid?) + true + else + @has_run = true + false + end + end + end + # Returns true if the document is valid with all lines # removed. By default it checks all blocks in present in # the frontier array, but can be used for arbitrary arrays # of codeblocks as well def holds_all_syntax_errors?(block_array = @frontier) - without_lines = block_array.map do |block| + return false if can_skip_check?(block_array) + + without_lines = block_array.flat_map do |block| block.lines end From 9bfed9b8e45e2c48b53e49a7f4d1cbb9dcf6ec6c Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 1 Nov 2021 20:54:16 -0500 Subject: [PATCH 10/10] Even faster frontier 1.85x Before: ``` 0.518002 0.007275 0.525277 ( 0.526825) ``` After: ``` 0.277188 0.004640 0.281828 ( 0.281940) ``` Instead of checking if the frontier holds all syntax errors any time there's an `invalid?` block in the frontier, only check when an invalid block is added. This prevents redundant checks as invalid blocks that are already on the frontier won't change and therefore won't give different results. This also prevents a pathological performance case where there are two syntax errors, one at the furthest indentation and one at the lowest indentation. --- lib/dead_end/code_frontier.rb | 33 +++++++++++++++---------------- spec/integration/dead_end_spec.rb | 30 ++++++++++++++++------------ spec/spec_helper.rb | 1 + 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/lib/dead_end/code_frontier.rb b/lib/dead_end/code_frontier.rb index ababae9..e068a0b 100644 --- a/lib/dead_end/code_frontier.rb +++ b/lib/dead_end/code_frontier.rb @@ -55,6 +55,7 @@ def initialize(code_lines:) @frontier = [] @unvisited_lines = @code_lines.sort_by(&:indent_index) @has_run = false + @check_next = true end def count @@ -68,20 +69,16 @@ def count # syntax, then we know we cannot have found # the incorrect syntax yet. # - # We need to short-circuit this logic for the - # first pass, since passing in valid document - # would result in an infinite loop - # - # This logic gets re-used as well so we have to - # check for when the default value is also the frontier - private def can_skip_check?(block_array) - if block_array == @frontier - if @has_run && !block_array.any?(&:invalid?) - true - else - @has_run = true - false - end + # When an invalid block is added onto the frontier + # check document state + private def can_skip_check? + check_next = @check_next + @check_next = false + + if check_next + false + else + true end end @@ -89,8 +86,8 @@ def count # removed. By default it checks all blocks in present in # the frontier array, but can be used for arbitrary arrays # of codeblocks as well - def holds_all_syntax_errors?(block_array = @frontier) - return false if can_skip_check?(block_array) + def holds_all_syntax_errors?(block_array = @frontier, can_cache: true) + return false if can_cache && can_skip_check? without_lines = block_array.flat_map do |block| block.lines @@ -147,6 +144,8 @@ def <<(block) @frontier.reject! { |b| b.starts_at >= block.starts_at && b.ends_at <= block.ends_at } + + @check_next = true if block.invalid? @frontier << block @frontier.sort! @@ -169,7 +168,7 @@ def self.combination(array) # the smallest possible set of blocks that contain all the syntax errors def detect_invalid_blocks self.class.combination(@frontier.select(&:invalid?)).detect do |block_array| - holds_all_syntax_errors?(block_array) + holds_all_syntax_errors?(block_array, can_cache: false) end || [] end end diff --git a/spec/integration/dead_end_spec.rb b/spec/integration/dead_end_spec.rb index 19959d3..47174a6 100644 --- a/spec/integration/dead_end_spec.rb +++ b/spec/integration/dead_end_spec.rb @@ -73,20 +73,24 @@ module DeadEnd it "re-checks all block code, not just what's visible issues/95" do file = fixtures_dir.join("ruby_buildpack.rb.txt") io = StringIO.new - DeadEnd.call( - io: io, - source: file.read, - filename: file - ) - debug_display(io.string) - expect(io.string).to_not include("def ruby_install_binstub_path") - expect(io.string).to include(<<~'EOM') - ❯ 1067 def add_yarn_binary - ❯ 1068 return [] if yarn_preinstalled? - ❯ 1069 | - ❯ 1075 end - EOM + benchmark = Benchmark.measure do + DeadEnd.call( + io: io, + source: file.read, + filename: file + ) + + expect(io.string).to_not include("def ruby_install_binstub_path") + expect(io.string).to include(<<~'EOM') + ❯ 1067 def add_yarn_binary + ❯ 1068 return [] if yarn_preinstalled? + ❯ 1069 | + ❯ 1075 end + EOM + end + debug_display(io.string) + debug_display(benchmark) end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index e01a6ac..fd06247 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -3,6 +3,7 @@ require "bundler/setup" require "dead_end" +require "benchmark" require "tempfile" RSpec.configure do |config|