Bug #18813
closedLet Module#autoload be strict about the autoloaded constant
Description
Introduction¶
Let's consider
module M
autoload :X, 'x'
end
The constants API does not distinguish existing constants from potential constants:
M.constants(false) # => [:X]
M.const_defined?(:X, false) # => true
Expectations¶
For a false
inherited flag, the documentation of Module#constants
says
Returns an array of the names of the constants accessible in mod.
As a Ruby programmer, that is telling me X
belongs to M
, and M::X
is a valid reference.
Therefore, for coherence, if loading x.rb
does not define M::X
, I'd expect that to be a programming error. I'd expect Module#autoload
to raise NameError
with an ad-hoc error message in the line of "file '/full/path/to/x.rb' failed to define the constant M::X`.
This does not happen today, Module#autoload
is a simple trigger that loads a file and execution resumes with whatever side-effects that happened to have. Similar to Module#const_missing
.
However, to me, Module#autoload
is different from Module#const_missing
in a fundamental way. If autoloads were not present in the constants API, both could be equal, but they are present. For consistency with the constants API, I believe there has to be a strict expectation. If the autoload does not define M::X
, the programmer did a mistake and an exception should say so. Zeitwerk does this by hand nowadays.
There is a patch implementing this in https://github.com/ruby/ruby/pull/5949.
Backwards Compatibility¶
Backwards compatibility has to be considered, because some people do, for example:
module MyGem
autoload :OpenSSL, 'openssl'
end
While that is technically allowed, in my opinion that idiom is an unnecessary abuse of autoloading. The autoload is not even scoped to your gem, because
class MyGem::MyClass
OpenSSL
end
would not work. That autoload should be in the top-level, where OpenSSL
is going to be defined (assuming the top-level nesting is empty, you know).
My hunch is that such autoloads may technically exist, but this is a very edge feature and very few people know about it. And those that know, may still define autoloads in their natural place. I'd be surprised if it is widely used.
Anyway, in case you'd like this proposal, whether this change deserves a deprecation cycle is totally your call.
Updated by byroot (Jean Boussier) over 2 years ago
Thanks for opening the ticket @fxn (Xavier Noria).
I'm in favor of this change, however I believe that a deprecation cycle is preferable.
This behavior is a bit of a cruft, but at the same time it's not really breaking much things, and it's not in the way of an optimization or anything like that.
So a warning seems enough to ensure this mis-use will be eliminated over time, a hard break doesn't had value in my opinion.
Updated by ioquatix (Samuel Williams) over 2 years ago
I support raising an error when this fails to load the right constant. I think we can add a warning in 3.2 and make it an error in 3.3.
Updated by byroot (Jean Boussier) over 2 years ago
- Tracker changed from Misc to Bug
- Backport set to 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
Changing this from Misc to bug, and I'll add it to the upcoming dev-meeting.
Updated by matz (Yukihiro Matsumoto) over 2 years ago
I am against the behavior change, because of the compatibility concern. It's OK for me to give warning instead of raising exceptions.
Matz.
Updated by byroot (Jean Boussier) over 2 years ago
- Status changed from Open to Closed
Applied in changeset git|eca31d24d606a73def3674938112dc3c5b79c445.
[Bug #18813] Warn when autoload has to lookup in parent namespace
This is a verbose mode only warning.
Updated by fxn (Xavier Noria) over 2 years ago
Hi @matz (Yukihiro Matsumoto)! I'd like to work on some documentation. Would it be OK for you to say something along the line that declaring autoloads for constants not defined in the receiver is discouraged?