Fix for Negated condition violators (retry) #18738

carolahp · 2025-10-23T12:06:20Z

No description provided.

request-info · 2025-10-23T12:06:33Z

This issue has either a default title or empty body. We would appreciate it if you could provide more information. Note: I am not a very intelligent bot, I can only react to new comments. Please add a comment for me if you update the body or title.

balsa-sarenac

Just general comments not related to the PR, let me know when it's ready for full review.

balsa-sarenac · 2025-10-26T09:22:06Z

src/Refactoring-Core/RBNewAbstractCondition.class.st

+RBNewAbstractCondition >> candidates [
+	"Complex conditions require this method to compute their own subjects"
+	^ subjects


Should we rename iv to candidates too?

Or, should this be removed since accessor is used everywhere?

Yes! I am removing it since we are using the accessor everywhere

balsa-sarenac · 2025-10-26T10:30:38Z

src/Refactoring-Core/ReMethodsDontReferToLocalSharedVarsCondition.class.st

This is general comment on all preconditions, we can discuss this on Thursday: I think we should have all Conditions classes be positive. I mean:
ReMethodsDontReferToLocalSharedVarsCondition -> ReMethodsReferToLocalSharedVarsCondition

This is easier to use, and negation makes more sense. Double negation is always a bit confusing and harder to reason about.

Yes I agree!

I also agree

balsa-sarenac

What is the status of this PR? Should I do a full review?

src/Refactoring-Core/RBAndCondition.class.st

Co-authored-by: Балша Шаренац <[email protected]>

carolahp · 2025-11-13T12:31:26Z

What is the status of this PR? Should I do a full review?

I am about to finish, I only have 13 tests not passing.
I merged the changes from PR#18890, to work on the latest version, in a fresh image. I will merge it all back to this branch as soon as I fix the tests

…s-in-negated-conditions2

…olahp/pharo into fix/violators-in-negated-conditions2

Ducasse · 2025-11-15T20:13:53Z

There is a failing test

testFailure
Found undeclared Variables: 
RBProtectVariableTransformation in: RBProtectVariableTransformationTest>>#testVariableDoesNotExist
ReleaseTest(TestAsserter)>>assert:description:resumable:
ReleaseTest(TestAsserter)>>assert:description:
[ "we compile a second method with the undeclared #undeclaredStubInstVar1 to trigger the code path of removing twice in #cleanOutUndeclared"
	self class compile: 'methodForTest ^undeclaredStubInstVar1'.
	Smalltalk cleanOutUndeclared.
	undeclaredVariables := Undeclared associations select: [ :each | each isUndeclaredVariable ].

	validExceptions := { #undeclaredStubInstVar1. #undeclaredStubInstVar2. #Gofer }. "The laste one is for Smalltalk CI and our external projects... But we should find a better solution at some point."

	"for now we filter by name, maybe filtering by variable would be better"
	remaining := undeclaredVariables reject: [ :each | validExceptions includes: each name ].

	"we look for one of the using methods of the undeclared var and report that,
	this should be enough to fix it quickly"
	description := String streamContents: [ :stream |
		               stream nextPutAll: 'Found undeclared Variables: '.
		               remaining do: [ :variable |
			               | method |
			               method := variable usingMethods anyOne.
			               stream cr
				               nextPutAll: variable name;
				               nextPutAll: ' in: ';
				               print: method methodClass;
				               nextPutAll: '>>';
				               print: method selector ] ].

	self assert: remaining isEmpty description: description ] in ReleaseTest>>testUndeclared
FullBlockClosure(BlockClosure)>>ensure:
ReleaseTest>>testUndeclared
ReleaseTest(TestCase)>>performTest

jecisc · 2025-11-15T20:18:21Z

We should merge P14 in this branch because I fixed those tests 2 days ago I think

balsa-sarenac · 2025-11-21T12:53:25Z

src/Refactoring-Core/ReRenameClassRefactoring.class.st

-			   self doesClassToBeRenamedExist and: [ self isNotMetaclass ] ]).
-		  self isValidClassName.
-		  self isNotGlobal }
+		  ((ReClassesExistCondition new classes: { (class name -> class) }) & (ReClassesAreNotMetaClassCondition new classes: { class })).


These don't have to be joined with &, the have almost the same meaning if you put them as list items.

Suggested change

((ReClassesExistCondition new classes: { (class name -> class) }) & (ReClassesAreNotMetaClassCondition new classes: { class })).

((ReClassesExistCondition new classes: { (class name -> class) }).

(ReClassesAreNotMetaClassCondition new classes: { class })).

