Add Rubocop::Cops::Rails::ZeitwerkFriendlyConstant

[shioyama]

This PR adds a cop named Rails/ZeitwerkFriendlyConstant.

The cop ensures that all constants defined in files are independently autoloadable by Zeitwerk. The inline comments in the cop explain this, but in a nutshell, this avoids situation like:

# /some/directory/foo/bar.rb
module Foo
  module Bar
  end

  module Baz
  end
end

Foo::Baz # blows up with NameError

Foo::Bar # loads
Foo::Baz # now loads, since Foo::Baz was indirectly loaded while loading Foo::Bar

The way it checks this is by crawling the AST and tracking the nesting of constants relative to the file path nesting. The cop makes no assumptions about autoload paths, it simply looks for any possible inconsistencies between the constants defined and the filename nesting.

We use this at Shopify and it has helped weeding out issues that can arise when load paths change and suddenly previously "loadable" constants suddenly break, as in the example above.

To use the cop, you'll need to ensure you exclude file paths that are not autoloaded. There is also an Acronyms config to define acronyms, roughly matching what you might define with ActiveSupport::Inflector.inflections.

Here's our config for reference (components has all our in-repo engines):

Shopify/ZeitwerkFriendlyConstant:
  Enabled: true
  Acronyms:
    - GraphQL
    - GCS
    - GeoIP
    - B2B
    - KYM
    - IBAN
    - TOS
    - VM
    - SQLite
  Exclude:
    - 'bin/**/*'
    - 'test/**/*'
    - 'lib/**/*'
    - 'gems/**/*'
    - 'config/**/*'
    - '**/config/initializers/*'
    - 'components/*/config/**/*'
    - 'db/**/*'
    - 'components/**/lib/**/*'
    - 'components/**/test/**/*'

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[Earlopain]

I've tried this one out and its flagging a bunch of things I'm not sure are actually problems.

• Monkeypatches in initializers (config/initializers/core_extensions.rb). Will not be autoloaded in dev, I know

  class String
    include Extensions::String
  end

• The Application class in config/application.rb (since it's nested in a module)
• Migrations (they contain leading numbers in their filename)
• Miscelanious scripts in a path that is not managed by the autoloader (db/fixes in my case). That seems rather specific though and would probably be need to be excluded in the config on my side)
• These lines in my test_helper:

  BCrypt::Engine.send(:remove_const, :DEFAULT_COST)
  BCrypt::Engine::DEFAULT_COST = BCrypt::Engine::MIN_COST

• Monkeypatched ActiveSupport::TestCase in my test helper

It however also exposed a few cases where test classes contains typos/copy-paste issues, or where a class is nested wrongly which I think is the main purpose of this cop.

[shioyama]

@Earlopain

Oh yeah, neglected to mention we have this in our config:

  Exclude:
    - 'bin/**/*'
    - 'test/**/*'
    - 'lib/**/*'
    - 'gems/**/*'
    - 'config/**/*'
    - '**/config/initializers/*'
    - 'components/*/config/**/*'
    - 'db/**/*'
    - 'components/**/lib/**/*'
    - 'components/**/test/**/*'

components is where we have engine code. Can you try this and see if it covers the false positives for you?

[shioyama]

where a class is nested wrongly which I think is the main purpose of this cop.

Yes, this indeed is the main purpose of the cop. I created it in the context of a project where we were running each of our tests from our CI suite in a local (lazy-loaded) environment, which exposed issues related to load order that were not apparent in an eager-loaded environment.

But since then, it has also come in handy to avoid issues that crop up in cases where you have:

# foo/bar.rb
module Foo
  class Bar; end

  class SomeOtherClass; end
end

# baz.rb
module Baz
  # some code that references `Foo::Bar`
  # some code that references `Foo::SomeOtherClass`
end

In this context, if the line that references Foo::Bar is removed, the reference to Foo::SomeOtherClass may break in a lazy-loaded environment if Foo::Bar has not already been loaded. This can also happen while eager-loading code. In general violations to this cop increase the likelihood of autoload breakage which can be pretty infuriating and sometimes hard to track down.

[shioyama]

cc @fxn You might find this interesting 😄

[Earlopain]

Oh yeah, neglected to mention we have this in our config:

