Autofix for Style/StructInheritance

[bubaflub]

Adds an autofix for Style/StructInheritance.

This is my first attempt at writing developing on Rubocop so I'm open to any suggestions. In particular I know that the autofix method is currently violating Metrics/AbcSize and Metrics/MethodLength but I'm unsure of the best way to fix this. Should I pull out some of the fixes in to private methods in the checker? I think that would address at least some of those violations.

I'm also sure there's a better way to do the autofix itself. Removing the class keyword and changing the < operator to an = is simple enough, but I seem to sometimes be losing the do of a block that is passed in. Maybe there's a different part of the AST I should be operating on.

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[buehmann]

Should I pull out some of the fixes in to private methods in the checker?

Makes sense to me. That's what a lot of cops already do. There are only some where the auto-correct logic is so complex that it was extracted into a class (see https://github.com/rubocop-hq/rubocop/tree/master/lib/rubocop/cop/correctors for examples).

but I seem to sometimes be losing the do of a block that is passed in.

Do you have an example for this?

[buehmann]

What if this and the example above are combined? That is: Struct.new takes a block and the class body is not empty?

class Person < Struct.new(:first_name, :last_name) do end
  def age
    42
  end
end

Your auto-correction seems to produce invalid code then:

Person = Struct.new(:first_name, :last_name) do end do
  def age
    42
  end
end

(That might be a pathological example. I have never seen such code before. But still …)

[bubaflub]

Hmmm. Looking at the docs for Struct.new it's not clear to me what the first (empty) block is doing. It looks like that code shouldn't even parse but it definitely does. I think I'm missing something because I thought the only block Struct.new took was the class body.

Is the block of the class body actually part of the Class node and this empty block a parameter to Struct.new?

[bubaflub]

Sorry if this is an ill-formed question, but is there a different mechanism I could use for implementing the autocorrect? What I mean is, rather than manipulating strings with ranges of source locations and flipping the < character to a = character, can I construct a completely new set of AST nodes that will generate the autocorrected code?

I think that might be clearer, though I'm not sure how the original source's formatting would be preserved.

[bubaflub]

Hello again, @buehmann. I'm ready to pick up this PR again and make some revisions. I have some questions about Struct.new syntax -- what block parameters is it taking besides the class body?

Also, is there a different mechanism for me to write my autofix code? Rather than manipulate strings and source locations can I construct a new AST and do the replacement that way?

[buehmann]

Hi @bubaflub, sorry for the late response. Struct.new (like any other method) can take one block only. For Struct.new it can be used to extend the class being defined (by defining methods for the struct, for example).

irb(main):003:0> s = Struct.new(:foo) { def bar; end }
=> #<Class:0x0055c07d8f04b0>
irb(main):004:0> s.instance_methods(false)
=> [:foo, :bar, :foo=]

In the example above, class Person is then a subclass of that class that was constructed by Struct.new.

irb(main):005:0> class Person < s
irb(main):006:1>   def baz; end
irb(main):007:1>   end
=> :baz
irb(main):008:0> Person.instance_methods(false)
=> [:baz]
irb(main):009:0> Person.superclass
=> #<Class:0x0055c07d8f04b0>
irb(main):010:0> Person.superclass.instance_methods(false)
=> [:foo, :bar, :foo=]

The example above was more a pathological one. I cannot imagine someone actually writing such code. But it would be good to guard against such cases and to not attempt autocorrection if Struct.new already has a block.

As to the mechanics of performing auto-corrections: Unfortunately, to my knowledge, in current RuboCop there is no way of transforming the AST in order to rewrite the code. At the moment, it is all low-level string operations on the source text, with all the potential problems that entails. 😶

[bubaflub]

Hi @bubaflub, sorry for the late response.

No worries! Thanks for taking time to respond and helping me better understand Rubocop and the Ruby language.

The example above was more a pathological one... But it would be good to guard against such cases...

Fully agree! Who knows what code lurks out there and I'd rather have an autocorrect not fix something that it might have been able to fix than to an autocorrect change code incorrectly.

I'll rebase this branch, fix up the commit, and add a few more tests.

[bubaflub]

I think this should do it for the auto-correct -- we now ignore the autocorrect if the Struct itself has a block.

Should I have updated config/default.yml to specify AutoCorrect: false or SafeAutoCorrect: false?

[bbatsov]

Should I have updated config/default.yml to specify AutoCorrect: false or SafeAutoCorrect: false?

SafeAutoCorrect: false

[bubaflub]

SafeAutoCorrect: false

@bbatsov: Thanks! I've added that config, rebased off of latest master, updated the CHANGELOG, regenerated the docs, and pushed.

[bbatsov]

@bubaflub And I totally missed your message back then. Mind going over the rebase process one more time?

[bbatsov]

@koic @dvandersluis Would one of you be interested in wrapping up this PR?

[dvandersluis]

@bbatsov the majority of this PR was implemented separately in other PRs, such as #8111.

The only thing missing from this PR is a fix for this case:

class Person < Struct.new(:first_name, :last_name) do def baz; end end
  def age
    42
   end
end

Which I'm not sure we need to even handle? It doesn't look like code that someone would actually write haha.

[bbatsov]

Roger that! Thanks for looking into this! 🙇‍♂️