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

Bug encountered with Layout/RescueEnsureAlignment Cop and a memoized method #6254

Closed
ashmaroli opened this issue Sep 9, 2018 · 19 comments
Closed

Comments

@ashmaroli
Copy link

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 🐛
/cc @rrosenblum

@ashmaroli
Copy link
Author

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
Copy link
Collaborator

bbatsov commented Sep 9, 2018

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
Copy link
Author

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
Copy link
Author

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

@Drenmi
Copy link
Collaborator

Drenmi commented Sep 10, 2018

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
Copy link
Author

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

@bquorning
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

atomaka commented Sep 11, 2018

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
Copy link
Contributor

andrew-aladev commented Sep 24, 2018

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
Copy link
Contributor

andrew-aladev commented Sep 25, 2018

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.

@Drenmi Drenmi closed this as completed in c8ab8d9 Jan 22, 2019
Drenmi added a commit that referenced this issue Jan 22, 2019
[Fix #6254] Fix RescueEnsureAlignment for non-local assignments
@oehlschl
Copy link

oehlschl commented Feb 7, 2019

@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
Copy link
Author

@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
Copy link

oehlschl commented Feb 7, 2019

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
Copy link

@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
Copy link
Author

@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
Copy link
Author

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

@marcotc
Copy link
Contributor

marcotc commented Feb 22, 2019

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

  1. Left-hand side
    a = begin
     1
    end
  2. Beginning of block
    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
Copy link
Author

@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

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

No branches or pull requests

9 participants