Skip to content

Commit a4c04e6

Browse files
committed
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
1 parent 791417c commit a4c04e6

4 files changed

Lines changed: 185 additions & 1 deletion

File tree

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
## HEAD (unreleased)
22

3+
- Added: Support for detecting `if ... do` (accidental `do` after an `if` or `unless`) (https://github.com/ruby/syntax_suggest/pull/244)
4+
35
## 2.0.3
46

57
- Fix: Correctly identify trailing slashes when using Prism > 1.8.0. (https://github.com/ruby/syntax_suggest/pull/243)

lib/syntax_suggest/explain_syntax.rb

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,48 @@ def errors
111111
return GetParseErrors.errors(@code_lines.map(&:original).join).uniq
112112
end
113113

114-
missing.map { |miss| why(miss) }
114+
out = missing.map { |miss| why(miss) }
115+
out.concat(keyword_do_block_hints)
116+
out
117+
end
118+
119+
# Keywords that do not accept a `do` block
120+
# Note: `while` and `until` DO accept `do`, e.g. `while true do; end`
121+
KEYWORD_NO_DO_BLOCK = %w[if unless].freeze
122+
123+
# Detects lines with a conditional keyword followed by `do`
124+
# (e.g., `if ... do` instead of `it ... do` (common in rspec syntax)
125+
#
126+
# Only triggers when `do` appears after `if`/`unless` but before
127+
# an `end` that would close it. For example:
128+
#
129+
# `if x do; end; end` - triggers (do before if's end)
130+
# `if x; end; foo do; end` - does not trigger (do after if's end)
131+
#
132+
# Returns an array of hint messages
133+
private def keyword_do_block_hints
134+
hints = []
135+
136+
@code_lines.each do |line|
137+
# Stack of unclosed if/unless keywords
138+
kw_stack = []
139+
140+
line.lex.each do |lex|
141+
next unless lex.type == :on_kw
142+
143+
if lex.is_kw? && KEYWORD_NO_DO_BLOCK.include?(lex.token)
144+
kw_stack << lex.token
145+
elsif lex.token == "end" && kw_stack.any?
146+
# An `end` closes the most recent keyword
147+
kw_stack.pop
148+
elsif lex.token == "do" && kw_stack.any?
149+
# `do` appeared while there's still an unclosed if/unless
150+
hints << "Both `#{kw_stack.last}` and `do` require an `end`."
151+
end
152+
end
153+
end
154+
155+
hints.uniq
115156
end
116157
end
117158
end

spec/integration/syntax_suggest_spec.rb

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,32 @@ def bark
209209
EOM
210210
end
211211

212+
it "if .. do" do
213+
source = <<~EOM
214+
describe "something" do
215+
if "does something" do
216+
print "foo"
217+
end
218+
end
219+
EOM
220+
221+
io = StringIO.new
222+
SyntaxSuggest.call(
223+
io: io,
224+
source: source
225+
)
226+
out = io.string
227+
expect(out).to include(<<~EOM)
228+
Unmatched keyword, missing `end' ?
229+
Both `if` and `do` require an `end`.
230+
231+
1 describe "something" do
232+
> 2 if "does something" do
233+
> 4 end
234+
5 end
235+
EOM
236+
end
237+
212238
it "empty else" do
213239
source = <<~EOM
214240
class Foo

spec/unit/explain_syntax_spec.rb

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,121 @@ def meow
177177
expect(explain.errors).to eq([explain.why("end")])
178178
end
179179

180+
# https://github.com/ruby/syntax_suggest/issues/206
181+
it "explains `if` with `do` error" do
182+
source = <<~EOM
183+
describe "something" do
184+
if "does something" do
185+
print "foo"
186+
end
187+
end
188+
EOM
189+
190+
explain = ExplainSyntax.new(
191+
code_lines: CodeLine.from_source(source)
192+
).call
193+
194+
expect(explain.missing).to eq(["end"])
195+
expect(explain.errors).to eq([
196+
explain.why("end"),
197+
"Both `if` and `do` require an `end`."
198+
])
199+
end
200+
201+
it "shows hint for multiple `if`/`unless` with `do` on separate lines" do
202+
source = <<~EOM
203+
describe "something" do
204+
unless "does something" do
205+
print "bar"
206+
end
207+
if "does something" do
208+
print "foo"
209+
end
210+
end
211+
EOM
212+
213+
explain = ExplainSyntax.new(
214+
code_lines: CodeLine.from_source(source)
215+
).call
216+
217+
expect(explain.missing).to eq(["end"])
218+
expect(explain.errors).to eq([
219+
explain.why("end"),
220+
"Both `unless` and `do` require an `end`.",
221+
"Both `if` and `do` require an `end`."
222+
])
223+
end
224+
225+
it "shows hint for innermost unclosed `if` when nested before `do`" do
226+
source = <<~EOM
227+
describe "something" do
228+
unless if "do something" do; end
229+
print "bar"
230+
end
231+
end
232+
EOM
233+
234+
explain = ExplainSyntax.new(
235+
code_lines: CodeLine.from_source(source)
236+
).call
237+
238+
expect(explain.missing).to eq(["end"])
239+
expect(explain.errors).to eq([
240+
explain.why("end"),
241+
"Both `if` and `do` require an `end`."
242+
])
243+
end
244+
245+
it "shows hint for `unless` when inner `if` is closed before `do`" do
246+
source = <<~EOM
247+
describe "something" do
248+
unless if "do something"; end; do
249+
print "bar"
250+
end
251+
end
252+
EOM
253+
254+
explain = ExplainSyntax.new(
255+
code_lines: CodeLine.from_source(source)
256+
).call
257+
258+
expect(explain.missing).to eq(["end"])
259+
expect(explain.errors).to eq([
260+
explain.why("end"),
261+
"Both `unless` and `do` require an `end`."
262+
])
263+
end
264+
265+
it "does not show hint if the `do` is after a method call that might accept a block" do
266+
source = <<~EOM
267+
[1,2,3].map { |x| x * 2 if x > 1 }.each do |y|
268+
puts y
269+
EOM
270+
271+
explain = ExplainSyntax.new(
272+
code_lines: CodeLine.from_source(source)
273+
).call
274+
275+
expect(explain.missing).to eq(["end"])
276+
expect(explain.errors).to eq([
277+
explain.why("end")
278+
])
279+
280+
source = <<~EOM
281+
[1,2,3].map { |x| if x > 1; x * 2; end }.each do |y|
282+
puts y
283+
EOM
284+
285+
explain = ExplainSyntax.new(
286+
code_lines: CodeLine.from_source(source)
287+
).call
288+
289+
expect(explain.missing).to eq(["end"])
290+
expect(explain.errors).to eq([
291+
explain.why("end")
292+
])
293+
end
294+
180295
it "falls back to ripper on unknown errors" do
181296
source = <<~EOM
182297
class Cat

0 commit comments

Comments
 (0)