Fix #19936: Forbid removing temporary variables with usages unless shadowed

src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st

@@ -29,30 +29,26 @@ RBRemoveTemporaryVariableTransformationTest >> testTransform [
 
 	| transformation class |
 	transformation := RBAddMethodTransformation
-		                  sourceCode: 'foo
-									| temp bar |
-									bar := 5.
-									temp := bar * bar.
-									Transcript show: temp printString; cr.
-									^temp * temp'
-		                  in: self changeMockClass name
-		                  withProtocol: #accessing.
+		sourceCode: 'foo
+			| temp bar unused |
+			bar := 5.
+			temp := bar * bar.
+			Transcript show: temp printString; cr.
+			^temp * temp'
+		in: self changeMockClass name
+		withProtocol: #accessing.
 	transformation generateChanges.
 
 	transformation := RBRemoveTemporaryVariableTransformation
-		                  model: transformation model
-		                  variable: 'temp'
-		                  inMethod: #foo
-		                  inClass: self changeMockClass name.
+		model: transformation model
+		variable: 'unused'
+		inMethod: #foo
+		inClass: self changeMockClass name.
 	transformation generateChanges.
 
 	self assert: transformation model changes changes size equals: 2.
-
 	class := transformation model classNamed: self changeMockClass name.
-	self assert: (class directlyDefinesMethod: #foo).
-	self
-		assert: (class parseTreeForSelector: #foo) temporaries size
-		equals: 1
+	self assert: (class parseTreeForSelector: #foo) temporaries size equals: 2
 ]
 
 { #category : 'tests - failures' }
@@ -63,3 +59,57 @@ RBRemoveTemporaryVariableTransformationTest >> testVariableDoesNotExist [
 			 inMethod: #foo
 			 inClass: #RBRemoveTemporaryVariableTransformationTest)
 ]
+
+{ #category : 'tests' }
+RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithoutShadowing [
+
+	| transformation |
+	transformation := RBAddMethodTransformation
+		sourceCode: 'foo
+			| temp bar |
+			bar := 5.
+			temp := bar * bar.
+			^temp'
+		in: self changeMockClass name
+		withProtocol: #accessing.
+	transformation generateChanges.
+
+	transformation := RBRemoveTemporaryVariableTransformation
+		model: transformation model
+		variable: 'temp'
+		inMethod: #foo
+		inClass: self changeMockClass name.
+
+	self should: [ transformation generateChanges ] raise: RBRefactoringError
+]
+
+{ #category : 'tests' }
+RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithShadowing [
+
+	| transformation class |
+	transformation := RBAddVariableTransformation
+		instanceVariable: 'temp'
+		class: self changeMockClass name.
+	transformation generateChanges.
+
+	transformation := RBAddMethodTransformation
+		model: transformation model
+		sourceCode: 'foo
+			| temp bar |
+			bar := 5.
+			temp := bar * bar.
+			^temp'
+		in: self changeMockClass name
+		withProtocol: #accessing.
+	transformation generateChanges.
+
+	transformation := RBRemoveTemporaryVariableTransformation
+		model: transformation model
+		variable: 'temp'
+		inMethod: #foo
+		inClass: self changeMockClass name.
+	transformation generateChanges.
+
+	class := transformation model classNamed: self changeMockClass name.
+	self assert: (class parseTreeForSelector: #foo) temporaries size equals: 1
+]

src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st

@@ -50,25 +50,56 @@ RBRemoveTemporaryVariableTransformation class >> variable: aString inMethod: aSe
 		yourself
 ]
 
-{ #category : 'preconditions' }
+{ #category : #preconditions }
 RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [
 
-	^ {
-		  self classExist.
-		 (ReDefinesSelectorsCondition new definesSelectors: (Array with: selector) in: self definingClass).
-		  (RBCondition
-			   withBlock: [
-			   self definingMethod allTemporaryVariables includes: variableName ]
-			   errorString: 'Method named ' , selector
-				   , ' does not define a temporary variable named ' , variableName).
-		  (RBCondition
-			   withBlock: [
-				   (self definingMethod allArgumentVariables includes:
-					    variableName) not ]
-			   errorString: 'Variable named ' , variableName
-				   , ' cannot be removed because it is an argument in this method') }
+	^ { 
+		self classExist.
+		(ReDefinesSelectorsCondition new definesSelectors: { selector } in: self definingClass).
+		(RBCondition
+			withBlock: [ 
+				self definingMethod allTemporaryVariables includes: variableName ]
+			errorString: 'Method named ' , selector
+				, ' does not define a temporary variable named ' , variableName).
+		(RBCondition
+			withBlock: [ 
+				(self definingMethod allArgumentVariables includes: variableName) not ]
+			errorString: 'Variable named ' , variableName
+				, ' cannot be removed because it is an argument in this method').
+		(RBCondition
+			withBlock: [ 
+				self hasUsagesInScope not or: [ 
+					self definingClass definesInstanceVariable: variableName ] ]
+			errorString: 'Variable named ' , variableName
+				, ' cannot be removed because it is used in the method') }
 ]
 
+{ #category : #'private - accessing' }
+RBRemoveTemporaryVariableTransformation >> definingSequenceNode [
+
+	^ (self definingMethod allChildren select: [ :each | each isSequence ])
+		detect: [ :each | each exactNodeDefines: variableName ]
+		ifNone: [ nil ]
+]
+
+{ #category : #'private - testing' }
+RBRemoveTemporaryVariableTransformation >> hasUsagesInScope [
+
+	| definingSeq |
+	definingSeq := self definingSequenceNode.
+	definingSeq ifNil: [ ^ false ].
+
+	^ definingSeq statements anySatisfy: [ :stmt | 
+		stmt allChildren anySatisfy: [ :node | 
+			node isVariable and: [ 
+				node name = variableName and: [ 
+					| innerSeq |
+					innerSeq := node parent.
+					[ innerSeq isNotNil and: [ innerSeq isSequence not ] ] 
+						whileTrue: [ innerSeq := innerSeq parent ].
+					innerSeq == definingSeq or: [ 
+						innerSeq isNil or: [ (innerSeq exactNodeDefines: variableName) not ] ] ] ] ] ]
+]
 { #category : 'scripting api - conditions' }
 RBRemoveTemporaryVariableTransformation >> checkPreconditions [