Consider flagging all uses of OpenStruct

[mttkay]

Is your feature request related to a problem? Please describe.

OpenStruct has been considered problematic for a while now; with the arrival of Ruby 3, those problems have compounded enough for a "Caveats" section to be added: https://docs.ruby-lang.org/en/3.0.0/OpenStruct.html#class-OpenStruct-label-Caveats. In fact, it states at the end:

For all these reasons, consider not using OpenStruct at all.

In fact, we found another (undocumented) issue with this class while working on the Ruby 3 migration at GitLab: an application settings test fake inherited from OpenStruct and mixed in other module methods, so as to fake settings keys on code paths where they do not matter.

We found that in Ruby 3, method dispatch in OpenStruct works differently: instead of struct methods being defined lazily upon first access, they are now defined eagerly in the initializer. This means that if a class inheriting from OpenStruct attempts to override methods it expects to be dispatched to this struct (such as those included from other modules), the original struct methods would be called instead of the manually defined methods (and all return nil), leading to surprising behavior that is inconsistent with ordinary method dispatch in superclass hierarchies.

We therefore started to write a Cop that also flags classes inheriting from OpenStruct: https://gitlab.com/gitlab-org/gitlab-styles/-/merge_requests/91

However, given the sheer number of potential issues with this class, I think it could be a good idea to flag any use of it outright. Its behavior can typically be approximated by use of test doubles, hashes, or Structs.

Describe the solution you'd like

Include a Cop that flags any use of OpenStruct as potentially problematic. This could be disabled by default.

Describe alternatives you've considered

A more specific Cop that only flags the additional problem we found, which is broken inheritance chains when inheriting from OpenStruct: https://gitlab.com/gitlab-org/gitlab-styles/-/merge_requests/91

[mttkay]

Here is a proposal for how a cop could be implemented that flags all uses of OpenStruct: https://gitlab.com/gitlab-org/gitlab-styles/-/merge_requests/92

If there is enough demand, I'm happy to look into how this could be upstreamed?

[mttkay]

Side note: that would also subsume (and therefore supersede) the existing Performance/OpenStruct cop, since there are other issues than just performance with that class.

[dvandersluis]

I agree with this cop concept, and I have been removing OpenStruct from my own codebase recently. I think it should probably be a Style cop like Style/StructInheritence. If users choose to disable it, Performance/OpenStruct still makes sense IMO.

[mttkay]

Thanks for the feedback. I've received similar feedback on the GitLab MR so far. I can turn the issue into a pull request so folks have something more tangible to look at, and we can go from there?

[mttkay]

WIP PR: #10217

[mildred]

However, given the sheer number of potential issues with this class, I think it could be a good idea to flag any use of it outright. Its behavior can typically be approximated by use of test doubles, hashes, or Structs.

We are using OpenStruct in our codebase for simple things, and this does not belongs to tests. We cannot replace them with doubles. Hashes does not provides a nice method interface like OpenStruct and Struct needs you to define your structure schema beforehand.

Example of code that uses OpenStruct that I believe is completely harmless:

    def self.glyphs
      OpenStruct.new(
        action: OpenStruct.new(
          edit_item: 'pen',
          edit_line: 'edit',
          copy_item: 'copy',
          cut_item: 'cut',
          paste_item: 'paste',
        ),
        import: 'cloud-download-alt',
        info: 'info'
      )
    end

We use it to bundle information about enums too:

  FOLDER          = OpenStruct.new(id: 0, name: 'folder',      glyph: 'folder',       export_id: 'F')
  PART            = OpenStruct.new(id: 1, name: 'part',        glyph: 'puzzle-piece', export_id: 'P')

[mttkay]

@mildred what you describe can be done entirely with Struct:

Action = Struct.new(
  :edit_item,
  :edit_line,
  :copy_item,
  :cut_item,
  :paste_item,
  keyword_init: true
)

Glyph = Struct.new(
  :action,
  :import,
  :info,
  keyword_init: true
)

def self.glyphs
  Glyph.new(
    action: Action.new(
      edit_item: 'pen',
      edit_line: 'edit',
      copy_item: 'copy',
      cut_item: 'cut',
      paste_item: 'paste',
    ),
    import: 'cloud-download-alt',
    info: 'info'
  )
end

In fact, your example does not even utilize the principle use case for OpenStruct, which is synthesizing fields on demand:

os = OpenStruct.new
os.a = 1

The above is not possible to do with Struct, and it is this behavior that makes OpenStruct difficult to work with and maintain.

[mildred]

Here, in your example, there are a lot of repetitions. You need to declare the struct type first, which leads to much more repetition than when using OpenStruct.

[mttkay]

Fair enough. In that case, why not just silence the rule?

[ReganRyanNZ]

Without fully understanding the problem.. is this a legit substitute? (rubocop is fine with this even when flagging OpenStruct)

Struct.new(:name, :number).new("Dennis", "123456")

It's not as pretty as OpenStruct.new(name: "Dennis", number: "123456"), but gets the job done for 99% of my OpenStruct use cases. If it is legit, then can this monkeypatch replace OpenStruct?

class BetterStruct
  def self.new(**args)
    Struct.new(*(args.keys)).new(*(args.values))
  end
end

BetterStruct.new(name: "Dennis", number: "123456")

This keeps the prettiness of OpenStruct, and avoids OpenStruct. Am I on the right path here, or just reintroducing the same problems?

[mttkay]

@ReganRyanNZ Your example is completely legit and is in fact the recommended way to replace most uses of OpenStruct.

You also don't need BetterStruct; there is a flag you can set when creating a Struct to pass constructor arguments as kwargs:

Person = Struct.new(:name, :number, keyword_init: true)
dennis = Person.new(name: "Dennis", number: "123456")

Hope that helps!