Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strange behavior for inline blocks #41

Closed
kavu opened this issue Apr 11, 2013 · 12 comments
Closed

Strange behavior for inline blocks #41

kavu opened this issue Apr 11, 2013 · 12 comments
Labels

Comments

@kavu
Copy link

kavu commented Apr 11, 2013

Just an example:

Code:

[1,2,3].map { |r| "s-#{r}" }.join(',')

Exception:

/usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cop/blocks.rb:32:in `check':  (RuntimeError)
1.rb:1:9: Matching brace not found
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cop/blocks.rb:17:in `block in inspect'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cop/blocks.rb:17:in `each_index'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cop/blocks.rb:17:in `inspect'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cli.rb:62:in `block (2 levels) in run'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cli.rb:57:in `each'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cli.rb:57:in `block in run'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cli.rb:48:in `each'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cli.rb:48:in `run'
    from /usr/local/opt/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/bin/rubocop:12:in `block in <top (required)>'
    from /usr/local/opt/rbenv/versions/2.0.0-p0/lib/ruby/2.0.0/benchmark.rb:296:in `realtime'
    from /usr/local/opt/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/bin/rubocop:11:in `<top (required)>'
    from /usr/local/opt/rbenv/versions/2.0.0-p0/bin/rubocop:23:in `load'
    from /usr/local/opt/rbenv/versions/2.0.0-p0/bin/rubocop:23:in `<main>'

Seems like an bug for me.

By the way, if I add braces around the block (making this code invalid)

[1,2,3].map({ |r| "s-#{r}" }).join(',')

i get:

/usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cop/syntax.rb:13:in `block in inspect': undefined method `captures' for nil:NilClass (NoMethodError)
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cop/syntax.rb:12:in `each_line'
    from /usr/local/var/lib/rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/rubocop-0.4.0/lib/rubocop/cop/syntax.rb:12:in `inspect'

Is this a correct behavior for invalid syntax?

Next one - reduce.

[1,2,3].reduce(0) { |memo, n| memo += n * n }

I get:

C:  1: Space missing after comma.
C:  1: Space missing after comma.

Ehm? What comma? I've already had that comma here.

Hope that helps!

@ismaelga
Copy link

I got the the same problem using String interpolation inside a block. So I guess there should be made some changes on how interpolation braces are identified.

Ehm? What comma? I've already had that comma here.

Commas in [1,2,3], It should be [1, 2, 3]

@jonas054
Copy link
Collaborator

@ismaelga Were you also using Ruby 2.0? My suspicion is that these errors are all related to differences in the output from Ripper in 1.9 and 2.0. They are bugs nonetheless, but it would be interesting to see if they appear in Ruby 1.9 or not.

@ismaelga
Copy link

@jonas054 Yeah. I just tested on 1.9.3 and it worked there. So this seems to be only on 2.0.0

@ismaelga
Copy link

And the output from Ripper is the same on both. So it should be something else

@lloeki
Copy link
Contributor

lloeki commented Apr 12, 2013

Minimal ruby source:

foo { "#{n}" }

I pry'ed blocks.rb, and this is what I get for @reverse_correlations on 1.9.3:

{70102431113960=>[0], 70102431112780=>[2, 10], 70102431121220=>[6]}

Here's the matching @correlations:

{0=>[:program, :method_add_block, :method_add_arg, :fcall, :@ident],
 2=>[:program, :method_add_block, :brace_block],
 10=>[:program, :method_add_block, :brace_block],
 6=>
  [:program,
   :method_add_block,
   :brace_block,
   :string_literal,
   :string_content,
   :string_embexpr,
   :vcall,
   :@ident]}

while this is what I get for @reverse_correlations on 2.0:

{70225979303160=>[0], 70225979322180=>[2], 70225979338680=>[6]}

and the matching @correlations

{0=>[:program, :method_add_block, :method_add_arg, :fcall, :@ident],
 2=>[:program, :method_add_block, :brace_block],
 6=>
  [:program,
   :method_add_block,
   :brace_block,
   :string_literal,
   :string_content,
   :string_embexpr,
   :vcall,
   :@ident]}

So #inspect and #check look correct, just fed with bad data.

Next stop, Cop::Grammar#correlate found via cli.rb#L61 and #L91

@lloeki
Copy link
Contributor

lloeki commented Apr 12, 2013

In Grammar#add_matching_rbrace, @tokens_without_pos contains:

[[:on_ident, "foo"],
 [:on_sp, " "],
 [:on_lbrace, "{"],
 [:on_sp, " "],
 [:on_tstring_beg, "\""],
 [:on_embexpr_beg, "\#{"],
 [:on_ident, "n"],
 [:on_embexpr_end, "}"],
 [:on_tstring_end, "\""],
 [:on_sp, " "],
 [:on_embexpr_end, "}"]]

Since it misses :on_rbrace, it won't be able to match.

in Grammar#initialize, @tokens_without_pos has :on_rbrace before the call to process_embedded_expressions and lacks :on_rbrace right after. This is, I gather, the crux of the problem.

@lloeki
Copy link
Contributor

lloeki commented Apr 12, 2013

I focused too much on the last brace and finally found the cause:

On ruby 2.0, this is what @tokens_without_pos looks like right after being set:

[[:on_ident, "foo"],
 [:on_sp, " "],
 [:on_lbrace, "{"],
 [:on_sp, " "],
 [:on_tstring_beg, "\""],
 [:on_embexpr_beg, "\#{"],
 [:on_ident, "n"],
 [:on_embexpr_end, "}"],
 [:on_tstring_end, "\""],
 [:on_sp, " "],
 [:on_rbrace, "}"]]

Notice how @tokens_without_pos[-4] is not a :on_rbrace, as it is supposed to be according to the process_embedded_expressions code and comment.

It follows that:

  • when :on_rbrace can be changed to when :on_rbrace, :on_embexpr_end
  • the call to process_embedded_expressions is only necessary for 1.9.3 and below.

@lloeki
Copy link
Contributor

lloeki commented Apr 12, 2013

Fixed in PR #46.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 12, 2013

That's some impressive bug hunting you did, @lloeki! You fixed this issue just as I was about to take a closer look at it. Thanks!

@ismaelga
Copy link

@lloeki awesome! 👍

@jonas054
Copy link
Collaborator

Good job!

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 13, 2013

RuboCop 0.4.1 is now out and features that fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants