@fxn if you have time to fix the tests, looks like we can merge this.

On it!

@casperisfine what do you think about raising NameError versus raising a dedicated new subclass of NameError?

Could you add a spec under spec/ruby for this? Would also help to see what the message looks like.

@casperisfine what do you think about raising NameError versus raising a dedicated new subclass of NameError?

I'd say a subclass is only worth it if there a good use case for rescuing it particularly. Otherwise a different message is good enough.

@casperisfine but Shopify rescues Zeitwerk::NameError no?

@fxn, no that was for require_dependency in Rails. Shopify never did anything special with theses.

New specs up!

I think this is a good change. When you define an autoload, in my opinion it is right to require the constant you claimed to be defined, and exactly in the receiver. Also, cool that the internal metadata is deleted to leave a clean internal state. I believe this will fix https://bugs.ruby-lang.org/issues/15790 too.

I kept the existing specs, and added new ones for 3.2. Please let me know if you prefer to delete them instead.

in my opinion it is right to require the constant you claimed to be defined, and exactly in the receiver.

Could you clarify this?

Because I've seen code such as:

module AppNamespace
  autoload :TopLevelGemNameSpace, "gem_name"
end

As a way to lazy load a gem. So changing this would break code. (I don't remember where I've seen this though, but I've seen it more than once).

@byroot This is what this patch does:

module Foo
  autoload :Date, 'date'
  Date # => autoload "date" failed to resolve Foo::Date (NameError)
end

It is also what Zeitwerk has been enforcing for three years already.

To me, this makes sense.

This is what this patch does

Ok, so we meant the same thing. And yes, some code rely on that today. I'll see if I can find some real work example next week.

Take this too in current Ruby:

module Foo
  autoload :Date, 'date'

  p Foo.constants # => [:Date]
  Date
  p Foo.constants # => []
end

Makes no sense! Because autoload is not fully aligned with Ruby semantics in this edge case, in my view.

Foo.constants contains :Date because you promised it is a constant in Foo, only you install a trigger to autoload it. Then it is gone? How's that possible?

This patch makes semantics more consistent.

This patch makes semantics more consistent.

I don't disagree, I'd go as far as to say I agree, but backward compatibility is taken very seriously by ruby-core, so this MUST be discussed and approved.

If it's approved I'll happily go fix the gems that depends on this, but I don't think we "have the mandate" to break this yet.

I don't disagree, I'd go as far as to say I agree

Hahaha.

his MUST be discussed and approved.

Oh, absolutely.

One example I finally remembered is Thor (vendored in bundler) https://github.com/rubygems/rubygems/blob/96e5cff3df491c4d943186804c6d03b57fcb459b/bundler/lib/bundler/vendor/thor/lib/thor/runner.rb#L9

Similarly there's this Gem::Requirement autoload in Gem::Version: https://github.com/rubygems/rubygems/blob/a9e305432955f56b847881187a7e01a97b6901e3/lib/rubygems/version.rb#L156

Breaking rubygems is a pretty big deal :/

I mentioned https://bugs.ruby-lang.org/issues/15790 in a comment above. That was fixed in 3.1 by #4715.

I remember a module Net; autoload :OpenSSL, 'openssl'; end but that's been fixed:

Given the CI is green though, it seems that pattern is still allowed maybe? (both RubyGems & Bundler are tested in CRuby's CI AFAIK).

That Gem::Requirement autoload is very weird, I wonder if it actually ever worked, or it just worked due to rubygems/requirement being require-d explicitly always before the autoload is triggered.

Given the CI is green though, it seems that pattern is still allowed maybe?

I suspect it's a test order thing. Something else must require the necessary parts properly earlier in the test suite?

Either way, I think this should go though a deprecation period. The PR description should be updated to clearly state what behavior is being changed, and an associated issue on Redmine should be created to it can be discussed with more core members (I feel this needs Matz approval).

Re that Gem::Requirement autoload (https://github.com/rubygems/rubygems/blob/a9e305432955f56b847881187a7e01a97b6901e3/lib/rubygems/version.rb#L156), I think it never worked, here is a small repro showing it doesn't work:

# autoload.rb
module Foo
  class R
  end
end

# main.rb
module Foo
end

class Foo::Bar
  autoload :R, File.expand_path('autoload', __dir__)
  p R
end

$ ruby -v main.rb
ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
main.rb:6:in `<class:Bar>': uninitialized constant Foo::Bar::R (NameError)
Did you mean?  Foo::R
	from main.rb:4:in `<main>'

(Replacing p R by require File.expand_path('autoload', __dir__) works and defines Foo::R, in no case Foo::Bar::R is defined)

I kept the existing specs, and added new ones for 3.2. Please let me know if you prefer to delete them instead.

Good as you did, we need to keep testing 3.1 and before.

@fxn well done. This is looking really good to me.

Obviously we don't want to break things but if it was already non-functional/broken I would say there is probably some wiggle room to propose a way forward for 3.2 with this PR. It sounds like @byroot has some good opinions about that. Of course, there is a chance to break things, but there is no way to avoid it since this by nature is preventing a class of functionality that was "allowed" before.

I think we should definitely consider practical concerns if they show up, but short of real world failures, I'd support merging this for 3.2. Bearing in mind that people (1) aren't forced to upgrade to 3.2 and (2) can fix libraries to be compatible in a relatively straight forward way.

FYI

$ gem-codesearch '\s+autoload.+openssl'
/srv/gems/bitcoin-ruby-0.0.20/lib/bitcoin.rb:  autoload :OpenSSL_EC, "bitcoin/ffi/openssl"
/srv/gems/bundler-2.3.14/CHANGELOG.md:  - Avoid autoloading `openssl` to try help with jruby load service issues [#3809](https://github.com/rubygems/rubygems/pull/3809)
/srv/gems/dragonfly-openssl-1.0.3/lib/dragonfly-openssl.rb:    autoload :Config,  'dragonfly-openssl/config'
/srv/gems/dragonfly-openssl-1.0.3/lib/dragonfly-openssl.rb:    autoload :Encoder, 'dragonfly-openssl/encoder'
/srv/gems/elgamal-0.0.4/lib/elgamal.rb:#   # autoload :OpenSSL, 'rsa/openssl'
/srv/gems/klacointe-openpgp-0.0.1.4/lib/openpgp/engine.rb:    autoload :OpenSSL, 'openpgp/engine/openssl'
/srv/gems/lightning-onion-0.2.10/lib/lightning/onion/chacha20.rb:      autoload :OpenSSL, 'lightning/onion/chacha20/openssl'
/srv/gems/monacoin-ruby-0.1.3/lib/bitcoin.rb:  autoload :OpenSSL_EC, "bitcoin/ffi/openssl"
/srv/gems/openpgp-0.0.3/lib/openpgp/engine.rb:    autoload :OpenSSL, 'openpgp/engine/openssl'
/srv/gems/rb2exe-0.3.1/bin/traveling-ruby-2.2.2/l32/lib/ruby/2.2.0/net/http.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/rb2exe-0.3.1/bin/traveling-ruby-2.2.2/l64/lib/ruby/2.2.0/net/http.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/rb2exe-0.3.1/bin/traveling-ruby-2.2.2/osx/lib/ruby/2.2.0/net/http.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/rb2exe-0.3.1/bin/traveling-ruby-2.2.2/win/lib/ruby/2.2.0/net/http.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/rhodes-7.4.1/lib/extensions/net-http/net/http.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/rsa-0.1.4/lib/rsa.rb:  autoload :OpenSSL, 'rsa/openssl'
/srv/gems/rsa-g-1.0.5/lib/rsa-g.rb:  autoload :OpenSSL, 'rsa-g/openssl'
/srv/gems/ruby-compiler-0.1.1/vendor/ruby/lib/net/http.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/rubygems-update-3.3.14/bundler/CHANGELOG.md:  - Avoid autoloading `openssl` to try help with jruby load service issues [#3809](https://github.com/rubygems/rubygems/pull/3809)
/srv/gems/rubysl-net-http-2.0.4/lib/rubysl/net/http/http.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/scrypt-ruby-2.0.2/lib/scrypt-ruby.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/simpleblockingwebsocketclient-0.43/lib/ws.rb:  autoload :OpenSSL, 'openssl'
/srv/gems/tauplatform-1.0.3/lib/extensions/net-http/net/http.rb:  autoload :OpenSSL, 'openssl'

$ gem-codesearch '\s+autoload.+open-uri'
/srv/gems/argon-1.3.1/vendor/bundle/ruby/2.7.0/gems/thor-1.0.1/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"
/srv/gems/asciidoctor-2.0.17/lib/asciidoctor/abstract_node.rb:      # autoload open-uri
/srv/gems/asciidoctor-2.0.17/lib/asciidoctor/reader.rb:        # autoload open-uri
/srv/gems/asciidoctor-2.0.17/lib/asciidoctor.rb:  autoload :OpenURI, 'open-uri'
/srv/gems/bundler-2.3.14/lib/bundler/vendor/thor/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"
/srv/gems/colorcode_convert_rgb-0.1.2/vendor/bundle/ruby/2.5.0/gems/thor-1.0.1/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"
/srv/gems/date_n_time_picker_activeadmin-0.1.2/vendor/bundle/ruby/2.6.0/gems/thor-1.1.0/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"
/srv/gems/rubygems-update-3.3.14/bundler/lib/bundler/vendor/thor/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"
/srv/gems/symbolic_enum-1.1.5/vendor/bundle/ruby/2.7.0/gems/thor-1.0.1/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"
/srv/gems/tdiary-5.2.2/vendor/bundle/ruby/3.1.0/gems/thor-1.2.1/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"
/srv/gems/thor-1.2.1/lib/thor/runner.rb:  autoload :OpenURI, "open-uri"

@ko1 Thanks!

I have checked the ones related to OpenSSL. The majority of them would work with this patch, because they define a constant in the expected class/module. As per the gobal ones, the autoload in scrypt-ruby is set at the top-level, so it is good. Same for rhodes, top-level, and tauplatform, top-level.

The gem traveling-ruby ships "self-contained, portable Ruby binaries", shouldn't be an issue either.

The gem ruby-compiler has a copy of the http.rb that ships with Ruby. It is an AOT copiler for Ruby that has not been touched in 5 years and whose CI only checks Ruby 2.4. I'd be surprised if this gem is supposed to run in 3.2 without changes.

This leaves us with two gems that would need to be updated: simpleblockingwebsocketclient, that has the autoload within Net (here), and rubysl (It contains a copy of the http.rb that ships with Ruby).

I need to work now, will check OpenURI later.

These are just example, I don't think we can properly assert the extend of the breakage with just a grep.

I don't see any urge to make this breaking change, it removes a weird cruft yes, but it's not in the way of anything, nor does it break anyone's code. So simply deprecating with a warning message makes sense to me.

But again, this needs to be discussed, who wish to open the Redmine issue?

I have checked OpenURI: Thor has an incompatible autoload. The one in Asciidoctor is fine because it is defined at top-level. That is all, since most occurrences come from vendored Thors.

I'll open a ticket in Redmine.

👉 https://bugs.ruby-lang.org/issues/18813

I can remove the Gem::Requirement autoload 👍.

Regarding the one in Thor, I think it's also a mistake and can be removed too. It was added by rails/thor#663 to avoid an undefined constant error in a rescue clause, but I believe the correct solution was to actually remove the rescue clause. URI.open is not used here, so the rescue will never actually happen. If Thor wanted to support uris here it would need to detect whether an uri was passed and act accordingly like it does here: https://github.com/rubygems/rubygems/blob/96e5cff3df491c4d943186804c6d03b57fcb459b/bundler/lib/bundler/vendor/thor/lib/thor/actions.rb#L212-L225.

@fxn Let's rework this branch for autoload_relative.