From a4c04e6e3e86f40b8294eb7c6e418138d3ecd968 Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 20 Jan 2026 21:05:22 -0600 Subject: [PATCH] Add hint when `if` or `unless` is followed by `do` A typo in RSpec is typing `if` instead of `it`. Depending on the font, they can be hard to distinguish at a glance: ```ruby if "does something" do # Should be `it "does something" do` # ... end ``` This results in a confusing "Unmatched keyword, missing `end'?" error because both `if` and `do` require their own `end`. And some keywords like `while` and `loop` take `do`, so it's not always obvious that `if` or `unless` don't. The message needs to accommodate a scenario where it's unknown if the value inside accepts a block like: ```ruby if method_call do end ``` This might be perfectly valid ruby code, so you wouldn't want to assert "if does not take a do" (since that's not the problem...the problem is that both if and do require an `end` to close, but the code doesn't provide that). Before: ``` Unmatched keyword, missing `end' ? 2 describe "something" do > 3 if "does something" do > 5 end 6 end ``` After: ``` Unmatched keyword, missing `end' ? Both `if` and `do` require an `end`. 2 describe "something" do > 3 if "does something" do > 5 end 6 end ``` Close #206 --- CHANGELOG.md | 2 + lib/syntax_suggest/explain_syntax.rb | 43 ++++++++- spec/integration/syntax_suggest_spec.rb | 26 ++++++ spec/unit/explain_syntax_spec.rb | 115 ++++++++++++++++++++++++ 4 files changed, 185 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc1a0a9..2d45431 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## HEAD (unreleased) +- Added: Support for detecting `if ... do` (accidental `do` after an `if` or `unless`) (https://github.com/ruby/syntax_suggest/pull/244) + ## 2.0.3 - Fix: Correctly identify trailing slashes when using Prism > 1.8.0. (https://github.com/ruby/syntax_suggest/pull/243) diff --git a/lib/syntax_suggest/explain_syntax.rb b/lib/syntax_suggest/explain_syntax.rb index 0d80c4d..babd0ff 100644 --- a/lib/syntax_suggest/explain_syntax.rb +++ b/lib/syntax_suggest/explain_syntax.rb @@ -111,7 +111,48 @@ def errors return GetParseErrors.errors(@code_lines.map(&:original).join).uniq end - missing.map { |miss| why(miss) } + out = missing.map { |miss| why(miss) } + out.concat(keyword_do_block_hints) + out + end + + # Keywords that do not accept a `do` block + # Note: `while` and `until` DO accept `do`, e.g. `while true do; end` + KEYWORD_NO_DO_BLOCK = %w[if unless].freeze + + # Detects lines with a conditional keyword followed by `do` + # (e.g., `if ... do` instead of `it ... do` (common in rspec syntax) + # + # Only triggers when `do` appears after `if`/`unless` but before + # an `end` that would close it. For example: + # + # `if x do; end; end` - triggers (do before if's end) + # `if x; end; foo do; end` - does not trigger (do after if's end) + # + # Returns an array of hint messages + private def keyword_do_block_hints + hints = [] + + @code_lines.each do |line| + # Stack of unclosed if/unless keywords + kw_stack = [] + + line.lex.each do |lex| + next unless lex.type == :on_kw + + if lex.is_kw? && KEYWORD_NO_DO_BLOCK.include?(lex.token) + kw_stack << lex.token + elsif lex.token == "end" && kw_stack.any? + # An `end` closes the most recent keyword + kw_stack.pop + elsif lex.token == "do" && kw_stack.any? + # `do` appeared while there's still an unclosed if/unless + hints << "Both `#{kw_stack.last}` and `do` require an `end`." + end + end + end + + hints.uniq end end end diff --git a/spec/integration/syntax_suggest_spec.rb b/spec/integration/syntax_suggest_spec.rb index 015d088..8bb73f9 100644 --- a/spec/integration/syntax_suggest_spec.rb +++ b/spec/integration/syntax_suggest_spec.rb @@ -209,6 +209,32 @@ def bark EOM end + it "if .. do" do + source = <<~EOM + describe "something" do + if "does something" do + print "foo" + end + end + EOM + + io = StringIO.new + SyntaxSuggest.call( + io: io, + source: source + ) + out = io.string + expect(out).to include(<<~EOM) + Unmatched keyword, missing `end' ? + Both `if` and `do` require an `end`. + + 1 describe "something" do + > 2 if "does something" do + > 4 end + 5 end + EOM + end + it "empty else" do source = <<~EOM class Foo diff --git a/spec/unit/explain_syntax_spec.rb b/spec/unit/explain_syntax_spec.rb index c62a42b..3c06750 100644 --- a/spec/unit/explain_syntax_spec.rb +++ b/spec/unit/explain_syntax_spec.rb @@ -177,6 +177,121 @@ def meow expect(explain.errors).to eq([explain.why("end")]) end + # https://github.com/ruby/syntax_suggest/issues/206 + it "explains `if` with `do` error" do + source = <<~EOM + describe "something" do + if "does something" do + print "foo" + end + 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"), + "Both `if` and `do` require an `end`." + ]) + end + + it "shows hint for multiple `if`/`unless` with `do` on separate lines" do + source = <<~EOM + describe "something" do + unless "does something" do + print "bar" + end + if "does something" do + print "foo" + end + 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"), + "Both `unless` and `do` require an `end`.", + "Both `if` and `do` require an `end`." + ]) + end + + it "shows hint for innermost unclosed `if` when nested before `do`" do + source = <<~EOM + describe "something" do + unless if "do something" do; end + print "bar" + end + 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"), + "Both `if` and `do` require an `end`." + ]) + end + + it "shows hint for `unless` when inner `if` is closed before `do`" do + source = <<~EOM + describe "something" do + unless if "do something"; end; do + print "bar" + end + 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"), + "Both `unless` and `do` require an `end`." + ]) + end + + it "does not show hint if the `do` is after a method call that might accept a block" do + source = <<~EOM + [1,2,3].map { |x| x * 2 if x > 1 }.each do |y| + puts y + 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") + ]) + + source = <<~EOM + [1,2,3].map { |x| if x > 1; x * 2; end }.each do |y| + puts y + 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