From 3c5838ad97776ac25e2a0f382b4ad1cf905f5d56 Mon Sep 17 00:00:00 2001 From: Dana Sherson Date: Sat, 20 Jun 2020 22:34:54 +1000 Subject: [PATCH] Add Lint/ConstantResolution cop (#8142) 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, Ambiguous, 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 --- CHANGELOG.md | 1 + config/default.yml | 9 ++ docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_lint.adoc | 90 ++++++++++++++ lib/rubocop.rb | 1 + lib/rubocop/cop/lint/constant_resolution.rb | 89 ++++++++++++++ .../cop/lint/constant_resolution_spec.rb | 113 ++++++++++++++++++ 7 files changed, 304 insertions(+) create mode 100644 lib/rubocop/cop/lint/constant_resolution.rb create mode 100644 spec/rubocop/cop/lint/constant_resolution_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f733f029d0..2ba0aadac29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 `Lint/ConstantResolution` cop. ([@robotdana][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index 62525f1e022..ddfbaa8eb22 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1361,6 +1361,15 @@ Lint/CircularArgumentReference: Enabled: true VersionAdded: '0.33' +Lint/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 from only looking at certain names + Ignore: [] + Lint/Debugger: Description: 'Check for debugger calls.' Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 340f20e7df2..003dfc3574f 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -187,6 +187,7 @@ In the following section you find all available cops: * xref:cops_lint.adoc#lintbigdecimalnew[Lint/BigDecimalNew] * xref:cops_lint.adoc#lintbooleansymbol[Lint/BooleanSymbol] * xref:cops_lint.adoc#lintcircularargumentreference[Lint/CircularArgumentReference] +* xref:cops_lint.adoc#lintconstantresolution[Lint/ConstantResolution] * xref:cops_lint.adoc#lintdebugger[Lint/Debugger] * xref:cops_lint.adoc#lintdeprecatedclassmethods[Lint/DeprecatedClassMethods] * xref:cops_lint.adoc#lintdeprecatedopensslconstant[Lint/DeprecatedOpenSSLConstant] diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index a23ce2c6a20..925ec93c28b 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -310,6 +310,96 @@ def cook(dry_ingredients = self.dry_ingredients) end ---- +== Lint/ConstantResolution + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Disabled +| Yes +| No +| 0.86 +| - +|=== + +Check that certain constants are fully qualified. + +This is not enabled by default because it would mark a lot of offenses +unnecessarily. + +Generally, gems should fully qualify all constants to avoid conflicts with +the code that uses the gem. Enable this cop without using `Only`/`Ignore` + +Large projects will over time end up with one or two constant names that +are problematic because of a conflict with a library or just internally +using the same name a namespace and a class. To avoid too many unnecessary +offenses, Enable this cop with `Only: [The, Constant, Names, Causing, Issues]` + +=== 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 +|=== + == Lint/Debugger |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 1c1ba69e389..f67ffe9a7bd 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -242,6 +242,7 @@ require_relative 'rubocop/cop/lint/big_decimal_new' require_relative 'rubocop/cop/lint/boolean_symbol' require_relative 'rubocop/cop/lint/circular_argument_reference' +require_relative 'rubocop/cop/lint/constant_resolution' require_relative 'rubocop/cop/lint/debugger' require_relative 'rubocop/cop/lint/deprecated_class_methods' require_relative 'rubocop/cop/lint/deprecated_open_ssl_constant' diff --git a/lib/rubocop/cop/lint/constant_resolution.rb b/lib/rubocop/cop/lint/constant_resolution.rb new file mode 100644 index 00000000000..15eb7a755f2 --- /dev/null +++ b/lib/rubocop/cop/lint/constant_resolution.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Lint + # Check that certain constants are fully qualified. + # + # This is not enabled by default because it would mark a lot of offenses + # unnecessarily. + # + # Generally, gems should fully qualify all constants to avoid conflicts with + # the code that uses the gem. Enable this cop without using `Only`/`Ignore` + # + # Large projects will over time end up with one or two constant names that + # are problematic because of a conflict with a library or just internally + # using the same name a namespace and a class. To avoid too many unnecessary + # offenses, Enable this cop with `Only: [The, Constant, Names, Causing, Issues]` + # + # @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 diff --git a/spec/rubocop/cop/lint/constant_resolution_spec.rb b/spec/rubocop/cop/lint/constant_resolution_spec.rb new file mode 100644 index 00000000000..19b170464c9 --- /dev/null +++ b/spec/rubocop/cop/lint/constant_resolution_spec.rb @@ -0,0 +1,113 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Lint::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