[Bug Report] Dependency violation not flagged when Zeitwerk collapsing is used

[pareeohnos]

Description
A package calling another package is not triggering a violation

To Reproduce

• Create app/modules folder
• Create two packages in the new folder, with package.yml files enforcing dependencies
• Add a class in each package. Make one call the other
• Run bin/packwerk check

Expected Behaviour
A violation error to be raised

Screenshots
I've created a reproduction of the issue here https://github.com/pareeohnos/packwerk-issue

Version Information

• Packwerk: 3.2
• Ruby 3.3.0

Additional Context
The project structure contains modules in the app/modules directory. Within each package, each sub-directory is collapsed in Zeitwerk, so for example the directory app/modules/package_one/services/service_one.rb is loaded its PackageOne::ServiceOne instead of PackageOne::Services::ServiceOne.

I've tried removing the dependency on . and it triggers violations, but this is because the ApplicationController and ApplicationRecord classes are both located in the root of the project - it still makes not mention of the cross package violation.

If I remove the collapsing altogether, it starts to work correctly but this causes issues in our application so we need the collapsing.

[fxn]

Packwerk uses its own constants resolver.

The last version of Zeitwerk provides Zeitwerk::Loader#all_expected_cpaths that could help Packwerk honor user configuration like that by delegating that resolution to the Rails autoloaders.

[pareeohnos]

Is this something that would be a straightforward swap out? We're in the fairly early days of our project so want to enforce the boundaries from the start. If this is a fairly easy fix I could try and take a look but I currently know nothing of the internals of Packwerk

[Catsuko]

This is acknowledged in the README:

Support for custom Zeitwerk configuration is limited. If custom ActiveSupport inflections are used, Packwerk will understand that and everything is fine. However, if Zeitwerk is directly configured with custom Zeitwerk inflections or to collapse directories, Packwerk will get confused and produce false positives.

I also hit this problem while trying to get a similar namespace to the one you mentioned 😭 Although I ended up giving up, while investigating I came across this gem which might be of interest to you.

[pareeohnos]

@Catsuko yeah I found that snippet yesterday afternoon :( I'll take a look at that other gem though thanks

[gmcgibbon]

Indeed the issue is:

Dir[Rails.root.join('app/modules/{*/*/}')].each do |dir|
  Rails.autoloaders.main.collapse(dir)
end

Constant resolver is what we use to map constants to files, and unfortunately I don't think we want to support this use-case. Rails.autoloaders is actually private API, so you arguably shouldn't be fighting with the frameworks' autoloading conventions. I would suggest using Rails engines to define package boundaries instead of your current setup with custom nesting. Since there's nothing to do here, I'm closing this for now.

[fxn]

Rails.autoloaders is actually private API

No, no, it has always been public API. The rationale is that it would be a lost race to abstract all the features into new configuration points in Rails, so we preserve autoload_paths and friends for backwards compat, but for custom namespaces, custom inflectors, ignoring things, collapsing, etc., you configure the autoloaders.

See, for example the section for custom namespaces, collapsing directories for STIs, eager loading individual directories, etc.

Packwerk boots the application, and for the same price, it would be more complete and aligned to resolve constants using the autoloaders.

I wrote all_expected_cpaths with Packwerk in mind, indeed.

[exterm]

It seems to me that all_expected_cpaths should be considered as a path forward - I'm just not sure there's anyone right now interested in doing this refactor.

There are other major things that could be done to improve packwerk - switching to native Prism parsing for example - which are not currently being tackled.

Packwerk is mainly using constant_resolver because of performance considerations that don't seem to be valid anymore: In the beginning we optimized for cold start performance for IDE integration, so we didn't want to boot the app. When we started booting the app, there was no straightforward way to use that context for constant resolution. Now there seems to be one, so it looks like the logical next step to use it.

[Catsuko]

@exterm @fxn

It seems to me that all_expected_cpaths should be considered as a path forward - I'm just not sure there's anyone right now interested in doing this refactor.

I am interested since I really want to use collapse however I don't quite understand how you would use all_expected_cpaths 🤔

My understanding from this thread is that when finding paths for constants, rather than using ConstantResolver we would use the Zeitwerk::Loader instead. So would you invert the all_expected_cpaths map and then use that to look up the path from the constant?

[fxn]

The natural map is from file system path to constant path. Because the file system is the input, from there, you compute the expected constant path at the spot. You cannot go from constant path to file system.

Now, when scanning source code, Packwerk needs to know where is the constant supposed to be located. So, from that initial map, you can go in the other direction too.

However, when you do, take into account namespaces may be spread in multiple directories and, in the case of explicit ones, a Ruby file. So, inverting the map would need to account for collections.

There is also the edge case of shadowed files. They can also produce collections for files that are not necessarily namespaces.

[Catsuko]

However, when you do, take into account namespaces may be spread in multiple directories and, in the case of explicit ones, a Ruby file. So, inverting the map would need to account for collections.

Ah I guess that is why the constant resolver will raise an error on duplicate files. Since Packwerk will check this as part of the validation step, it should simplify the problem if we keep the same behaviour.

[fxn]

I read that code a while back and could be mistaken, but I think that only deals with files with ".rb" extension. That is different (addresses shadowed files, I believe).

But, if Packwerk does not deal with namespace ownership, then directory entries can be ignored.

[Catsuko]

@fxn
I made a proof of concept here, you can see my use of all_expected_cpaths. It is passing all tests (with some setup changes) and also working for me on my repo.

If you had any time to take a look, I'd be eager to hear if this lines up with what you were thinking 🤔

[fxn]

Hey @Catsuko! In principle, the retrieved information and usage seem correct to me. People from the Packwerk project would be better informed to have an opinion on the overall patch.