Feature #20912: Add warning when redefining __id__ as well as object_id

• Status: Closed
• Assignee: -
• Target version: -

[jhawthorn (John Hawthorn)]

Currently if you create a class and redefine or remove either object_id or __send__ it will issue a warning. There's no such warning on __id__.

❯ ruby -we 'class << Object.new; def object_id = 1; end'
-e:1: warning: redefining `object_id' may cause serious problems
❯ ruby -we 'class << Object.new; def __id__ = 1; end'
❯ 

It makes sense that there's no warning on send, because we expect __send__ to be the method reliably available. __send__ is on BasicObject, send is only on Kernel. This seems a little inconsistent that object_id warns while __id__ does not warn. __id__ is the equivalent to __send__ as it's also on BasicObject, where object_id is only on Kernel.

This may be a more obvious problem in Ruby 3.4.0 as commit:cffd380390967d17899fde0a81f5151ae6ddd076 makes the warning print in more cases.

I propose we change this warning to be emitted only when __id__ is redefined and not when object_id is redefined:

Proposed behaviour:

❯ ruby -we 'class << Object.new; def object_id = 1; end'
❯ ruby -we 'class << Object.new; def __id__ = 1; end'
-e:1: warning: redefining `__id__' may cause serious problems 
❯ 

[jhawthorn (John Hawthorn)]

https://github.com/ruby/ruby/pull/12177

[nobu (Nobuyoshi Nakada)]

Agree that __id__ should be warned.

As for object_id, do you want to redefine it?

[jhawthorn (John Hawthorn)]

nobu (Nobuyoshi Nakada) wrote in #note-2:

As for object_id, do you want to redefine it?

I think it's best not to redefine, but a warning is probably more severe than is needed. I don't think it causes "serious problems", especially because since object_id doesn't exist on BasicObject calling code can't expect all objects to have the default implementation of object_id.

[byroot (Jean Boussier)]

do you want to redefine it?

Shopify has case like this where one of the GraphQL attribute is object_id and that cause a warning to be emitted. I also know the Facebook API has (or used to have) an object_id field.

Overall, it's not very common, but not totally rare either to want to redefine object_id.

[mame (Yusuke Endoh)]

I think it's a good idea to warn against redefining __id__, but I think we should keep warning against redefining object_id as well.

The reason is that it is relatively well known that "we should use __send__ instead of send for unknown objects", but I think it is less well known that "we should use __id__ instead of object_id". In fact, object_id is called far more often than __id__.

$ gem-codesearch \\.object_id | wc -l
24048
$ gem-codesearch \\.__id__ | wc -l
3763

I don't know how many of these call object_id for instances of user-defined classes, but such gems may not work properly if object_id is redefined.

If we really want to allow the redefinition of object_id, we will need to create a consensus that a call to object_id for unknown objects is dangerous, and get people to fix gems so that __id__ is used instead of object_id for unknown objects.
However, if you are currently not in that much trouble, I guess it would be better to maintain the current status.

---

Incidentally, the names __id__ and object_id have an interesting history.

• There used to be only Object#id.
• Object#__id__ was introduced to avoid the redefinition of the method name id.
• Object#object_id was also introduced.
• Object#id was completely removed to allow application authors to use tje name id freely.
• And __id__ and object_id were left.

I asked matz why the new name object_id was introduced when __id__ was already there.
He answered, "I didn't want to have to always use __id__ because it would be ugly".

https://twitter.com/yukihiro_matz/status/1861726864332742995

Indeed, it's not very cool to call __id__ on an object that you know you don't have to worry about redefining.

[byroot (Jean Boussier)]

Is object_id really called that much? I can't really think of something that really depend on it from the top of my head.

[Eregon (Benoit Daloze)]

mame (Yusuke Endoh) wrote in #note-5:

I think it's a good idea to warn against redefining __id__, but I think we should keep warning against redefining object_id as well.

The reason is that it is relatively well known that "we should use __send__ instead of send for unknown objects", but I think it is less well known that "we should use __id__ instead of object_id". In fact, object_id is called far more often than __id__.

I agree and I think this a well thought out argument.
In fact I used object_id plenty of times but almost never __id__.

He answered, "I didn't want to have to always use __id__ because it would be ugly".

Same feeling here, I always prefer to use send/object_id when it's safe, because I think it looks much better/is more readable.

Maybe it would even make sense to warn for send to discourage overriding it, but out of scope and probably not worth it (considering e.g. Socket#send in stdlib).

Is object_id really called that much?

It seems used a lot, yes: https://github.com/search?q=%2F%5C.object_id%5Cb%2F+language%3ARuby+&type=code&p=1
One example use is when overriding inspect.

[byroot (Jean Boussier)]

It seems used a lot, yes:

I meant in context where it is called on an unknown object where object_id may have been redefined and cause the "serious problem" the warning hints at. I've dealt with tons of meta-programing messes over the last decade, I don't remember a case where object_id was involved.

Skimming over these code sample I mostly see code that is just meant to use equal? instead, and some small snipets to showcase whether the object is similar etc.

[matz (Yukihiro Matsumoto)]

I am strongly discourage redefining object_id, so we should keep the warning. I am not against warning __id__ redefinition.

Matz.

[jhawthorn (John Hawthorn)]

• Subject changed from Move warning when redefining object_id to __id__ to Add warning when redefining __id__ as well as object_id

Okay, I will add the warning to __id__ as well as object_id

[jhawthorn (John Hawthorn)]

• Status changed from Open to Closed

Applied in changeset git|f1dda5ed011b79d0d7bd31b09b55b5e19d8abd0c.

---

Warn when redefining id as well as object_id

[Feature #20912]