Repository navigation
Name a class written from the top level as Ruby does, with --experimental-parser - #82
Merged
Merged
Conversation
…ntal-parser The experimental parser joined a class or module's name to the namespaces around it with `::`, so `class ::Taco` was defined as `::::Taco`, and `class ::Burrito` inside `module Outer` as `::Outer::::Burrito`. The constants assigned in them got the same prefix, as `::::Taco::FILLING`. No reference resolves to a name with `::::`, so references to these constants were missed. The experimental resolver built its guesses the same way, so `Shell` written inside `class ::Taco` was looked up as `::::Taco::Shell` and then `::Shell`, and a `::Taco::Shell` that another pack defines was missed too. `nest_constant_name` starts the name again from the top level at a namespace that begins with `::`, as Ruby does. The experimental parser names its definitions with it, and the experimental resolver makes its guesses with it, so inside `module Outer; class ::Foo`, `Bar` is `::Foo::Bar`, then `::Outer::Bar`, then `::Bar`, the order of Ruby's `Module.nesting`. The packwerk parser and the Zeitwerk resolver keep `combine_namespace_with_constant_name` and `get_definition_from`, which match packwerk. packwerk names `class ::Taco` `::::Taco` among a file's local definitions too, and its constant_resolver looks `Shell` up as `::Taco::Shell` against names that have no leading `::`, so it misses the same references. The CHANGELOG says so. The cache key includes pks's version, so a release doesn't serve the old names from the cache.
The test cases and `nest_constant_name` already say what they did.
The first commit says that no reference resolves to a name with `::::`. One written inside another `class ::Taco`, as the definition is, did. The resolver built its guess the same way, so `FILLING` there resolved to `::::Taco::FILLING`. The rest were missed. That reference is now named `::Taco::FILLING`, so a violation that a package_todo.yml records under the `::::` name is reported again under the new name, and the old entry as stale, until `pks update` is run. The CHANGELOG now says so.
No test caught the experimental parser naming `module ::Foo` the old way, or `nest_constant_name` starting again at the first namespace written from the top level in place of the last, and the integration test didn't reach the resolver. `definitions_from_the_top_level` now has a `module ::Foo`, and nestings with a namespace from the top level in the middle, as `module A; class ::B; module C`. A new integration test looks up `Shell` inside `module Outer; class ::Taco`, and finds `::Taco::Shell` in another pack. `nest_constant_name` builds the name in one String with a loop, in place of a fold that made a String for each part. Its doc comment says that a constant assigned from the top level starts again too, and the resolver's comment on its lookup order covers a namespace from the top level.
With `class ::String` inside `module CoreExt` now named `::String`, `expose-monkey-patches` lists it, and a reference to `String` from another pack is a reference to the pack that reopens it, as it is for a `class String` at the top level of a file. The CHANGELOG says so, and that `::BAR = 1` inside `module Outer` was `::Outer::::BAR`. Without `--experimental-parser`, a constant written inside `class ::Foo` isn't only missed: packwerk, and pks with it, can take `Bar` there for a top-level `::Bar`. The CHANGELOG says that in place of "misses".
6 tasks done
dduugg
added a commit
that referenced
this pull request
Oct 14, 2026
bc0ad33 said that only a definition written from the top level, at the top level of a file, has a name that starts with `::::`, so nothing else changes. That isn't so. The constants defined in such a class, as `::::Taco::Shell` in `class ::Taco; class Shell`, start with it too, as does the `::::Taco` pks takes from `class ::Taco::Shell`, and a `Taco` written inside `class ::Taco` was dropped the same way. pks now counts each of these as a reference, as packwerk 3.3.1 does, and the CHANGELOG says so. Tests cover a `Taco` inside `class ::Taco`, `::Taco = 1` at the top level of a file, and `class ::Burrito` in `module Outer` and `class ::Cee` in `module Aa; module Bb`, from the file to the references pks lists. e9a6092 said that for a reference written `Object::Bar`, the resolver looks `Object` up as Ruby does. Inside `module CoreExt` it tries `::CoreExt::Object::Bar` first, as Ruby would, but where `CoreExt` defines an `Object` with no `Bar`, Ruby raises a NameError, and pks goes on to `::Bar`, as it does for `Foo::Bar` when `CoreExt::Foo` has no `Bar`. New tests name `module Object::Bar`, the constants in `class Object::Bar`, `::Object::Object::BAR` and `ObjectSpace::Bar`, and find constants defined through `Object` from another pack. Comments now say that only the experimental parser and resolver drop a leading `::Object`, and that the empty names come off the end as in constant_resolver.
dduugg
added a commit
that referenced
this pull request
Oct 14, 2026
…ough `Object`, and match packwerk on `::` names and `class_eval` (#83) * Take no parent for `Outer::`, as packwerk does packwerk names `class ::Burrito` inside `module Outer` `::Outer::::Burrito`, and so does pks. When no file defines a constant, both look for it in the file of its parent, which for `Outer::::Burrito` is `Outer::`. packwerk takes the parent with Ruby's `String#split`, which drops the empty name at the end, so `Outer::` has no parent, and packwerk finds no constant. Rust's `split` keeps it, so pks went on to `Outer`, and reported `::Outer::::Burrito` against the pack that defines `Outer`. In `module Aa; module Bb; class ::Cee`, packwerk goes from `Aa::Bb::` to `Aa`, where pks stopped at `Aa::Bb`. The Zeitwerk resolver now drops the empty names at the end too. * Count a constant after `class ::Taco` as packwerk does packwerk names `class ::Taco` at the top level of a file `::::Taco` among the file's own definitions, so a `Taco` or `::Taco` written later in the file, which it looks for as `::Taco`, isn't one of them, and is a reference. pks looked for `::` and the name, `::::Taco`, as well, so it took them for the file's own and dropped them. The packwerk parser now looks for the name alone. Only a definition written from the top level, at the top level of a file, has a name that starts with `::::`, so nothing else changes. * Look up an association from its model's name, with --experimental-parser Rails looks up the class of `has_many :tacos` from the name of the model that declares it: in `Outer::Foo`, as `Outer::Foo::Taco`, `Outer::Taco`, then `Taco`. The experimental parser gave the reference the namespaces it's written in, so in `class Outer::Foo` pks looked for `::Outer::Foo::Taco`, then `::Taco`, and in `module Outer; class ::Foo` for `::Foo::Taco`, `::Outer::Taco`, then `::Taco`. The reference now has the namespaces of the model's name, `["Outer", "Foo"]` and `["Foo"]`, so the resolver looks where Rails does. Without `--experimental-parser`, pks looks it up as packwerk does, as before. * Name a constant written through `Object` as Ruby does, with --experimental-parser The constants of `Object` are the top level's, so `class Object::Bar` inside `module CoreExt` defines `::Bar`, as does `class Bar` inside a `class Object` at the top level of a file. The experimental parser named them `::CoreExt::Object::Bar` and `::Object::Bar`. `nest_constant_name` now drops a leading `::Object` from a name, and the experimental parser takes a class, module or constant defined as `Object::Bar` for `::Bar`. So both are named `::Bar`, and the experimental resolver finds `::Bar` for a reference written `::Object::Bar`. For a reference written `Object::Bar`, the resolver looks `Object` up as Ruby does, so inside `module CoreExt` it tries `::CoreExt::Object::Bar` before `::Bar`. The parser can't look it up for a definition, so it takes it that no namespace defines an `Object` of its own. A `class Object` inside a namespace is still the namespace's own, as Ruby makes it. * Say what the follow-ups to #82 change in the CHANGELOG The association lookup and the names written through `Object` change what `--experimental-parser` finds. Without it, pks now matches packwerk around a name written from the top level, so `pks check` may report a violation it didn't, or no longer report one, and leave a todo stale. * Try `::Taco` first for `has_many :tacos` in a model named `Taco` ActiveRecord's `_klass` (reflection.rb) looks up the class of `belongs_to :company` in a model whose name ends in `Company` as `::Company` first, and from the model's name only if that isn't defined. a81a247 gave the reference the namespaces of the model's name, so with `--experimental-parser`, in `class Payroll::Company`, pks looked for `::Payroll::Company::Company`, then `::Payroll::Company`, and found the model itself, where it had found `::Company`. So it missed a violation it used to report. In `module Payroll; class Company`, it found the model itself before a81a247 too. When the association's class has the model's own last name, the reference is now to `::Company`, so pks finds what Rails does in both. Where no `::Company` is defined, Rails goes on to find `Payroll::Company::Company` or the model itself, and pks finds nothing, so it misses a reference only where a `Payroll::Company::Company` is defined. A test also covers a model in `module Outer; class Foo` and in `class Object::Foo`. In a module, as a concern, pks can't know the model that includes it, and takes the module's name, and `model_namespaces` now says so. * Correct what the follow-ups to #82 say, and test what they change bc0ad33 said that only a definition written from the top level, at the top level of a file, has a name that starts with `::::`, so nothing else changes. That isn't so. The constants defined in such a class, as `::::Taco::Shell` in `class ::Taco; class Shell`, start with it too, as does the `::::Taco` pks takes from `class ::Taco::Shell`, and a `Taco` written inside `class ::Taco` was dropped the same way. pks now counts each of these as a reference, as packwerk 3.3.1 does, and the CHANGELOG says so. Tests cover a `Taco` inside `class ::Taco`, `::Taco = 1` at the top level of a file, and `class ::Burrito` in `module Outer` and `class ::Cee` in `module Aa; module Bb`, from the file to the references pks lists. e9a6092 said that for a reference written `Object::Bar`, the resolver looks `Object` up as Ruby does. Inside `module CoreExt` it tries `::CoreExt::Object::Bar` first, as Ruby would, but where `CoreExt` defines an `Object` with no `Bar`, Ruby raises a NameError, and pks goes on to `::Bar`, as it does for `Foo::Bar` when `CoreExt::Foo` has no `Bar`. New tests name `module Object::Bar`, the constants in `class Object::Bar`, `::Object::Object::BAR` and `ObjectSpace::Bar`, and find constants defined through `Object` from another pack. Comments now say that only the experimental parser and resolver drop a leading `::Object`, and that the empty names come off the end as in constant_resolver. * Look up associations and `Object` from the whole app, with --experimental-parser With `--experimental-parser`, two things that pks looked up from one file depend on the rest of the app, so they now wait until every file is read. Rails looks up the class of an association in a module, as a concern's `included` block, from the name of each model that includes the module. a81a247 and 15b614c took the module's name for the model's. pks now records the `include` and `prepend` calls in each class and module, and looks up the class from the name of each class that includes the module, directly or through other modules, so one association can reference a class for each. Where no class includes the module, the module's name stands in, as before. When the class has the model's own name and no `::Taco` is defined, Rails goes on to look it up from the model's name, as `Outer::Taco::Taco`; 15b614c looked for `::Taco` only. From the model's name, Rails takes only a class of the very name it looks for (`compute_type` in inheritance.rb), and pks now does too. In a controller, inherited_resources looks up the class of `belongs_to` from the controller's namespaces, and one given as `class_name:` from the top level, so pks no longer looks in the controller itself, and an ActiveRecord model whose name ends in `Controller` is looked up as any other. `class Object::Charge` in `module Payments` defines `::Payments::Object::Charge` where the app has a `Payments::Object`, even one that defines nothing. e9a6092 took it for `::Charge` whatever the app defines. Where some namespace has its own `Object`, pks now reads a file that defines a constant through `Object` in a namespace again, knowing which namespaces do, and names it as Ruby does. Since `check-contents` reads its file from stdin, which can be read only once, pks keeps what it read there. Separately, Ruby finds a constant of `Kernel` or `BasicObject`, which `Object` includes and inherits from, from anywhere, after the top level's own, and the experimental resolver now looks there last. * Name a class defined in `class_eval` or `Class.new` as packwerk does packwerk names the reference that a class or module definition makes to itself from every class, module, `class_eval` block on a constant, and constant assigned `Class.new` or `Module.new` around it (`parent_module_name` in node_helpers.rb). pks named it from the classes and modules alone, so in `module Aa; Bb.class_eval do; class ::Cee` it left out `Bb`. c081104 made the Zeitwerk resolver drop the empty name at the end of a parent, as packwerk does, and that turned the gap into a regression there: pks found no reference where packwerk finds one to `Aa`, and in `module Aa; module Bb; Cc.class_eval do; class ::Dee` it found one to `Aa` where packwerk's is to `Aa::Bb`. The packwerk parser now keeps a second stack of names for this, with the receiver of `Foo.class_eval do` and the constant of `Foo = Class.new do`, and names the reference from it. A block that takes `_1` or `it`, which packwerk's parser makes a `numblock` or `itblock`, adds nothing, as in packwerk. Where packwerk can't name the class, in a block passed with `&.` or to `super`, pks no longer counts a reference either. The file's own definitions, and the namespaces each reference is looked up in, still come from the classes and modules alone, as in packwerk. On a copy of `simple_app` with 66 such files, list-references and packwerk 3.3.1 now differ on one line: a reference packwerk counts to the scope `Kk` of `Kk::Ll = 1`, which pks has never counted.
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
1 participant
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
With
--experimental-parser, pks named a class or module written from the top level as if it nested in the namespaces around it.class ::Tacowas defined as::::Taco,class ::Burritoinsidemodule Outeras::Outer::::Burrito, and a constant assigned in one, as::::Taco::FILLING. The resolver built its guesses the same way, so a reference to one of these was found only from inside anotherclass ::Tacowritten the same way, and then under the::::name. The rest were missed. So wasShellwritten insideclass ::Taco, looked up as::::Taco::Shelland then::Shell, when another pack defines::Taco::Shellasclass Taco::Shell.nest_constant_namestarts the name again from the top level at a namespace that begins with::, as Ruby does.module Outer; class ::Foo,Baris now looked up as::Foo::Bar, then::Outer::Bar, then::Bar, the order of Ruby'sModule.nesting.::::name now has the plain one, as::Taco::FILLING. A violation that apackage_todo.ymlrecords under the::::name is reported again under the new name, and the old entry as stale, untilpks updateis run. The CHANGELOG says so.--experimental-parser, nothing changes. packwerk namesclass ::Taco::::Tacoamong a file's local definitions too, and its constant_resolver looksShellup as::Taco::Shellagainst names that have no leading::, so it misses the same references, or takesShellfor a top-level::Shell. The packwerk parser and the Zeitwerk resolver keep that lookup, and the CHANGELOG says so.class ::Stringinsidemodule CoreExt, is now::String, so with--experimental-parser,expose-monkey-patcheslists it, and a reference toStringfrom another pack is a reference to the pack that reopens it, as for aclass Stringat the top level of a file. The CHANGELOG says this too.The cache key includes pks's version, so a release doesn't serve the old names from the cache.
Test plan
cargo test(378 passed, and 379 on a merge withmain, which adds one),cargo fmt --check, andcargo clippy --all-targets --all-features -- -Dwarnings.definitions_from_the_top_levelandresolve_in_a_namespace_from_the_top_levelcover the parser's names and the resolver's lookup, andtest_list_references_to_a_class_defined_from_the_top_levellists a reference toclass ::Tacoinsidemodule Outerin another pack. All three fail whennest_constant_namejoins the old way.definitions_from_the_top_levelalso has amodule ::Fooand nestings likemodule A; class ::B; module C, andtest_list_references_inside_a_class_defined_from_the_top_levellooksShellup insidemodule Outer; class ::Taco. They fail whenvisit_module_nodenames a module the old way, when only the resolver joins the old way, and whennest_constant_namestarts again at the first::namespace in place of the last. No test caught those before.simple_appwithclass ::Tacoandmodule Outer; class ::Burrito,-e list-definitionsgives::Taco,::Taco::FILLINGand::Burrito, and-e list-referencesnow lists::Taco,::Taco::FILLING,::Burrito, and::Taco::Shellfrom insideclass ::Taco. The default parser's output is unchanged.class ::Taco; FILLING = 1; endinpacks/barandFILLINGwritten inside aclass ::Tacoinpacks/foo, the binary before this change lists::::Taco::FILLING, and this one::Taco::FILLING. Afterpks -e updatewith the old binary,pks -e checkwith this one reports the two violations under the new name and the old todo entry as stale.