ConstNodeInspector doesn’t try to resolve module namespaces
Nobody has claimed this yet.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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