Don't infer a constant from a polymorphic association - #68
Merged
Merged
Conversation
dduugg
force-pushed
the
fix-polymorphic-association-references
branch
from
October 2, 2026 00:58
2b0ceaa to
3907509
Compare
get_reference_from_active_record_association inferred a class from the association's name unless `class_name:` was given, so `belongs_to :event, polymorphic: true` was read as a reference to `Event`. When another pack defined `Event`, that produced privacy and dependency violations for code that never names it (#19). Rails reads a polymorphic association's class from its `_type` column at runtime, so the declaration references no constant. The function now returns no reference when the options include a literal `polymorphic: true`, even when `class_name:` is also given. Rails 8.1 rejects `class_name:` there (rails/rails#55089). Earlier versions accept it and use it only to load the class for `counter_cache:`, which pks doesn't count as a reference. The code comment and CHANGELOG say so. InheritedResources' `belongs_to` constantizes the class for each symbol, polymorphic or not, so the reference is kept where that method may be running: in a class or module whose name ends in `Controller`, or outside any class or module. The second case covers ActiveAdmin's `register` and `controller do` blocks, which pass their options through to it. An Active Record model declares its associations inside its class, so neither case drops a model's reference. `polymorphic: false`, `nil`, or a non-literal value still infer a class as before, and constants inside a scope lambda are still collected by the visitor. Both parsers share this function, so both are fixed, and it applies to custom_associations too. `with_options polymorphic: true` blocks are not handled. packwerk reports these references too, so this is a new difference from it, noted in the README. A package_todo.yml entry recorded for one of these associations is now reported as stale until `pks update` is run, and check-unused-dependencies may report a dependency that only such an association used. The CHANGELOG entry covers both. Fixes #19
dduugg
force-pushed
the
fix-polymorphic-association-references
branch
from
October 2, 2026 01:29
3907509 to
c04446c
Compare
7 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #19.
pks inferred a class from an association's name unless
class_name:was given, sobelongs_to :event, polymorphic: truecounted as a reference toEvent. When another pack definedEvent, that showed up as a privacy or dependency violation for code that never names it.Rails reads a polymorphic association's class from a type column (
event_typeby default) at runtime, so the declaration references no constant.get_reference_from_active_record_associationnow returns no reference when the options include a literalpolymorphic: true. Both parsers go through that function, so both are fixed.Where to look
class_name:is dropped too whenpolymorphic: trueis set. pks follows Rails 8.1, which rejects that option there (:class_name should be invalid in polymorphic belongs_to rails/rails#55089). Earlier versions accept it and use it only to load the class forcounter_cache:. pks doesn't count that load as a reference. The code comment and CHANGELOG say so. Withcounter_cache:, 8.1 still loads the class named after the association, and pks ignores that too.belongs_toconstantizes the class for each symbol, polymorphic or not (class_methods.rbin 2.1.0). So the reference is kept where that method may be running: in a class or module whose name ends inController, or outside any class or module. The second case covers ActiveAdmin'sregisterandcontroller doblocks, which pass their options through to it. An Active Record model declares its associations inside its class, so neither case keeps a model's reference.truecounts.polymorphic: false,nil, or a variable still infer the class as before.custom_associationssuch ascache_belongs_toget the same treatment, as they already do forclass_name:.packwerk
packwerk's
AssociationInspectorhas the same bug, which is why the issue saw the same result from both tools. So this is a deliberate difference from packwerk, and it's listed under "Behavioral differences" in the README. That section's intro now says some differences are deliberate.A
package_todo.ymlentry recorded for one of these associations is no longer found, sopks checkreports it as stale untilpks updateruns.check-unused-dependenciesmay also report a dependency that only such an association used. The CHANGELOG entry covers both.Not covered
with_options polymorphic: true do belongs_to :event endstill reportsEvent, since the option isn't on the call itself.belongs_to :event, { polymorphic: true }) or**optsstill reportsEvent, the same limitationclass_name:already has.belongs_towithpolymorphic: truein a concern, an abstract base controller not named*Controller, or aClass.new(InheritedResources::Base)block no longer reports the class. A model whose name ends inControllerstill reports it, as on main.belongs_to :post, :event, polymorphic: truein a controller reports onlyPost, as on main.Test plan
polymorphic: true,class_name:withpolymorphic: true, a scope lambda withpolymorphic: true,polymorphic: false, a controller, a plain class nested inside a controller, and an ActiveAdminregisterblock.polymorphic: true.test_check_ignores_polymorphic_associationrunspks checkon a fixture model withbelongs_to :event, polymorphic: true, whereEventis private to another pack. On main it reportsPrivacy violation: `::Event` is private to `packs/baz`. It also asserts an empty stderr, so a renamed fixture can't make it pass by matching no file.parse_utils.rs, the plain,class_name:, scope, and experimental unit tests fail, as does the integration test. Treating only controllers as exceptions fails the ActiveAdmin test, and checking the outermost namespace instead of the innermost fails the nested-class test.polymorphic: nil, a variable, and a controller (including a namespaced one) keep it.checkreports it stale,updateremoves it, andcheckis then clean.cargo test(323 passed), fmt, and clippy with-Dwarnings.