From d76e91e171b7484bac62ac7ebabeefa0ca0df4c7 Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Sat, 12 Sep 2020 16:18:14 +0100 Subject: [PATCH] Add new Lint/ConstantDefinitionInBlock cop Constants defined within a block are added to the current lexical scope, which means the block has no effect on where the constant is defined or how it can be accessed. I often see constants defined inside Rake tasks or RSpec `describe` blocks where the intent was clearly for them to be scoped to the block, but which actually leak into the top-level scope. --- CHANGELOG.md | 1 + config/default.yml | 5 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_lint.adoc | 43 ++++++ lib/rubocop.rb | 1 + .../cop/lint/constant_definition_in_block.rb | 54 +++++++ .../lint/constant_definition_in_block_spec.rb | 137 ++++++++++++++++++ tasks/prof.rake | 8 +- 8 files changed, 246 insertions(+), 4 deletions(-) create mode 100644 lib/rubocop/cop/lint/constant_definition_in_block.rb create mode 100644 spec/rubocop/cop/lint/constant_definition_in_block_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index fe501e276fe..b5e497fd9bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * [#8582](https://github.com/rubocop-hq/rubocop/issues/8582): Add new `Layout/BeginEndAlignment` cop. ([@koic][]) * [#8699](https://github.com/rubocop-hq/rubocop/pull/8699): Add new `Lint/IdentityComparison` cop. ([@koic][]) * Add new `Lint/UselessTimes` cop. ([@dvandersluis][]) +* [#8707](https://github.com/rubocop-hq/rubocop/pull/8707): Add new `Lint/ConstantDefinitionInBlock` cop. ([@eugeneius][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index 1ef1af085d0..f0b9390c8da 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1395,6 +1395,11 @@ Lint/CircularArgumentReference: Enabled: true VersionAdded: '0.33' +Lint/ConstantDefinitionInBlock: + Description: 'Do not define constants within a block.' + Enabled: pending + VersionAdded: '0.91' + Lint/ConstantResolution: Description: 'Check that constants are fully qualified with `::`.' Enabled: false diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 94d8b5c946f..b4425d42bae 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -190,6 +190,7 @@ In the following section you find all available cops: * xref:cops_lint.adoc#lintbinaryoperatorwithidenticaloperands[Lint/BinaryOperatorWithIdenticalOperands] * xref:cops_lint.adoc#lintbooleansymbol[Lint/BooleanSymbol] * xref:cops_lint.adoc#lintcircularargumentreference[Lint/CircularArgumentReference] +* xref:cops_lint.adoc#lintconstantdefinitioninblock[Lint/ConstantDefinitionInBlock] * xref:cops_lint.adoc#lintconstantresolution[Lint/ConstantResolution] * xref:cops_lint.adoc#lintdebugger[Lint/Debugger] * xref:cops_lint.adoc#lintdeprecatedclassmethods[Lint/DeprecatedClassMethods] diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index 9c472cbaf81..541741ac62d 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -350,6 +350,49 @@ def cook(dry_ingredients = self.dry_ingredients) end ---- +== Lint/ConstantDefinitionInBlock + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| No +| 0.91 +| - +|=== + +Do not define constants within a block, since the block's scope does not +isolate or namespace the constant in any way. + +Define the constant outside of the block instead, or use a variable or +method if defining the constant in the outer scope would be problematic. + +=== Examples + +[source,ruby] +---- +# bad +task :lint do + FILES_TO_LINT = Dir['lib/*.rb'] +end + +# bad +describe 'making a request' do + class TestRequest; end +end + +# good +task :lint do + files_to_lint = Dir['lib/*.rb'] +end + +# good +describe 'making a request' do + let(:test_request) { Class.new } +end +---- + == Lint/ConstantResolution |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index c026820f877..6f2c985b554 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -252,6 +252,7 @@ require_relative 'rubocop/cop/lint/binary_operator_with_identical_operands' require_relative 'rubocop/cop/lint/boolean_symbol' require_relative 'rubocop/cop/lint/circular_argument_reference' +require_relative 'rubocop/cop/lint/constant_definition_in_block' require_relative 'rubocop/cop/lint/constant_resolution' require_relative 'rubocop/cop/lint/debugger' require_relative 'rubocop/cop/lint/deprecated_class_methods' diff --git a/lib/rubocop/cop/lint/constant_definition_in_block.rb b/lib/rubocop/cop/lint/constant_definition_in_block.rb new file mode 100644 index 00000000000..230b5c3f9fa --- /dev/null +++ b/lib/rubocop/cop/lint/constant_definition_in_block.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Lint + # Do not define constants within a block, since the block's scope does not + # isolate or namespace the constant in any way. + # + # Define the constant outside of the block instead, or use a variable or + # method if defining the constant in the outer scope would be problematic. + # + # @example + # # bad + # task :lint do + # FILES_TO_LINT = Dir['lib/*.rb'] + # end + # + # # bad + # describe 'making a request' do + # class TestRequest; end + # end + # + # # good + # task :lint do + # files_to_lint = Dir['lib/*.rb'] + # end + # + # # good + # describe 'making a request' do + # let(:test_request) { Class.new } + # end + class ConstantDefinitionInBlock < Base + MSG = 'Do not define constants within a block.' + + def_node_matcher :constant_assigned_in_block?, <<~PATTERN + ({^block_type? [^begin_type? ^^block_type?]} nil? ...) + PATTERN + + def_node_matcher :module_defined_in_block?, <<~PATTERN + ({^block_type? [^begin_type? ^^block_type?]} ...) + PATTERN + + def on_casgn(node) + add_offense(node) if constant_assigned_in_block?(node) + end + + def on_class(node) + add_offense(node) if module_defined_in_block?(node) + end + alias on_module on_class + end + end + end +end diff --git a/spec/rubocop/cop/lint/constant_definition_in_block_spec.rb b/spec/rubocop/cop/lint/constant_definition_in_block_spec.rb new file mode 100644 index 00000000000..a93c49b2fd7 --- /dev/null +++ b/spec/rubocop/cop/lint/constant_definition_in_block_spec.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Lint::ConstantDefinitionInBlock do + subject(:cop) { described_class.new(config) } + + let(:config) { RuboCop::Config.new } + + it 'does not register an offense for a top-level constant' do + expect_no_offenses(<<~RUBY) + FOO = 1 + RUBY + end + + it 'does not register an offense for a top-level constant followed by another statement' do + expect_no_offenses(<<~RUBY) + FOO = 1 + bar + RUBY + end + + it 'registers an offense for a constant defined within a block' do + expect_offense(<<~RUBY) + describe do + FOO = 1 + ^^^^^^^ Do not define constants within a block. + end + RUBY + end + + it 'registers an offense for a constant defined within a block followed by another statement' do + expect_offense(<<~RUBY) + describe do + FOO = 1 + ^^^^^^^ Do not define constants within a block. + bar + end + RUBY + end + + it 'does not register an offense for a top-level class' do + expect_no_offenses(<<~RUBY) + class Foo; end + RUBY + end + + it 'does not register an offense for a top-level class followed by another statement' do + expect_no_offenses(<<~RUBY) + class Foo; end + bar + RUBY + end + + it 'registers an offense for a class defined within a block' do + expect_offense(<<~RUBY) + describe do + class Foo; end + ^^^^^^^^^^^^^^ Do not define constants within a block. + end + RUBY + end + + it 'registers an offense for a class defined within a block followed by another statement' do + expect_offense(<<~RUBY) + describe do + class Foo; end + ^^^^^^^^^^^^^^ Do not define constants within a block. + bar + end + RUBY + end + + it 'does not register an offense for a top-level module' do + expect_no_offenses(<<~RUBY) + module Foo; end + RUBY + end + + it 'does not register an offense for a top-level module followed by another statement' do + expect_no_offenses(<<~RUBY) + module Foo; end + bar + RUBY + end + + it 'registers an offense for a module defined within a block' do + expect_offense(<<~RUBY) + describe do + module Foo; end + ^^^^^^^^^^^^^^^ Do not define constants within a block. + end + RUBY + end + + it 'registers an offense for a module defined within a block followed by another statement' do + expect_offense(<<~RUBY) + describe do + module Foo; end + ^^^^^^^^^^^^^^^ Do not define constants within a block. + bar + end + RUBY + end + + it 'does not register an offense for a constant with an explicit self scope' do + expect_no_offenses(<<~RUBY) + describe do + self::FOO = 1 + end + RUBY + end + + it 'does not register an offense for a constant with an explicit self scope followed by another statement' do + expect_no_offenses(<<~RUBY) + describe do + self::FOO = 1 + bar + end + RUBY + end + + it 'does not register an offense for a constant with an explicit top-level scope' do + expect_no_offenses(<<~RUBY) + describe do + ::FOO = 1 + end + RUBY + end + + it 'does not register an offense for a constant with an explicit top-level scope followed by another statement' do + expect_no_offenses(<<~RUBY) + describe do + ::FOO = 1 + bar + end + RUBY + end +end diff --git a/tasks/prof.rake b/tasks/prof.rake index d6da3edb473..c709a5c10f1 100644 --- a/tasks/prof.rake +++ b/tasks/prof.rake @@ -1,7 +1,7 @@ # frozen_string_literal: true namespace :prof do - DUMP_PATH = 'tmp/stackprof.dump' + dump_path = 'tmp/stackprof.dump' desc 'Run RuboCop on itself with profiling on' task :run, [:path] do |_task, args| @@ -11,13 +11,13 @@ namespace :prof do end task :run_if_needed, [:path] do - Rake::Task[:run].run unless File.exist?(DUMP_PATH) + Rake::Task[:run].run unless File.exist?(dump_path) end desc 'List the slowest cops' task slow_cops: :run_if_needed do method = 'RuboCop::Cop::Commissioner#trigger_responding_cops' - cmd = "stackprof #{DUMP_PATH} --text --method '#{method}'" + cmd = "stackprof #{dump_path} --text --method '#{method}'" puts cmd output = `#{cmd}` _header, list, _code = *output @@ -33,7 +33,7 @@ namespace :prof do warn 'usage: bundle exec rake walk[Class#method]' exit! end - cmd = "stackprof #{DUMP_PATH} --walk --method '#{method}'" + cmd = "stackprof #{dump_path} --walk --method '#{method}'" puts cmd system cmd end