Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ParsedConstantDefinitions doesn’t support fully qualified constants #14

Open
wildmaples opened this issue Sep 25, 2020 · 1 comment
Open
Labels
bug Something isn't working

Comments

@wildmaples
Copy link
Contributor

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

@wildmaples wildmaples added the bug Something isn't working label Sep 25, 2020
@tomstuart
Copy link
Contributor

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.

shioyama pushed a commit that referenced this issue May 21, 2021
remove shopify/shopify specific message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants