Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PrasannaBhavan2005 wants to merge 10 commits into
base: Pharo15
Choose a base branch
from
+214 −31
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a12fd64
Fix #19936: Forbid removing temporary variables with usages unless sh…
PrasannaBhavan2005 Aug 21, 2026
4fa377d
Merge branch 'Pharo15' into fix-19936-remove-temporary-variable
PrasannaBhavan2005 Aug 21, 2026
c04ab52
Update src/Refactoring-Transformations/RBRemoveTemporaryVariableTrans…
PrasannaBhavan2005 Aug 22, 2026
d58ccd5
Fix usage check in temporary variable removal precondition
PrasannaBhavan2005 Aug 22, 2026
7e49f8a
Merge branch 'Pharo15' into fix-19936-remove-temporary-variable
PrasannaBhavan2005 Aug 22, 2026
ad964b2
Use exactNodeDefines: in temporary variable precondition check
PrasannaBhavan2005 Aug 24, 2026
82b9ac0
Fix #19936: Check variable usages properly in RBRemoveTemporaryVariab…
PrasannaBhavan2005 Aug 24, 2026
2171aac
Refactor temporary variable check into helper methods to reduce compl…
PrasannaBhavan2005 Aug 25, 2026
15d2571
Add unit tests for helper methods and block shadowing in RBRemoveTemp…
PrasannaBhavan2005 Aug 26, 2026
730eafb
Fix #19936: Use whichUpNodeDefines: and exclude block arguments from …
PrasannaBhavan2005 Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 173 additions & 17 deletions src/Refactoring-Transformations-Tests/RBRemoveTemporaryVariableTransformationTest.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,40 @@ RBRemoveTemporaryVariableTransformationTest >> testClassDoesNotExist [
inClass: #RBTemporaryVariableTransformationTest)
]

{ #category : 'tests' }
RBRemoveTemporaryVariableTransformationTest >> testDefiningSequenceNodeAndHasUsagesInScope [

@balsa-sarenac balsa-sarenac Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we usually prefer to test one thing in a test, so this would've been two tests

@PrasannaBhavan2005 PrasannaBhavan2005 Sep 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I split testDefiningSequenceNodeAndHasUsagesInScope into two separate tests (testDefiningSequenceNode and testHasUsagesInScope) to keep each test focused on a single responsibility.?

@balsa-sarenac balsa-sarenac Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's nitpick, you don't have to, we can merge like this. I'm just mentioning it to get familiar with team preferences and to potentially have it in mind for future changes

@PrasannaBhavan2005 PrasannaBhavan2005 Sep 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, thanks for the feedback! I'll keep that in mind for future contributions. Looking forward to make more and more contris, thanks!


| 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 [

Expand All @@ -24,35 +58,55 @@ 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 [

| 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' }
Expand All @@ -63,3 +117,105 @@ RBRemoveTemporaryVariableTransformationTest >> testVariableDoesNotExist [
inMethod: #foo
inClass: #RBRemoveTemporaryVariableTransformationTest)
]

{ #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
]

{ #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 >> testVariableShadowedByBlockArgumentNotUsed [

| transformation class |
transformation := RBAddMethodTransformation
sourceCode: 'foo
| temp |
[ :temp | temp ] value: 1.
^42'
in: self changeMockClass name
withProtocol: #accessing.
transformation generateChanges.

transformation := RBRemoveTemporaryVariableTransformation
model: transformation model
variable: 'temp'
inMethod: #foo
inClass: self changeMockClass name.

self deny: transformation hasUsagesInScope.
transformation generateChanges.

class := transformation model classNamed: self changeMockClass name.
self assert: (class parseTreeForSelector: #foo) temporaries isEmpty
]

{ #category : 'tests' }
RBRemoveTemporaryVariableTransformationTest >> testVariableUsedDirectly [

| transformation |
transformation := RBAddMethodTransformation
sourceCode: 'foo
| temp |
temp'
in: self changeMockClass name
withProtocol: #accessing.
transformation generateChanges.

transformation := RBRemoveTemporaryVariableTransformation
model: transformation model
variable: 'temp'
inMethod: #foo
inClass: self changeMockClass name.

self assert: transformation hasUsagesInScope.
self should: [ transformation generateChanges ] raise: RBRefactoringError
]
55 changes: 41 additions & 14 deletions src/Refactoring-Transformations/RBRemoveTemporaryVariableTransformation.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,20 +53,25 @@ 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.
(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 argumentNames includes: variableName) not ]
Comment thread
PrasannaBhavan2005 marked this conversation as resolved.
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') }
Comment on lines +69 to +74

@balsa-sarenac balsa-sarenac Aug 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for this to be standalone precondition, but it's good like this as well, it's important we are fixing these.
I'm now thinking that this delegation to instnace variable is behavior-preserving precondition, but this is a tricky scenario where applicability preconditions will fail (variable has usages) and then behavior can still be fixed. I think we didn't have this scenario so far. So nothing to fix now, rather I have to re-think some scenarios and how to support this.

@PrasannaBhavan2005 PrasannaBhavan2005 Aug 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the review @balsa-sarenac!

I've applied your suggestion for ReDefinesSelectorsCondition, happy to hear that the precondition logic helps address the issue, let me know if any further changes needed

]

{ #category : 'scripting api - conditions' }
Expand All @@ -83,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 [

Expand Down