From 79d6a8e9d70a49305c99d786dd94a688594b6bf3 Mon Sep 17 00:00:00 2001 From: Dana Sherson Date: Fri, 12 Jun 2020 09:27:08 +1000 Subject: [PATCH 1/3] Add Style/ConstantResolution cop 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 --- CHANGELOG.md | 1 + config/default.yml | 9 ++ docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_style.adoc | 79 ++++++++++++ lib/rubocop.rb | 1 + lib/rubocop/cop/style/constant_resolution.rb | 78 ++++++++++++ .../cop/style/constant_resolution_spec.rb | 113 ++++++++++++++++++ 7 files changed, 282 insertions(+) create mode 100644 lib/rubocop/cop/style/constant_resolution.rb create mode 100644 spec/rubocop/cop/style/constant_resolution_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f733f029d0..898452d64d8 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 `Style/ConstantResolution` cop. ([@robotdana][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index 62525f1e022..c1856c45362 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 340f20e7df2..522d7802f39 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -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] diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index d5203caa2c6..76dab985ef8 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -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 |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 1c1ba69e389..a9aad8b26d4 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -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' diff --git a/lib/rubocop/cop/style/constant_resolution.rb b/lib/rubocop/cop/style/constant_resolution.rb new file mode 100644 index 00000000000..abde3f88a91 --- /dev/null +++ b/lib/rubocop/cop/style/constant_resolution.rb @@ -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 diff --git a/spec/rubocop/cop/style/constant_resolution_spec.rb b/spec/rubocop/cop/style/constant_resolution_spec.rb new file mode 100644 index 00000000000..b95f662853f --- /dev/null +++ b/spec/rubocop/cop/style/constant_resolution_spec.rb @@ -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 From 96da2c4eb08ee8c96597b820fc26a29a2931ee6a Mon Sep 17 00:00:00 2001 From: Dana Sherson Date: Fri, 12 Jun 2020 13:30:37 +1000 Subject: [PATCH 2/3] Move Style/ConstantResolution to Lint/ This is a separate commit so we can easily not do it if you'd rather not --- CHANGELOG.md | 2 +- config/default.yml | 18 ++--- docs/modules/ROOT/pages/cops.adoc | 2 +- docs/modules/ROOT/pages/cops_lint.adoc | 79 +++++++++++++++++++ docs/modules/ROOT/pages/cops_style.adoc | 79 ------------------- lib/rubocop.rb | 2 +- .../{style => lint}/constant_resolution.rb | 2 +- .../constant_resolution_spec.rb | 2 +- 8 files changed, 93 insertions(+), 93 deletions(-) rename lib/rubocop/cop/{style => lint}/constant_resolution.rb (98%) rename spec/rubocop/cop/{style => lint}/constant_resolution_spec.rb (97%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 898452d64d8..2ba0aadac29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +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][]) +* [#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 c1856c45362..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 @@ -2628,15 +2637,6 @@ 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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 522d7802f39..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] @@ -335,7 +336,6 @@ 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] diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index a23ce2c6a20..5693680f8b1 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -310,6 +310,85 @@ 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 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 +|=== + == Lint/Debugger |=== diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index 76dab985ef8..d5203caa2c6 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -1465,85 +1465,6 @@ 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 |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index a9aad8b26d4..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' @@ -375,7 +376,6 @@ 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' diff --git a/lib/rubocop/cop/style/constant_resolution.rb b/lib/rubocop/cop/lint/constant_resolution.rb similarity index 98% rename from lib/rubocop/cop/style/constant_resolution.rb rename to lib/rubocop/cop/lint/constant_resolution.rb index abde3f88a91..22fcbebcb88 100644 --- a/lib/rubocop/cop/style/constant_resolution.rb +++ b/lib/rubocop/cop/lint/constant_resolution.rb @@ -2,7 +2,7 @@ module RuboCop module Cop - module Style + module Lint # Check that constants are fully qualified. # # @example diff --git a/spec/rubocop/cop/style/constant_resolution_spec.rb b/spec/rubocop/cop/lint/constant_resolution_spec.rb similarity index 97% rename from spec/rubocop/cop/style/constant_resolution_spec.rb rename to spec/rubocop/cop/lint/constant_resolution_spec.rb index b95f662853f..19b170464c9 100644 --- a/spec/rubocop/cop/style/constant_resolution_spec.rb +++ b/spec/rubocop/cop/lint/constant_resolution_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe RuboCop::Cop::Style::ConstantResolution, :config do +RSpec.describe RuboCop::Cop::Lint::ConstantResolution, :config do it 'registers no offense when qualifying a const' do expect_no_offenses(<<~RUBY) ::MyConst From 3db3ddd8124c40fc3d9c33cee1c2e392a555480e Mon Sep 17 00:00:00 2001 From: Dana Sherson Date: Sat, 20 Jun 2020 13:18:29 +1000 Subject: [PATCH 3/3] Move commit msg explanation into docs --- docs/modules/ROOT/pages/cops_lint.adoc | 13 ++++++++++++- lib/rubocop/cop/lint/constant_resolution.rb | 13 ++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index 5693680f8b1..925ec93c28b 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -322,7 +322,18 @@ end | - |=== -Check that constants are fully qualified. +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 diff --git a/lib/rubocop/cop/lint/constant_resolution.rb b/lib/rubocop/cop/lint/constant_resolution.rb index 22fcbebcb88..15eb7a755f2 100644 --- a/lib/rubocop/cop/lint/constant_resolution.rb +++ b/lib/rubocop/cop/lint/constant_resolution.rb @@ -3,7 +3,18 @@ module RuboCop module Cop module Lint - # Check that constants are fully qualified. + # 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