Rubocop: Unexpected Layout/ClosingParenthesisIndentation behavior with non-standard indententation

[jeremywadsack]

I'm not sure how this doesn't fail in the specs but with 0.57.1, the basic "good" case for Layout/ClosingParenthesisIndentation raises an error.

This looks like it's related to changed for Issue #4136 (I see no related PR). Likely something in 68609e6 which appears to be the (large) commit related to that effort.

---

Expected behavior

Given the following file:

# frozen_string_literal: true

some_method(
    a,
    b
)

I would expect rubocop to accept that code. It's _exactly_ what is described in the documentation example.

Actual behavior

Rubocop raises an error:

$ rubocop close-paren.rb 
Inspecting 1 file
C

Offenses:

close-paren.rb:6:1: C: Layout/ClosingParenthesisIndentation: Indent ) to column 2 (not 0)
)
^

1 file inspected, 1 offense detected

Steps to reproduce the problem

See above.

RuboCop version

$ rubocop -v
0.57.1

[Drenmi]

It's exactly what is described in the documentation example.

It's not exactly the same though. In the documentation example the arguments are indented one level, not two. I guess there are two assumptions we can make:

1. The arguments are indented too far.
2. The closing parenthesis is not indented far enough.

and we're making the second assumption.

WDYT?

[jeremywadsack]

Ah, that explains why the error didn't show up in the specs.

The indentation of the parameters is a matter of style controlled by Layout/FirstParameterIndentation.

We use this in our styles:

Layout/FirstParameterIndentation:
  IndentationWidth: 4

My understanding of the rule is that the closing parenthesis should be indented to match the start of the line that opened it. That's what the rule used to say.

This looks good to me:

some_method(
    a,
    b
)

This looks bad:

some_method(
    a,
    b
  )

So Layout/ClosingParenthesisIndentation should not be making assumptions about the indentation as that's controlled by another rule, right?

[oehlschl]

Attempting to upgrade tonight and am experiencing the same issue with:

Layout/FirstParameterIndentation: {IndentationWidth: 4}
Layout/IndentationWidth: {Width: 2}

Now that ClosingParenthesisIndentation relies on the value of IndentationWidth, it does seem to undermine the value of separate IndentationWidth options on things like FirstParameterIndentation.

[oehlschl]

Would it be a quick and amenable solution to make the "outdentation witdth" of ClosingParenthesisIndentation configurable but still default it to Layout/IndentationWidth?

[jeremywadsack]

As this is related to the parentheses for a method call, it seems to be that it should be looking at Layout/FirstParametersIndentation to find IndentationWidth, right? @jfelchner what are your thoughts (as you made the changes)?

In the meantime we've pinned rubocop to = 0.55.0 until this is resolved.

[jfelchner]

@jeremywadsack I would be pretty shocked if I had hard coded indentation at 2 spaces. My assumption is that it’s looking _somewhere_ for that value, but just probably not where you’re expecting. I agree that it should be looking at the same place that FirstParametersIndentation is looking.

Sorry, I’m not in front of my computer right now.

[jfelchner]

@Drenmi so the reference where it's picking up the width is here

So if IndentationWidth is set properly, I don't see why it would be grabbing the 2.

[jeremywadsack]

@jfelchner Are you saying that if we have configured Layout/FirstParameterIndentation as

Layout/FirstParameterIndentation:
  IndentationWidth: 4

we should be configuring the Layout/ClosingParenthesisIndentation as

Layout/ClosingParenthesisIndentation:
  IndentationWidth: 4

I can't find that documented, and it seems weird to me because it would imply to me that was the _indentation_ width for the _closing parenthesis_, not the _outdentation_ width.

[jfelchner]

@jeremywadsack it currently uses the indentation width of the IndentationWidth cop as the outdented width.

Changing the configuration value to be OutdentationWidth for a single cop is probably not going to happen. They all use the same shared code.