With these, yes. It looks much better. It leaves me with one legit offense and one that's somewhat my fault because of unconventional repo structure. You will need to add something like this to the default config since you can't expect users do to this themselves, especially for a cop that would be enabled by default (or rather be pending).

You're also excluding the test folder but at least for me that gave me some nice finds and no false positives. But I guess that depends on how you write/name them. They aren't part of autoloading (I think), so not that important.

Considering the many ways you can structure your code (rspec instead of minitest is one I can immediatly think of), in my mind using an explicit includelist of app/** instead of excluding everything else that might not fit would be a better idea. Many cops here limit themselves to a subfolder of app/ like controllers or models for similar reasons I presume.

I think most if not all autoloading for a traditional rails app is going on in the app folder exclusivly (and things in lib sans a few exceptions with Rails 7.1 defaults). This excludes what might be going on in engines, I'm not knowledgable on them.

For the acronyms/inflections: If you use them heavily you will also need to modify the cops config the same way. I believe it would makes sense to at least ignore casing differences by default. So api_key.rb with APIKey should be fine. This to me seems like the most common cases why someone would use custom inflections. This would make these cases pass by default while making the cop a bit less useful for these cases. I also noticed that there are no docs for the "Acronyms" options.

Anyways, I'm not a maintainer here, etc. etc. These are just my personal thoughts. You might be better off to wait for someone with actual authority unless some of my points make perfect sense to you.

[fxn]

❤️

It is going to be useful to have a cop preventing this kind of situations.

I wonder about the inherent limitations the cop may have, and whether they could break user expectations. For example, we know inflection rules can go beyond acronyms, there might be collapsed directories, custom namespaces, and ignored files or subtrees.

Given a certain file path, the loader has API to tell you which is its expected constant path. That would be robust and simple, but I guess RuboCop can only work with source code and should not boot the app.

[shioyama]

@Earlopain

They aren't part of autoloading (I think), so not that important.

Yep, we left those out since technically they are not autoloaded, so Zeitwerk is not involved. But that was just our preference.

in my mind using an explicit includelist of app/** instead of excluding everything else that might not fit would be a better idea. Many cops here limit themselves to a subfolder of app/ like controllers or models for similar reasons I presume.

Is it possible to set default include/exclude list for a cop? I didn't see anything like that, and honestly other than this one cop, I haven't worked that deeply with rubocop in general.

I believe it would makes sense to at least ignore casing differences by default.

That would change the logic of the cop somewhat, and in our case, on a huge codebase, just setting a list of acronyms (mostly copied from inflections.rb) worked fine. I'd prefer to be explicit about exceptions to the general rule personally.

@fxn

I wonder about the inherent limitations the cop may have, and whether they could break user expectations. For example, we know inflection rules can go beyond acronyms, there might be collapsed directories, custom namespaces, and ignored files or subtrees.

Yep, and all these things are reasons we haven't released this cop beyond our use in one repo thus far. But given that all those use cases are probably a small minority (I think?) I think the default setup here would be useful for a majority of cases (with config for acronyms, include/exclude etc)

But it may also "nudge" folks to not stray from the "standard" of what is supported here, which may be a small negative I suppose.

That would be robust and simple, but I guess RuboCop can only work with source code and should not boot the app.

Yes, this is the eternal issue of static vs runtime. If we could assume a booted app, then the acronyms config would also be unnecessary.

[Earlopain]

Is it possible to set default include/exclude list for a cop? I didn't see anything like that, and honestly other than this one cop, I haven't worked that deeply with rubocop in general.

Oh yeah, you can totally do this. Check out Rails/ActiveRecordCallbacksOrder for example which just looks at app/models. You just add this to the default config here in this repo like you would do in your own project specific config.

rubocop-rails/config/default.yml

Lines 133 to 139 in aa30af9

	Rails/ActiveRecordCallbacksOrder:
	Description: 'Order callback declarations in the order in which they will be executed.'
	StyleGuide: 'https://rails.rubystyle.guide/#callbacks-order'
	Enabled: 'pending'
	VersionAdded: '2.7'
	Include:
	- app/models/**/*.rb

Exclude works the same.

[shioyama]

@Earlopain thanks for the tip, I added a default include directive to only include ruby files in the /app directory. Seems like a reasonable starting place.