Refine Style/GlobalExceptionVariables.

Author: samuel-williams-shopify

lib/rubocop/socketry/style/global_exception_variables.rb

@@ -8,7 +8,7 @@
 module RuboCop
 	module Socketry
 		module Style
-			# A RuboCop cop that warns against using global exception variables.
+			# A RuboCop cop that warns against using global exception variables in unsafe contexts.
 			# 
 			# This cop discourages the use of:
 			# - `$!` (last exception)
@@ -17,26 +17,52 @@ module Style
 			# - `$ERROR_POSITION` (English name for `$@`)
 			# 
 			# These global variables are implicit and can make code harder to understand.
-			# Instead, use explicit exception handling with rescue blocks and local variables.
+			# 
+			# However, this cop allows their use in safe contexts where the scope is well-defined:
+			# - Inside rescue blocks (well-defined scope)
+			# - In rescue modifiers (`expression rescue $!`)
+			# - In method parameter defaults (`def foo(error = $!)`, `def bar(error: $!)`)
+			# 
+			# This cop specifically flags their use in unsafe contexts:
+			# - Inside ensure blocks (extremely unsafe - exception state is unpredictable)
+			# - Outside of exception handling contexts
 			# 
 			# @example
-			#   # bad
+			#   # bad - unsafe in ensure block
 			#   begin
 			#     risky_operation
-			#   rescue
+			#   ensure
+			#     log($!.message) if $!  # unsafe!
+			#   end
+			# 
+			#   # bad - outside exception handling
+			#   def process
 			#     puts $!.message
-			#     puts [email protected]
 			#   end
 			# 
-			#   # good
+			#   # good - explicit exception handling
 			#   begin
 			#     risky_operation
 			#   rescue => error
 			#     puts error.message
-			#     puts error.backtrace.first
 			#   end
+			# 
+			#   # allowed - inside rescue block (well-defined scope)
+			#   begin
+			#     risky_operation
+			#   rescue
+			#     puts $!.message
+			#   end
+			# 
+			#   # allowed - rescue modifier
+			#   result = risky_operation rescue $!
+			# 
+			#   # allowed - parameter defaults
+			#   def foo(error = $!)
+			#   def bar(error: $!)
 			class GlobalExceptionVariables < RuboCop::Cop::Base
-				MSG = "Avoid using global exception variable `%<variable>s`. Use explicit exception handling with `rescue => error` instead."
+				MSG = "Avoid using global exception variable `%<variable>s` in %<context>s. Use explicit exception handling with `rescue => error` instead."
+				ENSURE_MSG = "Using global exception variable `%<variable>s` in an ensure block is extremely unsafe."
 
 				EXCEPTION_VARIABLES = %i[$! $@ $ERROR_INFO $ERROR_POSITION].freeze
 
@@ -45,11 +71,53 @@ def on_gvar(node)
 
 					return unless EXCEPTION_VARIABLES.include?(variable_name)
 
+					# Allow in parameter defaults (explicitly opting in)
+					return if in_parameter_default?(node)
+					
+					# Allow in rescue modifier (well-defined scope)
+					return if in_rescue_modifier?(node)
+					
+					# Allow in rescue block (well-defined scope)
+					return if in_rescue_block?(node)
+					
+					# Flag if in ensure block (extremely unsafe)
+					if in_ensure_block?(node)
+						add_offense(
+							node,
+							message: format(ENSURE_MSG, variable: variable_name)
+						)
+						return
+					end
+					
+					# Flag in all other contexts
 					add_offense(
 						node,
-						message: format(MSG, variable: variable_name)
+						message: format(MSG, variable: variable_name, context: "this context")
 					)
 				end
+				
+				private
+				
+				def in_parameter_default?(node)
+					node.each_ancestor(:args, :optarg, :kwoptarg).any?
+				end
+				
+				def in_rescue_modifier?(node)
+					node.each_ancestor(:rescue).any? do |ancestor|
+						# A rescue modifier has no resbody children
+						# e.g., `expression rescue $!` is a rescue node with 2 children: expression and handler
+						# A regular rescue has resbody children
+						!ancestor.children.any?{|child| child.is_a?(RuboCop::AST::Node) && child.type == :resbody}
+					end
+				end
+				
+				def in_rescue_block?(node)
+					node.each_ancestor(:resbody).any?
+				end
+				
+				def in_ensure_block?(node)
+					node.each_ancestor(:ensure).any?
+				end
 			end
 		end
 	end

test/rubocop/socketry/style/global_exception_variables.rb

@@ -9,8 +9,8 @@
 	let(:config) {RuboCop::Config.new}
 	let(:cop) {subject.new(config)}
 