I do agree that changing the cop to look at the indentation width of FirstParameterIndentation makes sense.

[jeremywadsack]

Oh, the global indentation setting. Sorry, I didn't mean to imply changing that name.

[jfelchner]

@jeremywadsack a PR changing that line that I linked to to look at FirstParameterIndentation's IndentationWidth first, then the global IndentationWidth, then default to 2 should be easily accepted.

@Drenmi do you know if, when you ask a cop that uses an IndentationWidth inherited from the IndentationWidth cop, if you get nil, ~ or the actual value?

[jeremywadsack]

@jfelchner I haven't contributed to rubocop yet, so I think it would be faster for you to make this change as you know the code, already have a dev/test environment configured, and would know where to put the tests.

[jfelchner]

I’m not going to have time to do this for a while. All you have to do is fork, clone and bundle install.

[aried3r]

@jeremywadsack a PR changing that line that I linked to to look at FirstParameterIndentation's IndentationWidth first, then the global IndentationWidth, then default to 2 should be easily accepted.

If I understood correctly, it might not be so easy, since FirstParameterIndentation does not have a config, but calculates it:
https://github.com/rubocop-hq/rubocop/blob/777f1360e8612656a34c9191e61071174460dd1c/lib/rubocop/cop/layout/first_parameter_indentation.rb#L153

I'm not super familiar with the RuboCop source code, but it doesn't seem there's some low hanging fruit like

@config.for_cop('FirstParameterIndentation')['Width'] || @config.for_cop('IndentationWidth')['Width'] || 2

is there?

[jeremywadsack]

At this point we have just disabled the cop until this can be resolved. I have no idea how you would go about doing this.

# .rubocop.yml
Layout/ClosingParenthesisIndentation:
  # Disable until https://github.com/rubocop-hq/rubocop/issues/5975 is resolved
  Enabled: false

[maxh]

I've opened a PR to fix this here: https://github.com/rubocop-hq/rubocop/pull/6921

Please review the docs and confirm that's the behavior desired!

[jfelchner]

@Drenmi other than possible naming changes and/or philosophical architectural issues, this change looks good to me.

[maxh]

I suggest renaming this bug to "Unexpected Layout/ClosingParenthesisIndentation behavior with non-standard indent configs". The current title suggests the cop is broken in the general case, but in my team's codebase we use 2-space indent everywhere, and we don't have any issues.

For those of you experiencing issues, it would be useful to know what the value of IndentationWidth is for these rubocop.yml file blocks:

ClosingParenthesisIndentation
IndentFirstParameter (which mostly delegates to MultilineElementIndentation)
IndentationWidth

Looking at the source code for each of these cops, if you set IndentationWidth to the same value for these three, the issue _might_ be resolved. Admittedly it's a bit clunky, but my suspicion is that should address the issue.

I think a proper fix here would be to refactor the cops above to use a consistent indentation pattern.

I'm closing the PR I opened since it's hacky and adds more API surface without fixing the underlying issue. I also found better way to fix the ancillary problem in our codebase that motivated me to open that PR originally, per this comment:

https://github.com/rubocop-hq/rubocop/pull/6921#issuecomment-491310765

As a next step, it probably makes sense for someone affected by this issue to dig in and try to get the behavior you want. All the comments above still seem relevant. (fwiw RuboCop development is super fun, quick feedback cycle etc!)

[jeremywadsack]

@maxh, sorry but that still doesn't fix it:

$ rubocop --version
0.71.0

$ cat 5975.rb 
# frozen_string_literal: true

some_method(
    a,
    b
)

$ cat .rubocop.yml 
Layout/IndentFirstArgument:
  Enabled: true
  EnforcedStyle: consistent
  IndentationWidth: 4

Layout/IndentFirstParameter:
  Enabled: true
  EnforcedStyle: consistent
  IndentationWidth: 4

Layout/AlignParameters:
  Enabled: true

