Skip to content

Commit

Permalink
Add Style/ConstantResolution cop
Browse files Browse the repository at this point in the history
Ruby constant resolution sometimes gets things wrong.

I think generally gems (that are meant for including as a library, not
like rubocop itself) should fully qualify all constants to avoid
stepping on any toes inadvertently. I would run this on my gems without
using `Only`/`Ignore`

Large projects will probably end up with one or two constant names that
are problematic because of a conflict with a library or just internally
having sensible names for directories that end up being used by e.g.
rails as namespaces in multiple different places. I would run these with
`Only: [The, Constant, Names, Causing Issues]`

Sometimes a method refers to a constant that you want to refer to a
different constant at different calls, e.g. the method is defined on a
superclass and the constant differs between subclasses. (e.g. MSG in
rubocop add_offense). I would use
`Ignore: [The, Intentionally, Abiguous, Constant]` in these cases.
(or `self.class::MSG` now that I've looked up how add_offense works)

I've left this `Enabled: false` because I'm 50-50 on whether it should
be Enabled by default.

I didn't bother creating an autocorrect because I see there being two
options and thought I'd get feedback.

1. carelessly assume the written constant is fully qualified and just
missing a ::` prefix. This has the advantage of probably looking correct
more often, but also being wrong a lot.
2. look at the current namespace and prefix that. This has the advantage
of not necessarily changing behaviour for classes. For modules leaving
it unchanged or carelessly assuming, or checking if the constant is
defined in the module or not before prefixing
3. just don't autocorrect
  • Loading branch information
robotdana committed Jun 20, 2020
1 parent a90ec5f commit 79d6a8e
Show file tree
Hide file tree
Showing 7 changed files with 282 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* [#8113](https://github.com/rubocop-hq/rubocop/pull/8113): Let `expect_offense` templates add variable-length whitespace with `_{foo}`. ([@eugeneius][])
* [#8148](https://github.com/rubocop-hq/rubocop/pull/8148): Support autocorrection for `Style/MultilineTernaryOperator`. ([@koic][])
* [#8151](https://github.com/rubocop-hq/rubocop/pull/8151): Support autocorrection for `Style/NestedTernaryOperator`. ([@koic][])
* [#8142](https://github.com/rubocop-hq/rubocop/pull/8142): Add `Style/ConstantResolution` cop. ([@robotdana][])

### Bug fixes

Expand Down
9 changes: 9 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2628,6 +2628,15 @@ Style/ConditionalAssignment:
SingleLineConditionsOnly: true
IncludeTernaryExpressions: true

Style/ConstantResolution:
Description: 'Check that constants are fully qualified with `::`.'
Enabled: false
VersionAdded: '0.86'
# Restrict this cop to only looking at certain names
Only: []
# Restrict this cop to from looking at certain names
Ignore: []

Style/ConstantVisibility:
Description: >-
Check that class- and module constants have
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ In the following section you find all available cops:
* xref:cops_style.adoc#stylecommentannotation[Style/CommentAnnotation]
* xref:cops_style.adoc#stylecommentedkeyword[Style/CommentedKeyword]
* xref:cops_style.adoc#styleconditionalassignment[Style/ConditionalAssignment]
* xref:cops_style.adoc#styleconstantresolution[Style/ConstantResolution]
* xref:cops_style.adoc#styleconstantvisibility[Style/ConstantVisibility]
* xref:cops_style.adoc#stylecopyright[Style/Copyright]
* xref:cops_style.adoc#styledatetime[Style/DateTime]
Expand Down
79 changes: 79 additions & 0 deletions docs/modules/ROOT/pages/cops_style.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -1465,6 +1465,85 @@ end
| Boolean
|===

== Style/ConstantResolution

|===
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Disabled
| Yes
| No
| 0.86
| -
|===

Check that constants are fully qualified.

=== Examples

[source,ruby]
----
# By default checks every constant
# bad
User
# bad
User::Login
# good
::User
# good
::User::Login
----

==== Only: ['Login']

[source,ruby]
----
# Restrict this cop to only being concerned about certain constants
# bad
Login
# good
::Login
# good
User::Login
----

==== Ignore: ['Login']

[source,ruby]
----
# Restrict this cop not being concerned about certain constants
# bad
User
# good
::User::Login
# good
Login
----

=== Configurable attributes

|===
| Name | Default value | Configurable values

| Only
| `[]`
| Array

| Ignore
| `[]`
| Array
|===

== Style/ConstantVisibility

|===
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@
require_relative 'rubocop/cop/style/comment_annotation'
require_relative 'rubocop/cop/style/commented_keyword'
require_relative 'rubocop/cop/style/conditional_assignment'
require_relative 'rubocop/cop/style/constant_resolution'
require_relative 'rubocop/cop/style/constant_visibility'
require_relative 'rubocop/cop/style/copyright'
require_relative 'rubocop/cop/style/date_time'
Expand Down
78 changes: 78 additions & 0 deletions lib/rubocop/cop/style/constant_resolution.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Style
# Check that constants are fully qualified.
#
# @example
# # By default checks every constant
#
# # bad
# User
#
# # bad
# User::Login
#
# # good
# ::User
#
# # good
# ::User::Login
#
# @example Only: ['Login']
# # Restrict this cop to only being concerned about certain constants
#
# # bad
# Login
#
# # good
# ::Login
#
# # good
# User::Login
#
# @example Ignore: ['Login']
# # Restrict this cop not being concerned about certain constants
#
# # bad
# User
#
# # good
# ::User::Login
#
# # good
# Login
#
class ConstantResolution < Cop
MSG = 'Fully qualify this constant to avoid possibly ambiguous resolution.'

def_node_matcher :unqualified_const?, <<~PATTERN
(const nil? #const_name?)
PATTERN

def on_const(node)
return unless unqualified_const?(node)

add_offense(node)
end

private

def const_name?(name)
name = name.to_s
(allowed_names.empty? || allowed_names.include?(name)) &&
!ignored_names.include?(name)
end

def allowed_names
cop_config['Only']
end

def ignored_names
cop_config['Ignore']
end
end
end
end
end
113 changes: 113 additions & 0 deletions spec/rubocop/cop/style/constant_resolution_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Style::ConstantResolution, :config do
it 'registers no offense when qualifying a const' do
expect_no_offenses(<<~RUBY)
::MyConst
RUBY
end

it 'registers no offense qualifying a namespace const' do
expect_no_offenses(<<~RUBY)
::MyConst::MY_CONST
RUBY
end

it 'registers an offense not qualifying a const' do
expect_offense(<<~RUBY)
MyConst
^^^^^^^ Fully qualify this constant to avoid possibly ambiguous resolution.
RUBY
end

it 'registers an offense not qualifying a namespace const' do
expect_offense(<<~RUBY)
MyConst::MY_CONST
^^^^^^^ Fully qualify this constant to avoid possibly ambiguous resolution.
RUBY
end

context 'with Only set' do
let(:cop_config) { { 'Only' => ['MY_CONST'] } }

it 'registers no offense when qualifying a const' do
expect_no_offenses(<<~RUBY)
::MyConst
RUBY
end

it 'registers no offense qualifying a namespace const' do
expect_no_offenses(<<~RUBY)
::MyConst::MY_CONST
RUBY
end

it 'registers no offense not qualifying another const' do
expect_no_offenses(<<~RUBY)
MyConst
RUBY
end

it 'registers no with a namespace const' do
expect_no_offenses(<<~RUBY)
MyConst::MY_CONST
RUBY
end

it 'registers an offense with an unqualified const' do
expect_offense(<<~RUBY)
MY_CONST
^^^^^^^^ Fully qualify this constant to avoid possibly ambiguous resolution.
RUBY
end

it 'registers an offense when an unqualified namespace const' do
expect_offense(<<~RUBY)
MY_CONST::B
^^^^^^^^ Fully qualify this constant to avoid possibly ambiguous resolution.
RUBY
end
end

context 'with Ignore set' do
let(:cop_config) { { 'Ignore' => ['MY_CONST'] } }

it 'registers no offense when qualifying a const' do
expect_no_offenses(<<~RUBY)
::MyConst
RUBY
end

it 'registers no offense qualifying a namespace const' do
expect_no_offenses(<<~RUBY)
::MyConst::MY_CONST
RUBY
end

it 'registers an offense not qualifying another const' do
expect_offense(<<~RUBY)
MyConst
^^^^^^^ Fully qualify this constant to avoid possibly ambiguous resolution.
RUBY
end

it 'registers an with a namespace const' do
expect_offense(<<~RUBY)
MyConst::MY_CONST
^^^^^^^ Fully qualify this constant to avoid possibly ambiguous resolution.
RUBY
end

it 'registers no offense with an unqualified const' do
expect_no_offenses(<<~RUBY)
MY_CONST
RUBY
end

it 'registers no offense when an unqualified namespace const' do
expect_no_offenses(<<~RUBY)
MY_CONST::B
RUBY
end
end
end

0 comments on commit 79d6a8e

Please sign in to comment.