Rubocop: Bug encountered with `Layout/RescueEnsureAlignment` Cop and a memoized method

[ashmaroli]

Expected behavior

Expected RuboCop to not flag the method.

Actual behavior

$ rubocop test.rb
Inspecting 1 file
C

Offenses:

test.rb:8:5: C: Layout/RescueEnsureAlignment: rescue at 8, 4 is not aligned with begin at 6, 13.
    rescue StandardError
    ^^^^^^

Steps to reproduce the problem

1. Create test.rb:

# frozen_string_literal: true

# Dummy Class
class Foo
  def bar
    @bar ||= begin
      expensive_method
    rescue StandardError
      fall_back
    end
  end
end

1. Run rubocop test.rb

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler. Here's an example:

$ [bundle exec] rubocop -V
0.59.0

Inference

Rubocop recommending to place rescue at position 13 in this example is a distasteful :bug:
/cc @rrosenblum

[ashmaroli]

The autocorrect feature for this Cop is now broken as well.. The offense remains uncorrected

# Session-I

$ rubocop test.rb -a
Inspecting 1 file
C

Offenses:

test.rb:8:5: C: [Corrected]Layout/RescueEnsureAlignment: rescue at 8, 4 is not aligned with begin at 6, 13.
    rescue StandardError
    ^^^^^^
1 file inspected, 1 offense detected, 1 offense corrected

# Session-II

$ rubocop test.rb -a
Inspecting 1 file
C

Offenses:

test.rb:8:5: C: [Corrected]Layout/RescueEnsureAlignment: rescue at 8, 4 is not aligned with begin at 6, 13.
    rescue StandardError
    ^^^^^^
1 file inspected, 1 offense detected, 1 offense corrected

[bbatsov]

Why do you think this is a bug? All layout cops default to layout like:

@bar ||= begin
           expensive_method
         rescue StandardError
           fall_back
         end

(that's alignment with the keyword)

Most cops are configurable and allow you to configure fixed indent with respect to the first line as well (which seems to be what you want).

P.S. The markdown rendering is retarded and I can't get it to show what I want.

[ashmaroli]

I can't get it to show what I want.

@bbatsov I understand that you're recommending the following

# frozen_string_literal: true

# Dummy Class
class Foo
  def bar
    @bar ||= begin
               expensive_method
             rescue StandardError
               fall_back
             end
  end
end

but IMO, that just reduces the readability, IMO..

allow you to configure fixed indent with respect to the first line as well

I could not find any docs on how to configure this cop..

Why do you think this is a bug?

Because to me, the existing code is more readable / visually pleasing:

class Foo
  def bar
    @bar ||= begin
      expensive_method
    rescue StandardError
      fall_back
    end
  end
end

I would've even expected Rubocop to recommend aligning with the def column.. but that's incorrect as well..

[ashmaroli]

Either ways, the autocorrect for this cop is broken..

[Drenmi]

Barring all personal opinion on what is readable and not, I labelled this as a feature request to add a configuration option to make indentation respect the assignment LHS like so:

@bar ||= begin
  expensive_method
rescue StandardError
  fall_back
end

when used as RHS of an assignment.

I added a separate bug report for the auto-correct problem in #6258.

[ashmaroli]

@Drenmi Thank you for getting the ball rolling on the two issues..

[bquorning]

This feature request is similar to how Layout/ElseAlignment works together with Layout/EndAlignment. Consider this code:

foo = if bar
  2
else
  3
end

On its own, ElseAlignment will tell you to indent lines 2–5 six spaces. But if Layout/EndAlignment is configured with e.g. EnforcedStyleAlignWith: variable, ElseAlignment will no longer complain.

I believe RescueEnsureAlignment must include the EndKeywordAlignment module and then change some internals…

[rrosenblum]

I apologize for introducing this new bug. It was introduced while fixing another bug, #6246. I switched the check for alignment to be based on the keyword rather than end. Unfortunately, the cop was missing tests for several important scenarios.

I agree with @Drenmi and @bquorning on handling this with new configuration.

[atomaka]

Barely related, this cop seems oddly opinionated when using rescue with blocks:

[1, 2, 3, 4, 5].
  map do |number|
    number * number
  rescue StandardError => error
    puts "Error: #{error}"
  end.
  each { |number| puts number }

triggers

test.rb:4:3: C: Layout/RescueEnsureAlignment: rescue at 4, 2 is not aligned with [1, 2, 3, 4, 5].
map do at 1, 0.
rescue StandardError => error
^^^^^^

I would expect to align rescue with the map keyword over the data set.

[andrew-aladev]

Old implementation depended on end keyword. @rrosenblum fixed it, see commit fef557a9adc716efaf1596a58b344e82f4fe82e7.

He doesn't break anything, because rescue and ensure shouldn't have the same column as end keyword. end could live it's own life.

Ryan attached rescue and ensure keywords to the beginning of ancestor node. For example

@bar ||= begin
           expensive_method
         rescue StandardError
           fall_back
         end

rescue is aligned with begin. It is implemented just as rescue.set_column alignment_node.get_column (pseudo code).

Today I am introducing fix #6328 for inline access modifiers. It is not related with this issue, but I read this code and can say something about it.

I see you want to receive

@bar ||= begin
  expensive_method
rescue StandardError
  fall_back
end

We need to implement something like baseline method for any RuboCop::AST::Node. It will provide alignment column for some keywords like rescue, ensure, etc. rescue.set_column alignment_node.get_baseline

What do you think, how to implement such method? Math.min(alignment_node.get_begin_column, node.get_end_column)? What options will control baseline behaviour?

Some special nodes like SendNode should have special baseline: child_nodes.first.get_baseline.

We need here any old project developer to ask how to provide baseline method. I think it is not a trivial change.

[andrew-aladev]

We can compare the following code.

[1, 2, 3, 4, 5]
  .map do |number|; end

[1, 2, 3, 4, 5]
  .join.split.map do |number|; end

@bar = begin; end
@bar = begin
  end

I can offer algorithm:

1. Starting from node with target modifier (ex: begin with rescue modifier).
2. Traverse ancestors and find last inline ancestor while prev_ancestor.line == next_ancestor.line
3. Find baseline of node as Math.min(last_inline_ancestor.expression.column, node.end.column).
4. Align modifier with node baseline.

What do you think about it? I don't know whether we should implement it as a mixin for cops or as a method for RuboCop::AST::Node or other way. I am new to rubocop.

[oehlschl]

@Drenmi @ashmaroli is it just me, or does the example from the original comment still fail even after the fix?

# test.rb
class Foo
  def bar
    @bar ||= begin
      expensive_method
    rescue StandardError
      fall_back
    end
  end
end

$ rubocop ./test.rb 
Inspecting 1 file
C

Offenses:

test.rb:5:5: C: Layout/RescueEnsureAlignment: rescue at 5, 4 is not aligned with begin at 3, 13.
    rescue StandardError
    ^^^^^^

1 file inspected, 1 offense detected

$ rubocop -V
0.63.1 (using Parser 2.6.0.0, running on ruby 2.4.5 x86_64-darwin16)

[ashmaroli]

@oehlschl The fix has not released yet. To test, you'll have to point your Gemfile to the master branch of this repository:

# Gemfile

gem 'rubocop', git: 'https://github.com/rubocop-hq/rubocop.git', branch: 'master'

$ bundle install
$ bundle exec rubocop test.rb

[oehlschl]

Thanks @ashmaroli! Should've checked that before asking. I suspected as much, though I was confused by the changelog, since it looked to me like it went into 0.63.0.

Anyway, thanks again all.

[oehlschl]

@ashmaroli @Drenmi not trying to be dense, but it looks like the syntax above still fails in master? I opened a spec-only PR in #6771.

[ashmaroli]

@oehlschl I agree with you. My test case is failing with rubocop-0.64.0.

What's worse is that the autocorrect result is not correct as well:

Original script

# frozen_string_literal: true

# Dummy Class
class Foo
  def bar
    @bar ||= begin
      expensive_method
    rescue StandardError
      fall_back
    end
  end
end

Running RuboCop

rubocop -V
# => 0.64.0 (using Parser 2.6.0.0, running on ruby 2.4.4 x64-mingw32)

rubocop test.rb --auto-correct
Inspecting 1 file
C

Offenses:

test.rb:8:5: C: [Corrected] Layout/RescueEnsureAlignment: rescue at 8, 4 is not aligned with begin at 6, 13.
    rescue StandardError
    ^^^^^^
test.rb:9:7: C: [Corrected] Layout/IndentationWidth: Use 2 (not -7) spaces for indentation.
      fall_back
      ^^^^^^^

1 file inspected, 2 offenses detected, 2 offenses corrected

Result

# frozen_string_literal: true

# Dummy Class
class Foo
  def bar
    @bar ||= begin
      expensive_method
             rescue StandardError
               fall_back
    end
  end
end

[ashmaroli]

@Drenmi I would like this ticket to be re-opened based on above evidence.
Thanks.

[marcotc]

What configuration options are looking at here?
Something like "align everything related to this cop to":

1. Left-hand side
   ruby a = begin 1 end
2. Beginning of block
   ruby a = begin 1 end

There's room for a combinatorial explosion of options here (rescue,begin,def,class, etc.), but I don't think anything complicated will be worth maintaining.

I'm looking into fixing this issue.

[ashmaroli]

@marcotc I did some digging around and realized the issue is actually with Layout/EndAlignment cop. I opened a new ticket for that in #6774. So resolving that issue will resolve this issue for me