-	# Test $! variable
-	with "code using $!" do
+	# Test $! variable in rescue block (allowed - well-defined scope)
+	with "code using $! in rescue block" do
 		let(:source) do
 			<<~RUBY
 				begin
@@ -21,18 +21,17 @@
 			RUBY
 		end
 
-		it "registers an offense" do
+		it "does not register an offense" do
 			processed_source = RuboCop::ProcessedSource.new(source, RUBY_VERSION.to_f)
 			investigator = RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
 			report = investigator.investigate(processed_source)
 			offenses = report.offenses
-			expect(offenses).not.to be(:empty?)
-			expect(offenses.first.message).to be(:include?, "$!")
+			expect(offenses).to be(:empty?)
 		end
 	end
 
-	# Test $@ variable
-	with "code using $@" do
+	# Test $@ variable in rescue block (allowed - well-defined scope)
+	with "code using $@ in rescue block" do
 		let(:source) do
 			<<~RUBY
 				begin
@@ -43,59 +42,74 @@
 			RUBY
 		end
 
-		it "registers an offense" do
+		it "does not register an offense" do
 			processed_source = RuboCop::ProcessedSource.new(source, RUBY_VERSION.to_f)
 			investigator = RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
 			report = investigator.investigate(processed_source)
 			offenses = report.offenses
-			expect(offenses).not.to be(:empty?)
-			expect(offenses.first.message).to be(:include?, "$@")
+			expect(offenses).to be(:empty?)
 		end
 	end
 
-	# Test $ERROR_INFO variable
-	with "code using $ERROR_INFO" do
+	# Test $! in ensure block (extremely unsafe - should be flagged)
+	with "code using $! in ensure block" do
 		let(:source) do
 			<<~RUBY
-				require 'English'
 				begin
 					risky_operation
-				rescue
-					puts $ERROR_INFO.message
+				ensure
+					log($!.message) if $!
 				end
 			RUBY
 		end
 
-		it "registers an offense" do
+		it "registers offenses with unsafe message" do
 			processed_source = RuboCop::ProcessedSource.new(source, RUBY_VERSION.to_f)
 			investigator = RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
 			report = investigator.investigate(processed_source)
 			offenses = report.offenses
-			expect(offenses).not.to be(:empty?)
-			expect(offenses.first.message).to be(:include?, "$ERROR_INFO")
+			expect(offenses.size).to be == 2
+			expect(offenses.first.message).to be(:include?, "extremely unsafe")
 		end
 	end
 
-	# Test $ERROR_POSITION variable
-	with "code using $ERROR_POSITION" do
+	# Test $! in rescue modifier (allowed)
+	with "code using $! in rescue modifier" do
 		let(:source) do
 			<<~RUBY
-				require 'English'
-				begin
-					risky_operation
-				rescue
-					puts $ERROR_POSITION.first
+				result = risky_operation rescue $!
+			RUBY
+		end
+		
+		it "does not register an offense" do
+			processed_source = RuboCop::ProcessedSource.new(source, RUBY_VERSION.to_f)
+			investigator = RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
+			report = investigator.investigate(processed_source)
+			offenses = report.offenses
+			expect(offenses).to be(:empty?)
+		end
+	end
+	
+	# Test $! in parameter defaults (allowed - explicitly opting in)
+	with "code using $! in parameter defaults" do
+		let(:source) do
+			<<~RUBY
+				def foo(error = $!)
+					puts error
+				end
+				
+				def bar(error: $!)
+					puts error
 				end
 			RUBY
 		end
 
-		it "registers an offense" do
+		it "does not register an offense" do
 			processed_source = RuboCop::ProcessedSource.new(source, RUBY_VERSION.to_f)
 			investigator = RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
 			report = investigator.investigate(processed_source)
 			offenses = report.offenses
-			expect(offenses).not.to be(:empty?)
-			expect(offenses.first.message).to be(:include?, "$ERROR_POSITION")
+			expect(offenses).to be(:empty?)
 		end
 	end
 
@@ -143,8 +157,8 @@
 		end
 	end
 
-	# Test multiple uses in same block
-	with "code using multiple global exception variables" do
+	# Test multiple uses in same rescue block (allowed - well-defined scope)
+	with "code using multiple global exception variables in rescue block" do
 		let(:source) do
 			<<~RUBY
 				begin
@@ -156,12 +170,12 @@
 			RUBY
 		end
 
-		it "registers multiple offenses" do
+		it "does not register offenses" do
 			processed_source = RuboCop::ProcessedSource.new(source, RUBY_VERSION.to_f)
 			investigator = RuboCop::Cop::Commissioner.new([cop], [], raise_error: true)
 			report = investigator.investigate(processed_source)
 			offenses = report.offenses
-			expect(offenses.size).to be == 2
+			expect(offenses).to be(:empty?)
 		end
 	end