Project

General

Profile

Actions

Bug #15790

open

Strange interaction between autoload and $LOADED_FEATURES

Added by fxn (Xavier Noria) over 2 years ago. Updated about 1 month ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.6.1p33 (2019-01-30 revision 66950) [x86_64-darwin18]
[ruby-core:92403]

Description

If an autoload fails and we remove its associated file from $LOADED_FEATURES, the autoload is back:

$ cat x.rb
Y = 1 # should be X, emulates a typo

$ cat test.rb
def au
  Object.autoload?(:X).inspect
end

x_rb = File.realpath("x.rb")
autoload :X, x_rb

puts "before failed autoload autoload path is #{au}"

X rescue nil

puts "after failed autoload autoload path is #{au}"

$LOADED_FEATURES.delete(x_rb)

puts "after $LOADED_FEATURES deletion autoload path is #{au}"

The output is

$ ruby -v test.rb
ruby 2.6.1p33 (2019-01-30 revision 66950) [x86_64-darwin18]
before failed autoload autoload path is "/Users/fxn/tmp/x.rb"
after failed autoload autoload path is nil
after $LOADED_FEATURES deletion autoload path is "/Users/fxn/tmp/x.rb"

See? Last line would be expected to print a nil autoload path.

Actions #1

Updated by hsbt (Hiroshi SHIBATA) over 1 year ago

  • Tags set to core, autoload

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

This is still an issue in the master branch. One way to fix this is to delete the autoload if the constant isn't present after the file is loaded. I submitted a pull request for that: https://github.com/ruby/ruby/pull/4715

Updated by Eregon (Benoit Daloze) about 1 month ago

Maybe we should remove the constant as well if we remove the autoload?
I think that would be natural and simpler semantically.
Currently there is some really strange semantics around that, which lead to a lot of complexity (e.g., the concept of "undefined constants":
https://github.com/ruby/spec/blob/e60485b648924cffa3bc48578000fe667a7b6a9e/core/module/autoload_spec.rb#L444-L475

On a related note, currently when requiring a file fails, the autoload is not removed:
https://github.com/ruby/spec/blob/e60485b648924cffa3bc48578000fe667a7b6a9e/core/module/autoload_spec.rb#L408-L442
I think it would be nice if those 2 cases are consistent (i.e., handle any exception from inside autoload the same way).

Updated by fxn (Xavier Noria) about 1 month ago

I deleted my previous comment, misunderstood the situation.

Yeah, in my view if you fire an autoload, the autoload should be removed regardless of the outcome. Additionally, if the constant has not been defined, it should be removed (as undefined) too.

Updated by fxn (Xavier Noria) about 1 month ago

Indeed, the concept of undefined constants for me is dubious. An autoload claims that the constant should be in the given file, but you don't know until the file is evaluated. So, it has been always surprising to me that the constants API acts as if the constant was actually defined for real.

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

I updated the pull request to remove the constant instead of just deleting the autoload. If it passes CI, I'm fine with that approach.

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

jeremyevans0 (Jeremy Evans) wrote in #note-6:

I updated the pull request to remove the constant instead of just deleting the autoload. If it passes CI, I'm fine with that approach.

Removing the constant breaks 4 specs:

ruby 3.1.0dev (2021-08-06T20:59:12Z autoload-delete-af.. 63f803ae45) [x86_64-openbsd6.9]

1)
Module#autoload does not remove the constant from Module#constants if the loaded file does not define it, but leaves it as 'undefined' FAILED
Expected ModuleSpecs::Autoload to have constant 'O' but it does not
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:455:in `block (2 levels) in <top (required)>'
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:34:in `<top (required)>'

2)
Module#autoload after autoloading searches for the constant like the original lookup in lexical scopes if declared in parent and defined in current FAILED
Expected ModuleSpecs::Autoload to have constant 'DeclaredInParentDefinedInCurrent'
but it does not
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:557:in `<module:Autoload>'
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:541:in `block (3 levels) in <top (required)>'
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:34:in `<top (required)>'

3)
Module#autoload after autoloading searches for the constant like the original lookup and fails when finding the undefined autoload constant in the current scope when declared in current and defined in parent FAILED
Expected NameError
but no exception was raised (:declared_in_current_defined_in_parent was returned)
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:573:in `<class:LexicalScope>'
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:571:in `<module:Autoload>'
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:566:in `block (3 levels) in <top (required)>'
/home/jeremy/tmp/ruby/spec/ruby/core/module/autoload_spec.rb:34:in `<top (required)>'

4)
Module#const_set when overwriting an existing constant does not warn if the previous value was undefined FAILED
Expected #<Module:0x0000061748b95cc0> to have constant 'Foo' but it does not
/home/jeremy/tmp/ruby/spec/ruby/core/module/const_set_spec.rb:112:in `block (3 levels) in <top (required)>'
/home/jeremy/tmp/ruby/spec/ruby/core/module/const_set_spec.rb:4:in `<top (required)>'

I pushed a commit to update the specs. Do we consider these specs to be desired behavior or just implementation details? Personally, I strongly dislike having specs for implementation details.

Updated by Eregon (Benoit Daloze) about 1 month ago

Thanks, it's fine to modify these specs.

Their description should still match the tested behavior though,
for "does not remove the constant from Module#constants if the loaded file does not define it, but leaves it as 'undefined'"
since the new behavior is the constant is no longer undefined but actually removes the constant.
So we should duplicate that spec, adapt the description and use version guards.

Do we consider these specs to be desired behavior or just implementation details?

Doesn't seem the best place to discuss this but in short, it is important to have those tests to ensure Ruby implementations are compatible in this regard.
These specs with the version guards are extremely helpful for alternative Ruby implementations to pick that change and understand what changed.

Updated by fxn (Xavier Noria) about 1 month ago

Indeed, to me, this belongs to the contract of these APIs and should be documented.

The documentation of Module#autoload should say what happens if the file is not found, if the file is found but raises, if the file is found and is fine, but does not define the constant. It should also document that a constant for which by now you only have an autoload set is assumed to be a constant of the receiver, etc.

The whole constants API is public interface that needs clearly-defined contracts in the docs. In particular, as you can imagine, Zeitwerk uses these APIs extensively.

Updated by fxn (Xavier Noria) about 1 month ago

BTW, Zeitwerk has its own "Ruby compatibility suite" for language details the implementation relies on. Like, you can override autoloads, remove_const does not execute autoloads, etc.: https://github.com/fxn/zeitwerk/blob/main/test/lib/zeitwerk/test_ruby_compatibility.rb.

Updated by jeremyevans0 (Jeremy Evans) about 1 month ago

Eregon (Benoit Daloze) wrote in #note-8:

Their description should still match the tested behavior though,
for "does not remove the constant from Module#constants if the loaded file does not define it, but leaves it as 'undefined'"
since the new behavior is the constant is no longer undefined but actually removes the constant.
So we should duplicate that spec, adapt the description and use version guards.

OK, I pushed another commit for that.

Actions

Also available in: Atom PDF