Shopify/packwerk

ParsedConstantDefinitions doesn’t support fully qualified constants

Opened this issue · 1 comments

RuboCop::AST::Node#const_name doesn’t reveal whether a constant is fully qualified (e.g. ::HELLO) or not:

>> RuboCop::ProcessedSource.new('::HELLO', 2.6).ast.const_name
=> "HELLO"

>> RuboCop::ProcessedSource.new('HELLO', 2.6).ast.const_name
=> "HELLO"

As a result, the implementation of ParsedConstantDefinitions#collect_local_definitions is unaware of whether a constant is fully qualified, so it’s unable to put it in the correct namespace:

>> definitions = ParsedConstantDefinitions.new(
     root_node: parse_code('module Sales; ::HELLO = "World"; end')
   )
=> #<Packwerk::ParsedConstantDefinitions
     @local_definitions=
      {"::Sales"=>#<Parser::Source::Range (string) 7...12>,
       "::Sales::HELLO"=>#<Parser::Source::Range (string) 16...21>}>

>> definitions.local_reference?('HELLO')
=> false # should be true

>> definitions.local_reference?('Sales::HELLO')
=> true # should be false

I’m not going to fix this right now because I’m trying to make progress on a different task, but I think it’s a bug that’s going to affect users.

cc: @tomstuart

Things have changed since I opened the original issue in June.

The good news is that Packwerk::Node.constant_name (née #const_name) is now aware of fully-qualified constant names:

>> Packwerk::Node.constant_name(Parser::CurrentRuby.parse('::HELLO'))
=> "::HELLO"

>> Packwerk::Node.constant_name(Parser::CurrentRuby.parse('HELLO'))
=> "HELLO"

The bad news is that ParsedConstantDefinitions#collect_local_definitions doesn’t know what to do with this:

>> definitions = Packwerk::ParsedConstantDefinitions.new(
     root_node: Parser::CurrentRuby.parse('module Sales; ::HELLO = "World"; end')
   )
=> #<Packwerk::ParsedConstantDefinitions
    @local_definitions=
     {"::Sales"=>#<struct Packwerk::Node::Location line=1, column=7>,
      "::Sales::::HELLO"=>#<struct Packwerk::Node::Location line=1, column=16>}>

>> definitions.local_reference?('HELLO')
=> false # should be true

>> definitions.local_reference?('Sales::HELLO')
=> false # correct, but for the wrong reason

You can see that @local_definitions now contains an erroneous entry for ::Sales::::HELLO, formed by naively joining 'Sales' and '::HELLO' with '::'. We still have no test coverage for this so it’s not affecting the build, but it’s obviously wrong behaviour that will bite anyone who tries to use fully-qualified constants.