Feature #18832
closedDo not have class/module keywords consider ancestors of Object
Description
The following code:
module M
class C
end
end
include M
p Object.const_defined?(:C, false)
class C < String # (1)
end
prints false
, as expected, but then raises superclass mismatch for class C (TypeError)
at (1).
I believe this is a bug, because Object
itself does not have a C
constant, so (1) should just work, and the superclasse of M::C
should be irrelevant.
Updated by jeremyevans0 (Jeremy Evans) over 2 years ago
I don't think this is a bug. p Object.const_defined?(:C, false)
explicitly says you don't want to look into ancestors. However class C
at top level will look into ancestors of Object (even class Object::C
will). I don't believe Ruby has syntax for a class opening that doesn't look into ancestors. You would have to do something like: Object::C = Class.new(String)
. Changing class C < String
to ignore Object::C
(i.e. M::C
) is likely to break backwards compatibility significantly.
Updated by fxn (Xavier Noria) over 2 years ago
There is something weird here somewhere.
In general, Ruby does not check the ancestors as far as I can tell. Take the same situation with non-empty nesting:
module M
class C
end
end
module N
include M
class C < String
end
end
That one does not raise, and M::C
and N::C
are different class objects.
This property is what allows you to be anywhere defining a class or module and be sure you are creating regardless of what the ancestors have (think about a chain you don't control like inheriting from ActiveRecord::Base
.
As you know, the class
and module
keyword have their own constant lookup (to decide whether to create or reopen), and they look only in the first element of the nesting without checking its ancestor chain.
Why isn't the top-level acting the same way?
Updated by fxn (Xavier Noria) over 2 years ago
Even more, if you set an autoload for C
in Object
, the autoload is triggered. Which is also consistent with "I have not found the constant in my lookup, but let's trigger the autoload to get it defined hopefully". This is also consistent. (And the autoload raises because it hits what the original example shows.)
To me, this smells like a bug related to some quirk of the top-level object that is leaking.
Updated by jeremyevans0 (Jeremy Evans) over 2 years ago
fxn (Xavier Noria) wrote in #note-3:
Even more, if you set an autoload for
C
inObject
, the autoload is triggered. Which is also consistent with "I have not found the constant in my lookup, but let's trigger the autoload to get it defined hopefully". This is also consistent. (And the autoload raises because it hits what the original example shows.)To me, this smells like a bug related to some quirk of the top-level object that is leaking.
I agree that it would probably be best for things to be consistent. Note that this is not specifically related to top-level, but behavior of Object
specifically, since it happens not just at top-level:
module M
class C
end
end
include M
p Object.const_defined?(:C, false)
class Object
class C < String # still superclass mismatch
end
end
Object
is treated specially during the lookup, that's probably where the difference comes from.
Updated by fxn (Xavier Noria) over 2 years ago
You're right, it happens too if the first element of the nesting is Object
.
For context, this happened in a Rails application that had app/models/comment.rb
, which worked regularly fine. However, in a .rake
file the project had include REXML
at the top-level and in Rake tasks the application could no longer define the Comment
model due to the superclass mismatch. REXML documents such top-level include. This is debatable, but cannot distract from discussing the consistency of the pure lookup logic.
If Object
does not have Comment
, I expect class Comment; end
to define a new class, as in any other "namespace".
Updated by Eregon (Benoit Daloze) over 2 years ago
I agree we shouldn't look in modules prepended/included in Object, only look on the lexical parent itself (not its ancestors).
This is one of these inconsistent "deep constant lookup" exceptions (the other I remember is "deepMethodSearch" used for alias
and public/protected/private
), I think we should fix it.
@fxn (Xavier Noria) I think it'd be useful if you show a Ruby code snippet of the real-world problem to motivate the change.
FWIW, the logic in TruffleRuby to deal with that special case:
https://github.com/oracle/truffleruby/blob/c99ec964fb51f8364c6919467e898db6969f4058/src/main/java/org/truffleruby/language/objects/LookupForExistingModuleNode.java#L63
https://github.com/oracle/truffleruby/blob/c99ec964fb51f8364c6919467e898db6969f4058/src/main/java/org/truffleruby/core/module/ModuleOperations.java#L202
Updated by fxn (Xavier Noria) over 2 years ago
@Eregon (Benoit Daloze) sure. The real-world problem that originated this ticket can be found here. It is the one I briefly described above.
Updated by Eregon (Benoit Daloze) over 2 years ago
@fxn (Xavier Noria) Yes, but that is too long. What I think is useful for the dev meeting is a snippet of a few lines, which represents and reproduces the real issue (and Ruby code is often much clearer than a textual description of it).
e.g. I find all the examples with M/C above too synthetic and hard to judge if it matters.
Updated by fxn (Xavier Noria) over 2 years ago
@Eregon (Benoit Daloze) Is this one better suited?
require "active_record"
require "rexml"
include REXML
class Comment < ActiveRecord::Base # superclass mismatch for class Comment (TypeError)
end
That is the gist of what happens in the ticket.
Updated by fxn (Xavier Noria) over 2 years ago
It is super important to highlight that the problem is not the behavior isolated, but that this is unexpected because it does not work like this for any other cref.
Updated by fxn (Xavier Noria) over 2 years ago
A good exercise is to try to document this (all lookups should be documented). Something like:
In the
class/module
keywords, the constant is checked in the cref. If present, the object is retrieved. If it is a class/module respectively, reopened, otherwise, error condition. If not present, a corresponding object is created and assigned to the constant in the cref. In the case ofObject
, ancestors are checked also and the object reopened if found because... WHY?
Does the team believe this documentation makes sense?
Updated by Eregon (Benoit Daloze) over 2 years ago
Yes, I think that's a great example why we should fix this.
Anyone should still be able to define a Comment
(defined as a constant of Object
) class with just class Comment
at the top-level, even if with REXML::Comment
+ include REXML
.
BTW, I thought class Object::Comment < String; end
might be a workaround but it does not (as Jeremy found out as well above):
$ ruby -v -rrexml -e 'include REXML; class Object::Comment < String; end'
ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
-e:1:in `<main>': superclass mismatch for class Comment (TypeError)
This is also very surprising (the intention is to define Object::Comment
with a superclass of Object
):
$ ruby -v -rrexml -e 'include REXML; class Comment; end; p Comment.ancestors'
ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
[REXML::Comment, Comparable, REXML::Child, REXML::Node, Object, REXML, PP::ObjectMixin, Kernel, BasicObject]
Updated by jeremyevans0 (Jeremy Evans) over 2 years ago
- Tracker changed from Bug to Feature
- Subject changed from Suspicious superclass mismatch to Do not have class/module keywords consider ancestors of Object
- Backport deleted (
2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN)
As I initially expected, this is not a bug. Having class/module keywords consider ancestors of Object is by design, with explicit code for just this purpose, and there is both a test and spec for the current behavior. Switching to feature request.
That being said, I do think it would be best to change the behavior, so I submitted a pull request to do so: https://github.com/ruby/ruby/pull/6048 . I'll add this as a topic for the next developer meeting.
Updated by fxn (Xavier Noria) over 2 years ago
Awesome, thanks Jeremy.
Updated by mame (Yusuke Endoh) over 2 years ago
We discussed this ticket at the dev meeting.
@matz (Yukihiro Matsumoto) said he wanted to try the change, i.e., include M; class C; end
at the toplevel should define ::C
instead of reopening M::C
. He may revisit the change if a compatibility issue is found.
Updated by jeremyevans (Jeremy Evans) over 2 years ago
- Status changed from Open to Closed
Applied in changeset git|12ac8971a394118a57640299f654e46e763093fa.
Do not have class/module keywords look up ancestors of Object
Fixes case where Object includes a module that defines a constant,
then using class/module keyword to define the same constant on
Object itself.
Implements [Feature #18832]