From a12fd641d70e07700e7cb447af5c31bbcdd2c632 Mon Sep 17 00:00:00 2001 From: PrasannaBhavan2005 Date: Fri, 21 Aug 2026 16:34:28 +0530 Subject: [PATCH 1/8] Fix #19936: Forbid removing temporary variables with usages unless shadowed --- ...mporaryVariableTransformationTest.class.st | 84 +++++++++++++++---- ...veTemporaryVariableTransformation.class.st | 40 +++++---- 2 files changed, 93 insertions(+), 31 deletions(-) diff --git a/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st b/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st index 5331303495f..d64c06ec24c 100644 --- a/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st +++ b/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 +] \ No newline at end of file diff --git a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st index 76a8171554c..623312f1cdd 100644 --- a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st +++ b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st @@ -53,20 +53,32 @@ RBRemoveTemporaryVariableTransformation class >> variable: aString inMethod: aSe { #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. + (RBCondition definesSelector: 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: [ + | sequence hasShadowedVar hasUsages | + sequence := (self definingMethod allChildren select: [ :each | each isSequence ]) + detect: [ :each | each defines: variableName ] + ifNone: [ nil ]. + hasUsages := sequence isNotNil and: [ + sequence allChildren anySatisfy: [ :node | + node isVariable and: [ node name = variableName ] ] ]. + hasShadowedVar := self definingClass definesInstanceVariable: variableName. + hasUsages not or: [ hasShadowedVar ] ] + errorString: 'Variable named ' , variableName + , ' cannot be removed because it is used in the method') } ] { #category : 'scripting api - conditions' } From c04ab521bdc2ae783e64e32256c69d21d0ff3faf Mon Sep 17 00:00:00 2001 From: Prasanna Bhavan Date: Sat, 22 Aug 2026 13:27:02 +0530 Subject: [PATCH 2/8] Update src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Балша Шаренац <34557616+balsa-sarenac@users.noreply.github.com> --- .../RBRemoveTemporaryVariableTransformation.class.st | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st index 623312f1cdd..c095fce8381 100644 --- a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st +++ b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st @@ -55,7 +55,7 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ ^ { self classExist. - (RBCondition definesSelector: selector in: self definingClass). + (ReDefinesSelectorsCondition new definesSelectors: { selector } in: self definingClass). (RBCondition withBlock: [ self definingMethod allTemporaryVariables includes: variableName ] From d58ccd5c4d1c5105b7965a61cbcf966504269a32 Mon Sep 17 00:00:00 2001 From: PrasannaBhavan2005 Date: Sat, 22 Aug 2026 18:32:57 +0530 Subject: [PATCH 3/8] Fix usage check in temporary variable removal precondition --- ...BRemoveTemporaryVariableTransformation.class.st | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st index c095fce8381..071828891ae 100644 --- a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st +++ b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st @@ -68,19 +68,19 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ , ' cannot be removed because it is an argument in this method'). (RBCondition withBlock: [ - | sequence hasShadowedVar hasUsages | - sequence := (self definingMethod allChildren select: [ :each | each isSequence ]) + | targetSequence hasRealUsages hasShadowedVar | + targetSequence := (self definingMethod allChildren select: [ :each | each isSequence ]) detect: [ :each | each defines: variableName ] ifNone: [ nil ]. - hasUsages := sequence isNotNil and: [ - sequence allChildren anySatisfy: [ :node | - node isVariable and: [ node name = variableName ] ] ]. + hasRealUsages := targetSequence isNotNil and: [ + targetSequence statements anySatisfy: [ :statement | + statement allChildren anySatisfy: [ :node | + node isVariable and: [ node name = variableName ] ] ] ]. hasShadowedVar := self definingClass definesInstanceVariable: variableName. - hasUsages not or: [ hasShadowedVar ] ] + hasRealUsages not or: [ hasShadowedVar ] ] errorString: 'Variable named ' , variableName , ' cannot be removed because it is used in the method') } ] - { #category : 'scripting api - conditions' } RBRemoveTemporaryVariableTransformation >> checkPreconditions [ From ad964b299c22d8c2ae9c456ab17782a488e7e1d3 Mon Sep 17 00:00:00 2001 From: PrasannaBhavan2005 Date: Mon, 24 Aug 2026 16:28:25 +0530 Subject: [PATCH 4/8] Use exactNodeDefines: in temporary variable precondition check --- .../RBRemoveTemporaryVariableTransformation.class.st | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st index 071828891ae..7be7aa57be4 100644 --- a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st +++ b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st @@ -70,7 +70,7 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ withBlock: [ | targetSequence hasRealUsages hasShadowedVar | targetSequence := (self definingMethod allChildren select: [ :each | each isSequence ]) - detect: [ :each | each defines: variableName ] + detect: [ :each | each exactNodeDefines: variableName ] ifNone: [ nil ]. hasRealUsages := targetSequence isNotNil and: [ targetSequence statements anySatisfy: [ :statement | @@ -81,6 +81,8 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ errorString: 'Variable named ' , variableName , ' cannot be removed because it is used in the method') } ] + + { #category : 'scripting api - conditions' } RBRemoveTemporaryVariableTransformation >> checkPreconditions [ From 82b9ac0be69312cc7173aa7c1013d90ad1de5ca4 Mon Sep 17 00:00:00 2001 From: PrasannaBhavan2005 Date: Mon, 24 Aug 2026 16:51:05 +0530 Subject: [PATCH 5/8] Fix #19936: Check variable usages properly in RBRemoveTemporaryVariableTransformation --- ...veTemporaryVariableTransformation.class.st | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st index 7be7aa57be4..9831ad512a6 100644 --- a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st +++ b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st @@ -68,21 +68,29 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ , ' cannot be removed because it is an argument in this method'). (RBCondition withBlock: [ - | targetSequence hasRealUsages hasShadowedVar | - targetSequence := (self definingMethod allChildren select: [ :each | each isSequence ]) + | definingSeq usages hasShadowedVar | + definingSeq := (self definingMethod allChildren select: [ :each | each isSequence ]) detect: [ :each | each exactNodeDefines: variableName ] ifNone: [ nil ]. - hasRealUsages := targetSequence isNotNil and: [ - targetSequence statements anySatisfy: [ :statement | - statement allChildren anySatisfy: [ :node | - node isVariable and: [ node name = variableName ] ] ] ]. + usages := definingSeq + ifNil: [ #() ] + ifNotNil: [ + definingSeq statements flatCollect: [ :stmt | + stmt allChildren select: [ :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 ] ] ] ] ] ] ]. hasShadowedVar := self definingClass definesInstanceVariable: variableName. - hasRealUsages not or: [ hasShadowedVar ] ] + usages isEmpty or: [ hasShadowedVar ] ] errorString: 'Variable named ' , variableName , ' cannot be removed because it is used in the method') } ] - { #category : 'scripting api - conditions' } RBRemoveTemporaryVariableTransformation >> checkPreconditions [ From 2171aac7f0cb46fd19f4eadbf48145d6c6f06224 Mon Sep 17 00:00:00 2001 From: PrasannaBhavan2005 Date: Tue, 25 Aug 2026 16:09:34 +0530 Subject: [PATCH 6/8] Refactor temporary variable check into helper methods to reduce complexity --- ...veTemporaryVariableTransformation.class.st | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st index 9831ad512a6..0e176c70bc9 100644 --- a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st +++ b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st @@ -50,7 +50,7 @@ RBRemoveTemporaryVariableTransformation class >> variable: aString inMethod: aSe yourself ] -{ #category : 'preconditions' } +{ #category : #preconditions } RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ ^ { @@ -68,29 +68,38 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ , ' cannot be removed because it is an argument in this method'). (RBCondition withBlock: [ - | definingSeq usages hasShadowedVar | - definingSeq := (self definingMethod allChildren select: [ :each | each isSequence ]) - detect: [ :each | each exactNodeDefines: variableName ] - ifNone: [ nil ]. - usages := definingSeq - ifNil: [ #() ] - ifNotNil: [ - definingSeq statements flatCollect: [ :stmt | - stmt allChildren select: [ :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 ] ] ] ] ] ] ]. - hasShadowedVar := self definingClass definesInstanceVariable: variableName. - usages isEmpty or: [ hasShadowedVar ] ] + 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 [ From 15d25716bf7aac24ac8216518cf9a3b7538c7a8c Mon Sep 17 00:00:00 2001 From: PrasannaBhavan2005 Date: Wed, 26 Aug 2026 12:00:23 +0530 Subject: [PATCH 7/8] Add unit tests for helper methods and block shadowing in RBRemoveTemporaryVariableTransformationTest --- ...mporaryVariableTransformationTest.class.st | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st b/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st index d64c06ec24c..3e27b195c06 100644 --- a/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st +++ b/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st @@ -112,4 +112,62 @@ RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithShadowin class := transformation model classNamed: self changeMockClass name. self assert: (class parseTreeForSelector: #foo) temporaries size equals: 1 +] + +{ #category : 'tests' } +RBRemoveTemporaryVariableTransformationTest >> testRemoveOuterUnusedTemporaryWhenInnerBlockShadowsSameName [ + + | transformation class | + transformation := RBAddMethodTransformation + sourceCode: 'foo + | sameName | + [ :x | | sameName | sameName := x * 2. sameName ] value: 5. + ^42' + in: self changeMockClass name + withProtocol: #accessing. + transformation generateChanges. + + transformation := RBRemoveTemporaryVariableTransformation + model: transformation model + variable: 'sameName' + inMethod: #foo + inClass: self changeMockClass name. + transformation generateChanges. + + class := transformation model classNamed: self changeMockClass name. + self assert: (class parseTreeForSelector: #foo) temporaries isEmpty +] + +{ #category : 'tests' } +RBRemoveTemporaryVariableTransformationTest >> testDefiningSequenceNodeAndHasUsagesInScope [ + + | transformation | + transformation := RBAddMethodTransformation + sourceCode: 'foo + | unused used | + used := 1. + ^used' + in: self changeMockClass name + withProtocol: #accessing. + transformation generateChanges. + + "Test helper methods on unused temp" + transformation := RBRemoveTemporaryVariableTransformation + model: transformation model + variable: 'unused' + inMethod: #foo + inClass: self changeMockClass name. + + self assert: transformation definingSequenceNode isNotNil. + self deny: transformation hasUsagesInScope. + + "Test helper methods on used temp" + transformation := RBRemoveTemporaryVariableTransformation + model: transformation model + variable: 'used' + inMethod: #foo + inClass: self changeMockClass name. + + self assert: transformation definingSequenceNode isNotNil. + self assert: transformation hasUsagesInScope ] \ No newline at end of file From 730eafb960dd82a9bd340033ed5add70c5d92285 Mon Sep 17 00:00:00 2001 From: PrasannaBhavan2005 Date: Thu, 3 Sep 2026 23:28:14 +0530 Subject: [PATCH 8/8] Fix #19936: Use whichUpNodeDefines: and exclude block arguments from preconditions --- ...mporaryVariableTransformationTest.class.st | 126 ++++++++++++------ ...veTemporaryVariableTransformation.class.st | 52 ++++---- 2 files changed, 111 insertions(+), 67 deletions(-) diff --git a/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st b/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st index 3e27b195c06..97013569cd3 100644 --- a/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st +++ b/src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st @@ -15,6 +15,40 @@ RBRemoveTemporaryVariableTransformationTest >> testClassDoesNotExist [ inClass: #RBTemporaryVariableTransformationTest) ] +{ #category : 'tests' } +RBRemoveTemporaryVariableTransformationTest >> testDefiningSequenceNodeAndHasUsagesInScope [ + + | transformation | + transformation := RBAddMethodTransformation + sourceCode: 'foo + | unused used | + used := 1. + ^used' + in: self changeMockClass name + withProtocol: #accessing. + transformation generateChanges. + + "Test helper methods on unused temp" + transformation := RBRemoveTemporaryVariableTransformation + model: transformation model + variable: 'unused' + inMethod: #foo + inClass: self changeMockClass name. + + self assert: transformation definingSequenceNode isNotNil. + self deny: transformation hasUsagesInScope. + + "Test helper methods on used temp" + transformation := RBRemoveTemporaryVariableTransformation + model: transformation model + variable: 'used' + inMethod: #foo + inClass: self changeMockClass name. + + self assert: transformation definingSequenceNode isNotNil. + self assert: transformation hasUsagesInScope +] + { #category : 'tests - failures' } RBRemoveTemporaryVariableTransformationTest >> testMethodDoesNotExist [ @@ -24,6 +58,30 @@ RBRemoveTemporaryVariableTransformationTest >> testMethodDoesNotExist [ inClass: #RBRemoveTemporaryVariableTransformationTest) ] +{ #category : 'tests' } +RBRemoveTemporaryVariableTransformationTest >> testRemoveOuterUnusedTemporaryWhenInnerBlockShadowsSameName [ + + | transformation class | + transformation := RBAddMethodTransformation + sourceCode: 'foo + | sameName | + [ :x | | sameName | sameName := x * 2. sameName ] value: 5. + ^42' + in: self changeMockClass name + withProtocol: #accessing. + transformation generateChanges. + + transformation := RBRemoveTemporaryVariableTransformation + model: transformation model + variable: 'sameName' + inMethod: #foo + inClass: self changeMockClass name. + transformation generateChanges. + + class := transformation model classNamed: self changeMockClass name. + self assert: (class parseTreeForSelector: #foo) temporaries isEmpty +] + { #category : 'tests' } RBRemoveTemporaryVariableTransformationTest >> testTransform [ @@ -61,10 +119,16 @@ RBRemoveTemporaryVariableTransformationTest >> testVariableDoesNotExist [ ] { #category : 'tests' } -RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithoutShadowing [ +RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithShadowing [ + + | transformation class | + transformation := RBAddVariableTransformation + instanceVariable: 'temp' + class: self changeMockClass name. + transformation generateChanges. - | transformation | transformation := RBAddMethodTransformation + model: transformation model sourceCode: 'foo | temp bar | bar := 5. @@ -79,21 +143,17 @@ RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithoutShado variable: 'temp' inMethod: #foo inClass: self changeMockClass name. - - self should: [ transformation generateChanges ] raise: RBRefactoringError + transformation generateChanges. + + class := transformation model classNamed: self changeMockClass name. + self assert: (class parseTreeForSelector: #foo) temporaries size equals: 1 ] { #category : 'tests' } -RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithShadowing [ - - | transformation class | - transformation := RBAddVariableTransformation - instanceVariable: 'temp' - class: self changeMockClass name. - transformation generateChanges. +RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithoutShadowing [ + | transformation | transformation := RBAddMethodTransformation - model: transformation model sourceCode: 'foo | temp bar | bar := 5. @@ -108,20 +168,18 @@ RBRemoveTemporaryVariableTransformationTest >> testVariableHasUsagesWithShadowin 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 + + self should: [ transformation generateChanges ] raise: RBRefactoringError ] { #category : 'tests' } -RBRemoveTemporaryVariableTransformationTest >> testRemoveOuterUnusedTemporaryWhenInnerBlockShadowsSameName [ +RBRemoveTemporaryVariableTransformationTest >> testVariableShadowedByBlockArgumentNotUsed [ | transformation class | transformation := RBAddMethodTransformation sourceCode: 'foo - | sameName | - [ :x | | sameName | sameName := x * 2. sameName ] value: 5. + | temp | + [ :temp | temp ] value: 1. ^42' in: self changeMockClass name withProtocol: #accessing. @@ -129,9 +187,11 @@ RBRemoveTemporaryVariableTransformationTest >> testRemoveOuterUnusedTemporaryWhe transformation := RBRemoveTemporaryVariableTransformation model: transformation model - variable: 'sameName' + variable: 'temp' inMethod: #foo inClass: self changeMockClass name. + + self deny: transformation hasUsagesInScope. transformation generateChanges. class := transformation model classNamed: self changeMockClass name. @@ -139,35 +199,23 @@ RBRemoveTemporaryVariableTransformationTest >> testRemoveOuterUnusedTemporaryWhe ] { #category : 'tests' } -RBRemoveTemporaryVariableTransformationTest >> testDefiningSequenceNodeAndHasUsagesInScope [ +RBRemoveTemporaryVariableTransformationTest >> testVariableUsedDirectly [ | transformation | transformation := RBAddMethodTransformation sourceCode: 'foo - | unused used | - used := 1. - ^used' + | temp | + temp' in: self changeMockClass name withProtocol: #accessing. transformation generateChanges. - "Test helper methods on unused temp" transformation := RBRemoveTemporaryVariableTransformation model: transformation model - variable: 'unused' - inMethod: #foo - inClass: self changeMockClass name. - - self assert: transformation definingSequenceNode isNotNil. - self deny: transformation hasUsagesInScope. - - "Test helper methods on used temp" - transformation := RBRemoveTemporaryVariableTransformation - model: transformation model - variable: 'used' + variable: 'temp' inMethod: #foo inClass: self changeMockClass name. - self assert: transformation definingSequenceNode isNotNil. - self assert: transformation hasUsagesInScope -] \ No newline at end of file + self assert: transformation hasUsagesInScope. + self should: [ transformation generateChanges ] raise: RBRefactoringError +] diff --git a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st index 0e176c70bc9..c5a929b5186 100644 --- a/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st +++ b/src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st @@ -50,7 +50,7 @@ RBRemoveTemporaryVariableTransformation class >> variable: aString inMethod: aSe yourself ] -{ #category : #preconditions } +{ #category : 'preconditions' } RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ ^ { @@ -63,7 +63,7 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ , ' does not define a temporary variable named ' , variableName). (RBCondition withBlock: [ - (self definingMethod allArgumentVariables includes: variableName) not ] + (self definingMethod argumentNames includes: variableName) not ] errorString: 'Variable named ' , variableName , ' cannot be removed because it is an argument in this method'). (RBCondition @@ -74,32 +74,6 @@ RBRemoveTemporaryVariableTransformation >> applicabilityPreconditions [ , ' 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 [ @@ -114,6 +88,28 @@ RBRemoveTemporaryVariableTransformation >> classExist [ errorString: 'No such class or trait named ' , className asString ] +{ #category : 'private - accessing' } +RBRemoveTemporaryVariableTransformation >> definingSequenceNode [ + + ^ (self definingMethod allChildren select: [ :each | each isSequence ]) + detect: [ :each | each exactNodeDefines: variableName ] + ifNone: [ nil ] +] + +{ #category : 'private-testing' } +RBRemoveTemporaryVariableTransformation >> hasUsagesInScope [ + + | definingSequence | + definingSequence := self definingSequenceNode. + definingSequence ifNil: [ ^ false ]. + + ^ definingSequence statements anySatisfy: [ :statement | + statement withAllChildren anySatisfy: [ :node | + node isVariable + and: [ node name = variableName + and: [ (node whichUpNodeDefines: variableName) == definingSequence ] ] ] ] +] + { #category : 'executing' } RBRemoveTemporaryVariableTransformation >> privateTransform [