Shopify / Shopify/packwerk

ConstNodeInspector doesn’t try to resolve module namespaces

Open
#23 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Ruby
Stars
1.9k
Forks
121
Avg merge
2m
Merged PRs (30d)
1

Description

This is a copy-paste of @tomstuart's bug report, originally posted to https://github.com/Shopify/packwerk-old/issues/289

Describe the bug

When Packwerk::ConstNodeInspector finds a constant in a class or module definition, it uses the lexical nesting to fully-qualify that constant’s name. This is not always correct because a namespace may refer to a constant that has already been defined elsewhere, e.g. in another package.

This bug is a variation on #234: this time it’s a reference to an existing constant elsewhere in the enclosing namespace, rather than to the root namespace, which causes the problem.

To Reproduce

Say we have a pair of components, one and two, which define the modules A::B and A::C respectively:

% tree components
components
├── one
│   ├── a
│   │   └── b.rb
│   └── package.yml
└── two
    ├── a
    │   └── c.rb
    └── package.yml

Both package.yml files have enforce_dependencies and enforce_privacy set to true and no dependencies defined, so Packwerk should not allow either component to refer to the other.

In addition to defining A::B, the file one/a/b.rb also defines a nested class A::B::D:

module A
  module B
    class D
      def info
        'defined inside ::A::B by component one'
      end
    end
  end
end

And in addition to defining A::C, the file two/a/c.rb reopens that class and overwrites one of its methods:

module A
  module C
    class B::D
      def info
        'defined inside ::A::C by component two'
      end
    end
  end
end

It might not be immediately obvious that two is redefining A::B::D#info here, but it is, because the reference to B in class B::D gets resolved to the existing constant A::B, regardless of the definition occurring lexically inside module C. (This is what #234 is about.)

So when these two components are loaded, two refers to (and modifies) the class A::B::D from one:

% irb -Icomponents/one -Icomponents/two
>> require 'a/b'
=> true
>> require 'a/c'
=> true
>> A::B::D.new.info
=> "defined inside ::A::C by component two"
=> nil

packwerk check doesn’t detect this violation because it doesn’t realise that class B::D is a reference to A::B::D. It assumes it must refer to A::C::B::D because it appears inside module A; module C; …; end end, but Ruby constant resolution is more complex.

This doesn’t present an immediate problem within Shopify because we don’t use compact constant nesting (class B::D) in class definitions, but Packwerk may give incorrect results on external codebases.

Expected behavior

packwerk check should give an error like this:

Dependency violation: ::A::B::D belongs to 'components/one', but 'components/two' does not specify a dependency on 'components/one'.
Are we missing an abstraction?
Is the code making the reference, and the referenced constant, in the right packages?

Inference details: 'B::D' refers to ::A::B::D which seems to be defined in components/one/a/b.rb.

Version information

  • Packwerk: v0.1.10
  • Ruby: v2.6.6p146

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with lib/packwerk/const_node_inspector.rb, especially the lexical-nesting logic linked in the issue. Reproduce the components/one and components/two example and run packwerk check to compare its inference with Ruby's resolution of B::D. Done means the check reports ::A::B::D as belonging to components/one and identifies the missing dependency.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.