[ruby-core:120959] [Ruby master Misc#21035] Clarify or redefine Module#autoload? and Module#const_defined?

Issue #21035 has been updated by Eregon (Benoit Daloze).


I recalled and searched for a couple places that rely on `defined?(Foo)` to be `nil` while autoloading `Foo`.

I found https://github.com/ruby/ruby/blob/f32d5071b7b01f258eb45cf533496d82d5c0f6a1/lib/bundler/deprecate.rb#L12-L13

This is actually `if defined?(Bundler::Deprecate) && !autoload?(:Deprecate)` so I think it's safe as long as this change changes both `defined?` and `autoload?` to be unaffected by ongoing autoloads.

I did a gem-codesearch for `autoload?`: https://gist.github.com/eregon/f19ab4347c521f860dc198446113ee01
Not easy to know if they would be affected by this change without looking deeper (which I don't have time for).

I think other places do something like:
```ruby
module M
  unless defined?(Foo)
    Foo = ...
  end
end
```
And this would not be safe with this change, because then `defined?(Foo)` would be true while autoloading that file, and M::Foo would never be assigned, leading to NameError in the caller accessing `M::Foo`.
This pattern seems hard to search with gem-codesearch, `gem-codesearch 'unless defined\?[( ][A-Z]'` gives too many results.
One example, if `M::VERSION` is an autoload, `module M; VERSION = "1.2.3" unless defined? VERSION; end` and that autoload is used, then `M::VERSION` would never be set (it's questionable why there is even that `defined?` check, but that pattern is used a lot, maybe to workaround when some files are loaded with `Kernel#load` and not `require`).
Another example, if YAML is an autoload, `require 'yaml' unless defined?(YAML)` should be fine, it would just trigger the actual loading later.

I also found https://github.com/oracle/truffleruby/blob/f1c77805d8d25d65c5dba811f1e9b3c58f4667ca/lib/patches/rubygems/specification.rb
and I think this actually relies on the current behavior, i.e., if that is being required while we are autoloading `::Gem`, then we need the `else` branch.
IOW it can be useful or even necessary to find out if we are currently autoloading some constant.
That specific usage could be worked around though since it is specific to TruffleRuby and so could use some internals if needed.

It might be interesting to just try the change in a PR and see if it would break anything in ruby/ruby's CI.

----------------------------------------
Misc #21035: Clarify or redefine Module#autoload? and Module#const_defined?
https://bugs.ruby-lang.org/issues/21035#change-111854

* Author: fxn (Xavier Noria)
* Status: Open
----------------------------------------
The documentation for `Module#autoload?` says:

> Returns filename to be loaded if name is registered as autoload in the namespace of mod or one of its ancestors.

As a user, if I declare an autoload, I expect this API:

```ruby
module M
  autoload :Foo, 'foo'

  constants            # => [:Foo]
  const_defined?(:Foo) # => true
  autoload?(:Foo)      # => 'foo'
end
```

That it is indeed how it generally works. Even if the autoload path does not exist. But there is an edge case.

While `constants` does include always `:Foo` as far as I can tell, the return value of `const_defined?` and `autoload?` depends on the stack of features being loaded: The autoload path is resolved and if seen to be in the stack of features being loaded, the predicates return `false` and `nil`, respectively. Do you think that is intuitive?

I find that logic totally unexpected. I just defined an autoload, therefore, I think it would be natural for `autoload?` to return what I just configured. Why should `const_defined?` return nothing but `true`? And why is it not consistent with `constants`?

To me, it would make more sense that in the previous example `const_defined?` returns `true`, and `autoload?` returns `foo` unconditionally (and instantly, nowadays it takes a relative long time due to the lookup).

Now, if the autoload is triggered in a lookup **then** I would expect `Kernel#require` logic to apply. But not when calling some simple predicates.

Please, note that the present behavior is not documented, so on paper the change would not be backwards incompatible.

If, on the other side, it is preferred to keep the behavior as it is, I guess it should be documented with precision (accounting for symlinks, relative paths in `$LOAD_PATH`, etc.)




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- [e-mail]
 To unsubscribe send an email to [e-mail]
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/