Deprecate ActiveSupport::Deprecation singleton usage

[etiennebarrie]

Motivation / Background

Following #46049, and discussions around Deprecation objects like #44772 (comment):

To me we should get rid of the singleton and just push the deprecation instance we today use in the singleton to the sub-framework's railties.

All the frameworks railties now have their own deprecator, but we were just missing the part where the singleton was deprecated.

cc @jonathanhefner

Detail

For each of the helper classes/users of ActiveSupport::Deprecation#warn, we add a deprecation warning about using them without passing a deprecator (giving a more specific deprecation instead of a generic one about using ActiveSupport::Deprecation directly).

Then the instance delegator deprecates its usage, giving a deprecation specific to the case: some things should be rewritten to use Rails.application.deprecators like silence, some directly to a Deprecation instance like behavior, and others like warn probably mean the user needs to create its own instance.

Finally, deprecates the singleton instance by de-documenting the inclusion of Singleton, and adding a deprecation warning when ActiveSupport::Deprecation.instance is used.

Additional information

One thing I wasn't sure is if deprecated usage should still be documented or not even. I removed all the deprecated examples from the documentation.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[zzak]

I think we cannot remove documentation until after it is deprecated 🤔

[etiennebarrie]

I think we cannot remove documentation until after it is deprecated 🤔

I checked and for example when ActiveSupport::TimeWithZone#to_s(:db) was deprecated in Rails 7.0, the documentation was removed as well. https://api.rubyonrails.org/v7.0/classes/ActiveSupport/TimeWithZone.html#method-i-to_s

Or the Enumerable.sum without a default value deprecation: afa8335

There are definitely cases where we mentioned the old deprecated way. https://api.rubyonrails.org/v6.1/classes/ActiveModel/Errors.html#method-i-each

It could go either way, either we really want to document the current behavior, even if it's deprecated, or we only want to document the non-deprecated behavior, and people can always go the previous version of the documentation to get the documentation.

Either way, if we come up with a decision on that I could go and document that in new deprecation cycle documentation: https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#breaking-changes

[skipkayhil]

A few documentation suggestions and one implementation question, overall this looks good!

[etiennebarrie]

I've also noticed the documentation for collect_deprecations is incorrect and it was missing a test for collect_deprecations (#43774), so I've added a commit about that.

I realize this PR is too big, I'll create PRs for the things that can clearly be separated.

[skipkayhil]

🙇 one more thing

[matthewd]

I think silence, behavior, etc should be redirected to Rails.application.deprecators, not _instance... otherwise they're going to immediately stop affecting all Rails-generated deprecations that have already been pointed elsewhere.

These lines also need to go the other way around, at minimum, to have some hope of directing the meta-deprecation warning towards the intended deprecation config.

I just discovered that my application had been failing to identify deprecation warnings for a while because ActiveSupport::Deprecation.disallowed_warnings = :all was no longer having the intended effect.

[etiennebarrie]

Well, this PR only keeps the current main behavior and only adds the deprecation for calling ActiveSupport::Deprecation.disallowed_warnings=, but there is indeed a loss of behavior between ActiveSupport::Deprecation.disallowed_warnings= in 7.0 and here.

I do wonder how much we want to support this "undocumented behavior" though. For example https://api.rubyonrails.org/v7.0/classes/ActiveSupport/Deprecation/Disallowed.html mentions ActiveSupport::Deprecation.disallowed_behavior, but documents an instance writer for disallowed_warnings, which means it should have been used as ActiveSupport::Deprecation.new.disallowed_warnings=, or maybe ActiveSupport::Deprecation.instance.disallowed_warnings= (since Singleton was documented as an included module).

But the "correct" way was to set the configuration on Rails.application.config.active_support, and that has preserved the correct behavior in #46328.

There was a mention in the Configuring guide to using the instance delegators as an alternative so we can definitely argue it was documented behavior.

Alternatively, you can set ActiveSupport::Deprecation.disallowed_warnings

Anyway, I'll try to do the right thing and restore the 7.0 behavior of not only impacting the singleton instance but all deprecators when the instance delegators are used for these settings. The deprecation message correctly points how to get the previous behavior without a deprecation by setting on Rails.application.deprecators, so we should do the same.

[jonathanhefner]

I just discovered that my application had been failing to identify deprecation warnings for a while because ActiveSupport::Deprecation.disallowed_warnings = :all was no longer having the intended effect.

My apologies! 🙏 My (loose) reasoning was:

1. Previous deprecations that exist in 7.0 will (likely) be removed for 7.1.
2. New deprecations that have been added for 7.1 will require new silence blocks, disallowed_warnings values, etc for applications.
3. Since applications will already be updating silence blocks, disallowed_warnings values, etc to match the new 7.1 deprecations, it would be reasonable for them to switch from ActiveSupport::Deprecation.* to Rails.application.deprecators.* at the same time.

I did not take into account applications that are between 7.0 and 7.1 (i.e tracking main branch).

[matthewd]

Since applications will already be updating silence blocks, disallowed_warnings values, etc to match the new 7.1 deprecations

Even ignoring the running-on-main state, any upgrading-to-7.1 application is potentially currently configured in some way to direct/disallow deprecations. At absolute minimum, these meta-deprecations need to go into exactly that channel.

The whole point of our deprecation system is that we keep things functioning as they were before, while warning that the application should be adapted to the new way. Creating a new parallel system next to where the old one was, and then reasoning that people will start using it because it's there, does not achieve that.

New deprecations that have been added for 7.1 will require new silence blocks, disallowed_warnings values, etc for applications.

Specific disallows perhaps, but :all is pretty universal.

(I also think that we should still have a way to globally silence deprecations, or to globally disallow deprecations, without involving a railties dependency, but whatever.)

[etiennebarrie]

It's not pretty because Rails.application.deprecators may not exist if ActiveSupport is used on its own. So I have to used defined?(Rails.application.deprecators) and still delegate to the instance if it's not defined.

[jonathanhefner]

I just discovered that my application had been failing to identify deprecation warnings for a while because ActiveSupport::Deprecation.disallowed_warnings = :all was no longer having the intended effect.

My apologies! 🙏 My (loose) reasoning was:

1. Previous deprecations that exist in 7.0 will (likely) be removed for 7.1.
2. New deprecations that have been added for 7.1 will require new silence blocks, disallowed_warnings values, etc for applications.
3. Since applications will already be updating silence blocks, disallowed_warnings values, etc to match the new 7.1 deprecations, it would be reasonable for them to switch from ActiveSupport::Deprecation.* to Rails.application.deprecators.* at the same time.

I did not take into account applications that are between 7.0 and 7.1 (i.e tracking main branch).

[jonathanhefner]

I think what Matthew was saying in #47354 (comment) is that the warning needs to come after the method call. That way if e.g. behavior= is being called to configure how deprecations are reported, the same configuration will apply to the warning as well.

Suggested change

	ActiveSupport.deprecator.warn("Calling #{method_name} on ActiveSupport::Deprecation is deprecated and will be removed from Rails (use #{use_instead} instead)")
	#{target}.#{method_name}(#{args})
	#{target}.#{method_name}(#{args})
	ensure
	ActiveSupport.deprecator.warn("Calling #{method_name} on ActiveSupport::Deprecation is deprecated and will be removed from Rails (use #{use_instead} instead)")

(Using ensure in case the method call has a block that raises an error, like silence { raise }.)

Also, we want any configuration to apply to warnings even when Rails.application.deprecators isn't defined. Meaning we should use ActiveSupport::Deprecation._instance to warn, since that is guaranteed to become configured in any case.

We could replace ActiveSupport.deprecator.warn with ActiveSupport::Deprecation._instance.warn here. But another possibility would be to swap the definitions of ActiveSupport.deprecator and Rails.deprecator. i.e. Make ActiveSupport.deprecator return ActiveSupport::Deprecation._instance, and Rails.deprecator return a new deprecator.

[etiennebarrie]

I'm not sure of all the consequences of swapping the definitions, but it does make sense when looking at activesupport both in isolation and in the context of Rails.
In isolation, it would be lacking if changing ActiveSupport::Deprecation.behavior would no longer impact deprecations emitted by activesupport itself.
In the context of Rails, it would be an issue if changing Rails.application.deprecators[:activesupport].silenced would impact all activesupport deprecations except this one.

[jonathanhefner]

Similarly, to cover e.g. ActiveSupport::Deprecation.instance.behavior = ...:

Suggested change

	ActiveSupport.deprecator.warn("ActiveSupport::Deprecation.instance is deprecated (use your own Deprecation object)")
	super
	super
	ensure
	ActiveSupport::Deprecation._instance.warn("ActiveSupport::Deprecation.instance is deprecated (use your own Deprecation object)")

Or, if we make ActiveSupport.deprecator == ActiveSupport::Deprecation._instance:

Suggested change

	ActiveSupport.deprecator.warn("ActiveSupport::Deprecation.instance is deprecated (use your own Deprecation object)")
	super
	super
	ensure
	ActiveSupport.deprecator.warn("ActiveSupport::Deprecation.instance is deprecated (use your own Deprecation object)")

[etiennebarrie]

This one is not necessary/possible, as calling ActiveSupport::Deprecation.instance will first emit a deprecation, before we ever have a chance to set the behavior on it, whether we emit the deprecation before calling super or not.

[jonathanhefner]

You're right — swapping the order here is nonsensical. 🤦‍♂️ 🤦‍♂️

Using ActiveSupport::Deprecation._instance as the underlying deprecator will benefit subsequent calls though. e.g.:

ActiveSupport::Deprecation.instance.behavior = ...
ActiveSupport::Deprecation.instance.disallowed_behavior = ...

(Which is now covered since ActiveSupport.deprecator is ActiveSupport::Deprecation._instance.)

[jonathanhefner]

We will probably remove ActiveSupport::Deprecation._instance whenever we remove ActiveSupport::Deprecation.instance. So I think eventually this will need to be:

Suggested change

	ActiveSupport::Deprecation._instance.deprecate_methods(self, *method_names, **options, deprecator: deprecator)
	ActiveSupport.deprecator.deprecate_methods(self, *method_names, **options, deprecator: deprecator)

[etiennebarrie]

That's a great idea, since we're not using the deprecator on which we call the method, but the one passed in, we can just use any instance, and the only one we're really sure exists is ActiveSupport.deprecator.

I think there's another discussion to have about this duck-typed API (only implementing deprecation_warning), it's not very Rails-y, and does not offer much value since you can just pass a real object, I'm not sure why this both added a way to instantiate deprecators and the duck-typed API: 71993c6

[jonathanhefner]

I think there's another discussion to have about this duck-typed API (only implementing deprecation_warning), it's not very Rails-y, and does not offer much value since you can just pass a real object, I'm not sure why this both added a way to instantiate deprecators and the duck-typed API: 71993c6

I agree, though I might be missing something. It seems like the intent was to make the API more "lightweight" so that the user doesn't have to be aware of the ActiveSupport::Deprecation class. But, like you said, the value of that is questionable. Also, if a lightweight variant is desirable, a callable (e.g. a Proc) seems like a better choice.