Fix #19936: Forbid removing temporary variables with usages unless shadowed by PrasannaBhavan2005 · Pull Request #19937 · pharo-project/pharo

PrasannaBhavan2005 · 2026-08-21T11:01:19Z

Fixes #19936

Summary of changes

Added precondition to RBRemoveTemporaryVariableTransformation preventing removal of temporary variables when they are still referenced in the method body.
Allowed removal if an instance variable with the same name exists (shadowing).
Added corresponding unit tests covering both safe shadowing and usage-error scenarios.

…ges unless shadowed

Ducasse · 2026-08-21T13:55:14Z

Thanks I looks like a good addition. I will have a look later.

balsa-sarenac

Thanks! Mostly looks ok, can be merged as is, but there are small improvements we can make (first comment).

balsa-sarenac · 2026-08-22T07:46:33Z

+		(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') }


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.

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

…formation.class.st Co-authored-by: Балша Шаренац <34557616+balsa-sarenac@users.noreply.github.com>

balsa-sarenac · 2026-08-22T08:47:57Z

There are a bunch of failing tests after touching this code, can you take a look at them?
They are here: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-19937/4/testReport/

Before next iteration, be sure to run refactoring tests in your pharo image, things get to mergeable state faster

Tests - #4 - Jenkins

PrasannaBhavan2005 · 2026-08-22T08:59:36Z

Thanks @balsa-sarenac! I see the test failures across the composite refactorings (RBExtractMethodTransformation, ReRemoveUnusedTemporaryVariableRefactoring, etc.). I will pull the branch locally into my Pharo image, inspect the exact failure stack traces in the test runner, and adjust the precondition accordingly.

PrasannaBhavan2005 · 2026-08-22T13:04:42Z

Hi @balsa-sarenac,

I identified the cause of the test failures: the usage check was previously checking all children of the sequence, including the declaration of the temporary variable itself, leading to false positives across composite refactorings.

I've updated the precondition to inspect statement nodes specifically:

hasRealUsages := targetSequence isNotNil and: [ 
    targetSequence statements anySatisfy: [ :statement | 
        statement allChildren anySatisfy: [ :node | 
            node isVariable and: [ node name = variableName ] ] ] ].

balsa-sarenac · 2026-08-24T05:58:00Z

@PrasannaBhavan2005 there are still failing tests: https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-19937/6/testReport/

Check the image, maybe there's already a method/way to check if temp is used? It might be better to re-use an existing good solution vs.creating a new one.

Tests - #6 - Jenkins

…mporaryVariableTransformation

PrasannaBhavan2005 · 2026-08-24T13:15:20Z

Hi @balsa-sarenac,

The previous failures were caused by the usage check traversing nested sequence scopes (such as inner blocks that declare their own temporary variable with the same name), which broke RBMoveTemporaryVariableDefinitionTransformation.

I have updated the precondition check to ignore variable references occurring inside child sequences that define their own local variable:

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 ] ] ] ] ] ] ].

balsa-sarenac · 2026-08-25T06:39:32Z

@PrasannaBhavan2005 can you check: ReTemporaryNeitherReadNorWrittenRule.class.st:23 because there seem to be temp isReferenced and if it works, maybe we can use that instead? This is just increasing in size and edge-cases and I'm uneasy about accepting it. Let me know if it doesn't work, on why it doesn't work

…exity

PrasannaBhavan2005 · 2026-08-25T10:40:35Z

Hi @balsa-sarenac,

I investigated using temp isReferenced, but that pattern doesn't work I think

self definingMethod returns an RBMethodNode (refactoring AST), which doesn't implement #allTemporaryNodes — it only provides #allTemporaryVariables, returning String names rather than AST nodes.
Critic rules like ReTemporaryNeitherReadNorWrittenRule operate on OCMethodNode (compiled semantic ASTs), where #isReferenced is populated by semantic analysis. These semantic bindings are not present on raw RB parse trees during refactorings.

To address the complexity concern, I have decomposed the logic by extracting two dedicated private helper methods (#definingSequenceNode and #hasUsagesInScope). This keeps applicabilityPreconditions concise, clears all QA linter warnings, and passes the entire 59-test suite cleanly.
Let me know if any other such issues. Thanks

balsa-sarenac · 2026-08-26T06:19:02Z

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


Would it be possible to write some tests for this new code as well? In the future we might want to move this code to RB metamodel, so having tests is really useful in that case.
Thanks for cleaning it up, we're closing in on the merge side

…oraryVariableTransformationTest

PrasannaBhavan2005 · 2026-08-26T06:31:15Z

Hi @balsa-sarenac,

I've added the unit tests in RBRemoveTemporaryVariableTransformationTest covering:

#definingSequenceNode and #hasUsagesInScope on both used and unused temporaries.
Removing an unused outer temporary when an inner block shadows a temporary of the same name.

All 8 tests in the transformation test suite (and the broader refactoring suite) pass cleanly. Let me know if any changes needed

PrasannaBhavan2005 · 2026-08-28T09:47:18Z

@balsa-sarenac let me know if any issues

PrasannaBhavan2005 · 2026-09-02T20:01:26Z

Hey @balsa-sarenac it would be great if we can close this PR, has been a long time still no updates, let me know if anything else you need

balsa-sarenac · 2026-09-03T17:12:00Z

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


…guments from preconditions

PrasannaBhavan2005 · 2026-09-03T17:59:24Z

@balsa-sarenac I have updated the implementation and pushed the changes:

Updated hasUsagesInScope to use whichUpNodeDefines: so that shadowed variables in nested scopes/blocks are properly ignored.
Updated applicabilityPreconditions to use self definingMethod argumentNames instead of allArgumentVariables, ensuring block arguments are not misidentified as method arguments.
Added test cases:
- testVariableUsedDirectly
- testVariableShadowedByBlockArgumentNotUsed

All tests are passing locally.

balsa-sarenac · 2026-09-04T07:42:03Z

 ]

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


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

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

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

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

Fix pharo-project#19936: Forbid removing temporary variables with usa…

a12fd64

…ges unless shadowed

PrasannaBhavan2005 force-pushed the fix-19936-remove-temporary-variable branch from 1b21766 to a12fd64 Compare August 21, 2026 11:04

Ducasse requested a review from balsa-sarenac August 21, 2026 13:54

Merge branch 'Pharo15' into fix-19936-remove-temporary-variable

4fa377d

balsa-sarenac reviewed Aug 22, 2026

View reviewed changes

Update src/Refactoring-Transformations/RBRemoveTemporaryVariableTrans…

c04ab52

…formation.class.st Co-authored-by: Балша Шаренац <34557616+balsa-sarenac@users.noreply.github.com>

Fix usage check in temporary variable removal precondition

d58ccd5

Merge branch 'Pharo15' into fix-19936-remove-temporary-variable

7e49f8a

PrasannaBhavan2005 added 2 commits August 24, 2026 16:28

Use exactNodeDefines: in temporary variable precondition check

ad964b2

Fix pharo-project#19936: Check variable usages properly in RBRemoveTe…

82b9ac0

…mporaryVariableTransformation

Refactor temporary variable check into helper methods to reduce compl…

2171aac

…exity

balsa-sarenac reviewed Aug 26, 2026

View reviewed changes

Add unit tests for helper methods and block shadowing in RBRemoveTemp…

15d2571

…oraryVariableTransformationTest

balsa-sarenac reviewed Sep 3, 2026

View reviewed changes

Fix pharo-project#19936: Use whichUpNodeDefines: and exclude block ar…

730eafb

…guments from preconditions

balsa-sarenac reviewed Sep 4, 2026

View reviewed changes

balsa-sarenac added the Status: Reviewed and Ready to Integrate label Sep 4, 2026

Uh oh!

Uh oh!

Conversation

PrasannaBhavan2005 commented Aug 21, 2026

Summary of changes

Uh oh!

Ducasse commented Aug 21, 2026

Uh oh!

balsa-sarenac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balsa-sarenac commented Aug 22, 2026 • edited by unfurl-links Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PrasannaBhavan2005 commented Aug 22, 2026

Uh oh!

PrasannaBhavan2005 commented Aug 22, 2026

Uh oh!

balsa-sarenac commented Aug 24, 2026 • edited by unfurl-links Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PrasannaBhavan2005 commented Aug 24, 2026

Uh oh!

balsa-sarenac commented Aug 25, 2026

Uh oh!

PrasannaBhavan2005 commented Aug 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PrasannaBhavan2005 commented Aug 26, 2026

Uh oh!

PrasannaBhavan2005 commented Aug 28, 2026

Uh oh!

PrasannaBhavan2005 commented Sep 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PrasannaBhavan2005 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

balsa-sarenac commented Aug 22, 2026 •

edited by unfurl-links Bot

Loading

balsa-sarenac commented Aug 24, 2026 •

edited by unfurl-links Bot

Loading

PrasannaBhavan2005 commented Sep 3, 2026 •

edited

Loading