New cop: Detect unexpected overwriting of Struct methods

[ybiquitous]

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

I've encountered an weird behavior on using Struct class:

# good
Good = Struct.new(:foo)
Good.new.members #=> [:foo]

# bad
Bad = Struct.new(:members)
Bad.new.members #=> nil (I've expected `[:members]`)

This is caused by unexpectedly overwriting the Struct#members method.

Describe the solution you'd like

Add a new cop to detect unexpected overwriting such as the example above.

Describe alternatives you've considered

None.

Additional context

Struct includes Enumerable and has many methods e.g. length, count, min, max, or sum, so it seems that some people might use such methods as a Struct member unexpectedly.

[bbatsov]

Sounds like a good Lint cop to me. PRs welcome!

[ybiquitous]

What name is best for us? For example, Lint/StuctMethodOverride?

[Drenmi]

I think linting for the use of members would be very nice. 👍

I'm not so sure about linting for methods on Enumerable. Then where do we draw the line? Do we also lint for methods from Object and BasicObject? Do we also lint regular classes that define methods also to check whether they override built-in methods?

[ybiquitous]

Then where do we draw the line?

Good catch. 👍

What about the way that we pick up some confusing methods? "Confusing" may differ from person to person, but I think we should draw a line somewhere.

For example, we can pick up from the following candidates:

$ ruby -e 'Struct.instance_methods.filter{|m| m.match?(/^[a-z][a-z_]+$/) }.sort.each {|m| puts m}'
chain
chunk
chunk_while
class
clone
collect
collect_concat
count
cycle
deconstruct
deconstruct_keys
define_singleton_method
detect
dig
display
drop
drop_while
dup
each
each_cons
each_entry
each_pair
each_slice
each_with_index
each_with_object
entries
enum_for
extend
filter
filter_map
find
find_all
find_index
first
flat_map
freeze
grep
grep_v
group_by
hash
inject
inspect
instance_eval
instance_exec
instance_variable_get
instance_variable_set
instance_variables
itself
lazy
length
map
max
max_by
members
method
methods
min
min_by
minmax
minmax_by
object_id
partition
private_methods
protected_methods
public_method
public_methods
public_send
reduce
reject
remove_instance_variable
reverse_each
select
send
singleton_class
singleton_method
singleton_methods
size
slice_after
slice_before
slice_when
sort
sort_by
sum
taint
take
take_while
tally
tap
then
to_a
to_enum
to_h
to_s
trust
uniq
untaint
untrust
values
values_at
yield_self
zip

[ybiquitous]

I've thought about this issue for a while, but now I feel it hard that we draw the line on what methods are detected by this cop.

For example, please assume that someone writes the following code:

Commit = Struct.new(:id, :hash)
a_commit = Commit.new(123, "9a4f0ee")
a_commit.hash #=> "9a4f0ee". Not an integer!

The code above violates the Object#hash method's contract (that is, breaks the Object#hash expected behavior), so it can produce bugs somewhere in the program. However, I think that it requires quite experience for the Ruby programming to find such a bug possibility.

And, I don't think that almost programmers write Struct.new(:id, :hash) to override Object#hash. Perhaps people who want to override it may write like this:

Commit = Struct.new(:id) do
  def hash
    # own calculation of a hash...
  end
end

Therefore, I think that it's valuable to check all the methods including Object, BasicObject, and Enumerable.

Also, I think that it's a discussion-required and hard task to pick up what methods we can allow overriding. Every time a method would be added to Struct or Object, we would need to discuss whether we should update the pick-up list or not.

What do you think about this opinion?

---

FYI, here is another interesting example (from this API doc):

Customer = Struct.new(:name, :address, :zip)
joe = Customer.new("Joe Smith", "123 Maple, Anytown NC", 12345)
joe.zip #=> 12345

AnotherCustomer = Struct.new(:name, :address)
tom = AnotherCustomer.new("Tom Smith", "123 Maple, Anytown NC")
tom.zip #=> [["Tom Smith"], ["123 Maple, Anytown NC"]]

Enumerable#zip is overriden! 😳

[ybiquitous]

I've opened the PR #7699, but I think the discussion is open yet. 😃

I hope the PR could promote the discussion about #7680 (comment) or #7680 (comment)!

[pocke]

@ybiquitous Thanks for the great explanation! It is really helpful to understand the problem.

I had the same concern with @Drenmi. It is below.

Do we also lint regular classes that define methods also to check whether they override built-in methods?

But now I think we don't need to lint regular classes. Because of the @ybiquitous's comment.

And, I don't think that almost programmers write Struct.new(:id, :hash) to override Object#hash. Perhaps people who want to override it may write like this:

def hash() end explicitly re-defines the hash method, but Struct.new(:id, :hash) is implicit.
So I think the cop will be valuable.

---

Also, I think that it's a discussion-required and hard task to pick up what methods we can allow overriding. Every time a method would be added to Struct or Object, we would need to discuss whether we should update the pick-up list or not.

Yes, I agree that picking up methods is a hard task. And I guess anyone can't choose proper methods, because the proper methods depend on cases.

So I propose we make the cop to add an offense for all overrides. But we can decrease the severity of the cop, or make the message milder.
The offense message is "Disallow overriding the Struct#%<method_name>s method." in the current implementation, but I feel it's too strong because we can override method by attribute properly, like the zip case.
What about ":hash attribute overrides Object#hash and it may be unexpected"?

[ybiquitous]

@pocke Thanks for your feedback!

we can decrease the severity of the cop, or make the message milder.

I agree completely! 👍

What about ":hash attribute overrides Object#hash and it may be unexpected"?

Looks good! I will try to change the PR. 👍