balsa-sarenac · 2025-11-21T12:54:28Z

src/Refactoring-Core/ReReifiedCondition.class.st

+ReReifiedCondition >> violationMessageOn: aStream violators: theViolators [
+	| isNegated |
+	theViolators ifEmpty:[^self].
+	isNegated := self violators ~= theViolators.
+
+	theViolators do: [ :violator |
+		aStream
+			nextPutAll: violator name;
+			nextPutAll: ('<1? is not:is>' expandMacrosWith: isNegated);
+			nextPutAll: ' empty.';
+			space ]


This is in ReifiedCondition, could it be that it ended up here by mistake? Since this is mentioning something empty?

balsa-sarenac · 2025-11-21T12:56:49Z

src/Refactoring-Core/ReNotInCascadedMessageCondition.class.st

+ReNotInCascadedMessageCondition >> violationMessageOn: aStream violators: theViolators [
+
+	theViolators = self violators 
+		ifTrue: [ aStream nextPutAll: 'Cannot extract code in a cascaded message.' ] 
+		ifFalse: [ aStream nextPutAll: 'Can extract code in a non cascaded message.' ]
+]


This is something to think about.
@Ducasse this condition is generic (is this message cascaded), and we use it in extract method, and this error message says you cannot extract method from cascade.
I think this should be general message here, and then Extract Method needs to make it specific for its use case?

balsa-sarenac · 2025-11-21T12:58:07Z

src/Refactoring-Core/ReNameIsGlobalCondition.class.st

 { #category : 'displaying' }
 ReNameIsGlobalCondition >> violationMessageOn: aStream [
-
+	self deprecated: 'Use violationMessageOn:violators: instead'.


Was this left here by mistake? or is this always deprecated? I forgot what we decided in the end

balsa-sarenac · 2025-11-21T16:33:48Z

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

@Ducasse these tests fail since ProtectVariableTransformation was removed. I'm guessing we should remove these tests as well? But also to make sure these tests exist for RBProtectInstanceVariableRefactoring

refactor: use candidates for computing nonViolators in MethodConditions

eb5b909

request-info bot added the more-information-needed label Oct 23, 2025

jecisc added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Oct 23, 2025

carolahp mentioned this pull request Oct 23, 2025

[Refactorings] Fix for Negated condition violators #18597

Closed

carolahp requested a review from balsa-sarenac October 23, 2025 13:50

carolahp marked this pull request as draft October 23, 2025 13:50

balsa-sarenac reviewed Oct 26, 2025

View reviewed changes

carolahp added 2 commits October 30, 2025 15:46

feat: refactor for conditions API and lots of tests

a00ca3a

fix: AndCondition nonViolators and NegationCondition not

a3ace41

balsa-sarenac reviewed Nov 10, 2025

View reviewed changes

src/Refactoring-Core/RBAndCondition.class.st Outdated Show resolved Hide resolved

fix: check for AndCondition

82637b7

Co-authored-by: Балша Шаренац <[email protected]>

carolahp added 2 commits November 13, 2025 13:35

Merge branch 'fix/violators-in-negated-conditions3' into fix/violator…

847f4c4

…s-in-negated-conditions2

Merge branch 'fix/violators-in-negated-conditions2' of github.com:car…

cae7925

…olahp/pharo into fix/violators-in-negated-conditions2

carolahp mentioned this pull request Nov 13, 2025

[Refactoring] Driver improvement for temp to instance variable #18821

Closed

Merge branch 'Pharo14' into fix/violators-in-negated-conditions2

647dc65

carolahp added 4 commits November 20, 2025 11:00

refactor: first steps to unify condition apis from parallel hierarchies

f27a4c9

refactor: violation message printing

e177c51

continue unifying apis

a9fd635

merge P14

ea49e25

balsa-sarenac reviewed Nov 21, 2025

View reviewed changes

	((ReClassesExistCondition new classes: { (class name -> class) }) & (ReClassesAreNotMetaClassCondition new classes: { class })).
	((ReClassesExistCondition new classes: { (class name -> class) }).
	(ReClassesAreNotMetaClassCondition new classes: { class })).

Uh oh!

Fix for Negated condition violators (retry) #18738

Are you sure you want to change the base?

Fix for Negated condition violators (retry) #18738

Conversation

carolahp commented Oct 23, 2025

Uh oh!

request-info bot commented Oct 23, 2025

Uh oh!

balsa-sarenac left a comment

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!

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!

balsa-sarenac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carolahp commented Nov 13, 2025

Uh oh!

Ducasse commented Nov 15, 2025

Uh oh!

jecisc commented Nov 15, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants