Skip ActiveSupport deprecation proxies when gathering DSL constants - #2701
Conversation
KaanOzkan
left a comment
KaanOzkan
left a comment
There was a problem hiding this comment.
Thank you for the fix. Minor suggestions.
| ActiveSupport.deprecator.behavior = previous_behavior | ||
| end | ||
|
|
||
| refute_includes(constants, "ActiveSupport::Concurrency::LoadInterlockAwareMonitor") |
There was a problem hiding this comment.
I didn't dive into it but agent suggested this diff instead and claimed that current refute_includes would pass even without this change because the name is delegated and becomes Monitor, it might be better to do the diff below. Also can you ensure the final test fails without your change?
- constants = gathered_constants
+ proxy = ActiveSupport::Concurrency.const_get(:LoadInterlockAwareMonitor, false)
+ gathered_constants
+ all_modules = Tapioca::Dsl::Compilers::ActiveSupportConcern.send(:all_modules)
...
- refute_includes(constants, "ActiveSupport::Concurrency::LoadInterlockAwareMonitor")
+ refute(all_modules.any? { |mod| Tapioca::Runtime::Reflection.are_equal?(mod, proxy) })
assert_empty(warnings.grep(/LoadInterlockAwareMonitor/))
There was a problem hiding this comment.
Updated. The spec now loads the proxy with inherit false and checks identity against all_modules so the assertion fails if the skip is missing.
| else | ||
| ObjectSpace.each_object(Module).to_a | ||
| end.freeze #: Enumerable[Module[top]]? | ||
| end.reject { |mod| deprecated_constant_proxy?(mod) }.freeze #: Enumerable[Module[top]]? |
There was a problem hiding this comment.
I think it's better to abide by the requested constants even if they result in warnings. Can you move this reject above?
ObjectSpace.each_object(Module).reject { |mod| deprecated_constant_proxy?(mod) }
There was a problem hiding this comment.
Moved. Requested constants stay in the set even if they warn. ObjectSpace discovery is the only path that skips the proxies.
Requested constants stay in the set even if they warn. The spec now checks the proxy by identity so it fails without the skip.
|
Moved the deprecation proxy skip onto the ObjectSpace path only, so requested constants still make it through. The spec now looks up the proxy by identity instead of the delegated name. |
|
|
||
| refute(all_modules.any? { |mod| Tapioca::Runtime::Reflection.are_equal?(mod, proxy) }) | ||
| assert_empty(warnings.grep(/FakeDeprecatedConstant/)) | ||
| end |
There was a problem hiding this comment.
I updated this test to register a fake constant as deprecated since these things might change in future Rails versions.
e5da488 to
9156fa1
Compare
|
Linters failed on RSpec/RemoveConst in the deprecation proxy spec. Added the same rubocop disable the other specs use for that cleanup. |
The nested Minitest `describe` block uses a dynamic class that is not registered in the Ruby constant tree. Assigning the proxy through `self.class` leaves `Module#name` unset, so discovery skips it before invoking warning-producing methods. Assign the proxy through `ActiveSupportConcernSpec` so the test covers the warning path. `DslSpec` runs each example in an isolated process, so the temporary constant does not need manual cleanup.
2 participants
Fixes #2463
Rails 8.1 replaced LoadInterlockAwareMonitor with a DeprecatedConstantProxy. DSL compilers walk every loaded module and call methods such as singleton_class on each one. Those calls go through method_missing on the proxy and print deprecation warnings even though Tapioca is only enumerating ObjectSpace.
This change drops DeprecatedConstantProxy modules from all_modules so every DSL compiler skips them. Kernel.class is used to recognize the proxy without triggering the warning.
The ActiveSupportConcern gather_constants spec covers the Rails 8.1 LoadInterlockAwareMonitor proxy.