Layout/AlignArguments:
  Enabled: true

Layout/ClosingParenthesisIndentation:
  Enabled: true

$ rubocop 5975.rb 
Inspecting 1 file
C

Offenses:

5975.rb:6:1: C: Layout/ClosingParenthesisIndentation: Indent ) to column 2 (not 0)
)
^

1 file inspected, 1 offense detected

---

I guess I'm confused about the difference between these two cops:

Layout/IndentFirstArgument

This cop checks the indentation of the first argument in a method call or definition.

Layout/IndentFirstParameter

This cop checks the indentation of the first parameter in a method call. Parameters after the first one are checked by Layout/AlignParameters, not by this cop.

Beyond the inconsistency of "argument" vs. "parameter" why do we need two cops?

[jfelchner]

@jeremywadsack and what if you add:

Layout/IndentationWidth:
  Width: 4

I think your issue lies from the fact that you're using a non-standard indentation width but you're not telling Rubocop that you're doing so.

If you add the above, you should be able to remove the other IndentationWidth configs from your config above.

[maxh]

Beyond the inconsistency of "argument" vs. "parameter" why do we need two cops?

I just created this PR to explain:
https://github.com/rubocop-hq/rubocop/pull/7158/files#diff-a84499718ddc87fd24a1cb3dc7d46a3dR10

When I first started looking into this, IndentFirstArgument didn't actually fix method definitions, only calls, which led us to see issues symptomatically similar to yours in our codebase. But I was able to fix those by adding IndentFirstParameter to fix the indents of params in method definitions. It might have been possible to extend the existing IndentFirstArgument logic to support definitions, but the code was quite complex, and most of the options exposed in the API don't apply to definitions. So it made more sense to make a new cop.

[jeremywadsack]

@jfelchner the problem with that is that we're not using 4 spaces for indentation. We just want parameters for multi-line method calls to be indented 4 spaces.

e.g.

class A
  def sum(a, b)
    a + b
  end

  def three
    sum(
        1,
        2
    )
  end
end

I realize this is non-standard. I suppose what we really should do is use the standard 2-space indentation for multi-line. That would certainly make the code more in-line with expectations of in-coming developers.

I guess I'd be willing to close this as being unsupported because we're non-standard, except that then I'm not sure what the point of "IndentationWidth" is for those cops if it doesn't work with other cops.

@maxh That PR is perfect. Thanks for the clarification.

[jfelchner]

@jeremywadsack wow, I think "non-standard" is the very polite way of putting that. 😂

IndentationWidth is indeed the thing that would do that, but for ClosingParenthesisIndentation, it's going to use that width for ALL closing parenthesis, so in your case, if you're only indenting 4 spaces on parameters, then you'll end up with errors on other invocations like this:

def add_and_divide(one, two, three, divisor)
  (
    one +
    two +
    three
  ) / divisor
end

The above example is contrived but illustrates the point.

In this case your team _wouldn't_ want the closing paren to be outdented by 4 spaces, you would only want it outdented by 2.

So yes, if I were you all, I would either say "We're using 4 spaces everywhere." or "We're using 2 spaces everywhere." And in my opinion you should do that regardless of whether Rubocop could accommodate that style or not... because that's some weird stuff. 😉

[jeremywadsack]

@jfelchner: We use 4 spaces for all set-like continuation. e.g. Method calls, arrays, hashes. This visually distinguishes it from 2-line indentation for control flow. I'm not sure where we picked up the style, likely some much older version of the style guide that's since been changed.

There were 6 other people who 👍'd this issue. I'm closing this as "obscure" but if anyone else has any concerns we can re-open.

[donv]

Using 4 spaces for a line continuation is pretty standard and is the standard in RubyMine. Indenting multi-line arguments with 2 levels to distinguish them from control flow is also common in Ruby and is the standard in Java as well.

Having the closing parenthesis on the same level as the start of the line of the opening parenthesis is also the norm.