Fix for Negated condition violators (retry)

src/Refactoring-Core-Tests/RBConditionTest.class.st

@@ -295,3 +295,19 @@ RBConditionTest >> testTrue [
 	self assert: RBCondition true check.
 	self deny: RBCondition true not check.
 ]
+
+{ #category : 'tests' }
+RBConditionTest >> testViolatorsForNegatedConditionAreCorrect [
+
+	| abstract concrete condition negation doubleNegation |
+	abstract := { TestCase . Number }.
+	concrete := { Object . Point }.
+
+	condition := ReClassesAreAbstractCondition new classes: abstract, concrete.
+	negation := condition not.
+	doubleNegation := negation not.
+
+	self assert: condition violators asSet equals: concrete asSet.
+	self assert: negation violators asSet equals: abstract asSet.
+	self assert: doubleNegation violators asSet equals: condition violators asSet
+]

src/Refactoring-Core-Tests/ReDefinesSelectorsConditionTest.class.st

@@ -30,6 +30,19 @@ ReDefinesSelectorsConditionTest >> testClassDirectlyDefinesSelector [
 	" the class MyClassARoot defines the methods #accessing and #accessingSharedVariable"
 	self assert: def check. 	
 	self assert: def violators isEmpty.
+	self assert: def nonViolators equals: {#accessing . #accessingSharedVariable}
+]
+
+{ #category : 'accessing' }
+ReDefinesSelectorsConditionTest >> testClassDirectlyDefinesSelectorWithMessage [
+
+	| myClassARoot def |
+	myClassARoot := self model classNamed: #MyClassARoot.
+
+	def := ReDefinesSelectorsCondition new definesSelectors: {#accessing . #accessingSharedVariable}  in: myClassARoot.
+
+	" the class MyClassARoot defines the methods #accessing and #accessingSharedVariable"
+	self assert: def errorString isEmpty
 
 ]
 
@@ -44,7 +57,8 @@ ReDefinesSelectorsConditionTest >> testClassDoesNotDirectlyDefinesSelector [
 	" the class MyClassARoot does not define method #accessingZZZ"
 	self deny: def check. 	
 	self assert: def violators isNotEmpty.
-	self assert: def violators size equals: 1
+	self assert: def violators size equals: 1.
+	self assert: def nonViolators equals: { #accessingSharedVariable }
 
 ]
 
@@ -59,8 +73,35 @@ ReDefinesSelectorsConditionTest >> testClassDoesNotDirectlyDefinesSelectorEvenIf
 	" the class MyClassBetaSub does not define method #fooAccessor"
 	self deny: def check. 	
 	self assert: def violators isNotEmpty.
-	self assert: def violators size equals: 1
+	self assert: def violators size equals: 1.
+	self assert: def nonViolators isEmpty
+
+]
+
+{ #category : 'accessing' }
+ReDefinesSelectorsConditionTest >> testClassDoesNotDirectlyDefinesSelectorEvenIfSuperClassDoesWithMessage [
 
+	| myClassARoot def |
+	myClassARoot := self model classNamed: #MyClassBetaSub.
+
+	def := ReDefinesSelectorsCondition new definesSelectors: { #fooAccessor }  in: myClassARoot.
+
+	" the class MyClassBetaSub does not define method #fooAccessor"
+	self assert: ('*fooAccessor*' match: def errorString)
+]
+
+{ #category : 'accessing' }
+ReDefinesSelectorsConditionTest >> testClassDoesNotDirectlyDefinesSelectorWithMessage [
+
+	| myClassARoot def defined undefined |
+	myClassARoot := self model classNamed: #MyClassARoot.
+	undefined := #accessingZZZ.
+	defined := #accessingSharedVariable.
+	def := ReDefinesSelectorsCondition new definesSelectors: { defined . undefined }  in: myClassARoot.
+
+	" the class MyClassARoot does not define method #accessingZZZ"
+	self assert: ('*' , undefined , '*' match: def errorString).
+	self deny: ('*' , defined , '*' match: def errorString)
 ]
 
 { #category : 'accessing' }
@@ -75,5 +116,6 @@ ReDefinesSelectorsConditionTest >> testMetaClassDirectlyDefinesSelector [
 	" the class MyClassARoot defines the methods #accessing and #accessingSharedVariable"
 	self assert: def check. 	
 	self assert: def violators isEmpty.
+	self assert: def nonViolators equals: { #accessingFromClassSide }
 
 ]

src/Refactoring-Core-Tests/ReMethodsDontReferToInstVarsTest.class.st

@@ -24,20 +24,37 @@ ReMethodsDontReferToInstVarsTest >> testMethodReferencingInstVarDefinedInItsOwnC
 	cond := ReMethodsDontReferToInstVarsCondition new 
 		class: myClassBeta selectors: { #methodReferencingInstanceVariable }.
 
-	" the method refers to and instance variable defined in its own defining class, therefore the condition fails "
-	self deny: cond check
+	" the method refers to an instance variable defined in its own defining class, therefore the condition fails "
+	self deny: cond check.
+	self assert: cond violators equals: { #(#methodReferencingInstanceVariable #instVarB) } asSet.
+	self assert: cond nonViolators isEmpty 
 ]
 
 { #category : 'tests' }
 ReMethodsDontReferToInstVarsTest >> testMethodReferencingInstVarDefinedInItsOwnClassAndAnotherOneDefinedInItsSuperclass [
+
 	| myClassBetaSub cond |
 	myClassBetaSub := self model classNamed: #MyClassBetaSub.
+
+	cond := ReMethodsDontReferToInstVarsCondition new
+		        class: myClassBetaSub
+		        selectors: { #methodReferencingInstVarDefinedInItsDefiningClassAndOneInItsSuperclass }. " the method refers to and instance variable defined in its own defining class, therefore the condition fails "
+	self deny: cond check.
+	self assert: cond violators equals: { #( #methodReferencingInstVarDefinedInItsDefiningClassAndOneInItsSuperclass
+		   #instVarBSub ) } asSet.
+	self assert: cond nonViolators isEmpty
+]
+
+{ #category : 'tests' }
+ReMethodsDontReferToInstVarsTest >> testMethodReferencingInstVarDefinedInItsOwnClassWithMessage [
+	| myClassBeta cond |
+	myClassBeta := self model classNamed: #MyClassBeta.
 
 	cond := ReMethodsDontReferToInstVarsCondition new 
-		class: myClassBetaSub selectors: { #methodReferencingInstVarDefinedInItsDefiningClassAndOneInItsSuperclass }.
+		class: myClassBeta selectors: { #methodReferencingInstanceVariable }.
 
-	" the method refers to and instance variable defined in its own defining class, therefore the condition fails "
-	self deny: cond check
+	" the method refers to an instance variable defined in its own defining class, therefore the condition fails "
+	self assert: ('*methodReferencingInstanceVariable*' match: cond errorString)
 ]
 
 { #category : 'tests' }
@@ -49,7 +66,9 @@ ReMethodsDontReferToInstVarsTest >> testMethodReferencingInstVarDefinedInSupercl
 		class: myClassBetaSub selectors: { #methodReferencingInstanceVariableDefinedInSuperclass }.
 
 	" the method only refers to an instance variable which is not defined in its own defining class. Therefore the condition succeeds "
-	self assert: cond check
+	self assert: cond check.
+	self assert: cond violators isEmpty.
+	self assert: cond nonViolators equals: { {#methodReferencingInstanceVariableDefinedInSuperclass . nil} } asSet
 ]
 
 { #category : 'tests' }
@@ -61,5 +80,19 @@ ReMethodsDontReferToInstVarsTest >> testMethodReferencingNoInstVars [
 		class: myClassBeta selectors: { #methodForPullUp }.
 
 	" the method does not refer to any instance variable therefore the condition succeeds "
-	self assert: cond check
+	self assert: cond check.
+	self assert: cond violators isEmpty.
+	self assert: cond nonViolators equals: { {#methodForPullUp . nil} } asSet
+]
+
+{ #category : 'tests' }
+ReMethodsDontReferToInstVarsTest >> testMethodReferencingNoInstVarsWithMessage [
+	| myClassBeta cond |
+	myClassBeta := self model classNamed: #MyClassBeta.
+
+	cond := ReMethodsDontReferToInstVarsCondition new 
+		class: myClassBeta selectors: { #methodForPullUp }.
+
+	" the method does not refer to any instance variable therefore the condition succeeds "
+	self assert: cond errorString isEmpty
 ]

src/Refactoring-Core-Tests/ReMethodsDontReferToSharedVarsTest.class.st

@@ -25,7 +25,21 @@ ReMethodsDontReferToSharedVarsTest >> testMethodReferencingInstVarDefinedInItsOw
 		class: myClassBeta selectors: { #methodReferencingSharedVarDefinedInItsDefiningClassAndOneInItsSuperclass }.
 
 	" the method refers to and shared variable defined in its own defining class, therefore the condition fails "
-	self deny: cond check
+	self deny: cond check.
+	self assert: cond violators equals: { { ##methodReferencingSharedVarDefinedInItsDefiningClassAndOneInItsSuperclass . #SharedVarB } } asSet.
+	self assert: cond nonViolators isEmpty.
+]
+
+{ #category : 'tests' }
+ReMethodsDontReferToSharedVarsTest >> testMethodReferencingInstVarDefinedInItsOwnClassAndAnotherOneDefinedInItsSuperclassWithMessage [
+	| myClassBeta cond |
+	myClassBeta := self model classNamed: #MyClassBeta.
+
+	cond := ReMethodsDontReferToLocalSharedVarsCondition new 
+		class: myClassBeta selectors: { #methodReferencingSharedVarDefinedInItsDefiningClassAndOneInItsSuperclass }.
+
+	" the method refers to and shared variable defined in its own defining class, therefore the condition fails "
+	self assert: ('*methodReferencingSharedVarDefinedInItsDefiningClassAndOneInItsSuperclass*SharedVarB*' match: cond errorString)
 ]
 
 { #category : 'tests' }
@@ -37,7 +51,21 @@ ReMethodsDontReferToSharedVarsTest >> testMethodReferencingNoSharedVariables [
 		class: myClassBeta selectors: { #methodForPullUp }.
 
 	" the method does not refer to shared variables "
-	self assert: cond check
+	self assert: cond check.
+	self assert: cond violators isEmpty.
+	self assert: cond nonViolators equals: {{ #methodForPullUp . nil }} asSet
+]
+
+{ #category : 'tests' }
+ReMethodsDontReferToSharedVarsTest >> testMethodReferencingNoSharedVariablesWithMessage [
+	| myClassBeta cond |
+	myClassBeta := self model classNamed: #MyClassBeta.
+
+	cond := ReMethodsDontReferToLocalSharedVarsCondition new 
+		class: myClassBeta selectors: { #methodForPullUp }.
+
+	" the method does not refer to shared variables "
+	self assert: cond errorString isEmpty
 ]
 
 { #category : 'tests' }

src/Refactoring-Core-Tests/ReMethodsHaveNoSendersConditionTest.class.st

@@ -0,0 +1,76 @@
+Class {
+	#name : 'ReMethodsHaveNoSendersConditionTest',
+	#superclass : 'TestCase',
+	#instVars : [
+		'model'
+	],
+	#category : 'Refactoring-Core-Tests-Conditions',
+	#package : 'Refactoring-Core-Tests',
+	#tag : 'Conditions'
+}
+
+{ #category : 'accessing' }
+ReMethodsHaveNoSendersConditionTest >> model [
+
+	^ model ifNil: [ model := RBNamespace onEnvironment:
+		  (RBClassEnvironment classes: {MyClassAlpha . MyClassBeta})]
+]
+
+{ #category : 'tests' }
+ReMethodsHaveNoSendersConditionTest >> testMethodWithNoSenders [
+	| myClassAlpha cond |
+	myClassAlpha := self model classNamed: #MyClassAlpha.
+
+	cond := ReMethodsHaveNoSendersCondition new model: self model;
+		classSelectorsMapping: { myClassAlpha -> { #methodWithNoSenders } }.
+
+	" the method is not sent by anyone "
+	self assert: cond check.
+	self assert: cond violators isEmpty.
+	self assert: cond nonViolators asSet equals: { myClassAlpha -> (myClassAlpha methodFor: #methodWithNoSenders) } asSet
+]
+
+{ #category : 'tests' }
+ReMethodsHaveNoSendersConditionTest >> testMethodWithNoSendersWithMessage [
+	| myClassAlpha cond |
+	myClassAlpha := self model classNamed: #MyClassAlpha.
+
+	cond := ReMethodsHaveNoSendersCondition new model: self model;
+		classSelectorsMapping: { myClassAlpha -> { #methodWithNoSenders } }.
+
+	" the method does not refer to shared variables "
+	self assert: cond errorString isEmpty
+]
+
+{ #category : 'tests' }
+ReMethodsHaveNoSendersConditionTest >> testMethodWithSend [
+	| myClassBeta cond violatorSelectors |
+	myClassBeta := self model classNamed: #myClassBeta.	
+	cond := ReMethodsHaveNoSendersCondition new model: self model;
+		classSelectorsMapping: { 
+			myClassBeta -> { #methodSentFromAMethodInItsOwnClass } 
+		}.
+
+	" one of the methods have senders "
+	self deny: cond check.
+	violatorSelectors := (cond violators collect: [ :assoc | assoc key ]) asArray.
+	self assert: violatorSelectors equals: { #methodSentFromAMethodInItsOwnClass }.
+	self assert: cond nonViolators isEmpty
+]
+
+{ #category : 'tests' }
+ReMethodsHaveNoSendersConditionTest >> testMethodWithSuperSend [
+	| myClassAlpha myClassBeta cond violatorSelectors |
+	myClassAlpha := self model classNamed: #MyClassAlpha.
+	myClassBeta := self model classNamed: #MyClassBeta.
+	cond := ReMethodsHaveNoSendersCondition new model: self model;
+		classSelectorsMapping: { 
+			myClassAlpha -> { #methodWithSuperSend }
+		}.
+
+	" the method is sent from MyClassBeta >> #methodWithSuperSend "
+	self deny: cond check.
+	violatorSelectors := (cond violators collect: [ :assoc | assoc key ]) asArray.
+	self assert: violatorSelectors equals: { #methodWithSuperSend }.
+	self assert: cond nonViolators isEmpty
+]

src/Refactoring-Core/RBNewAbstractCondition.class.st

@@ -14,6 +14,12 @@ RBNewAbstractCondition >> & aCondition [
 	^RBAndCondition new left: self right: aCondition
 ]
 
+{ #category : 'accessing - private' }
+RBNewAbstractCondition >> candidates [
+	"Complex conditions require this method to compute their own subjects"
+	^ subjects
+]
+
 { #category : 'checking' }
 RBNewAbstractCondition >> check [
 	self subclassResponsibility
@@ -42,12 +48,6 @@ RBNewAbstractCondition >> not [
 	^ReNewNegationCondition on: self
 ]
 
-{ #category : 'accessing - private' }
-RBNewAbstractCondition >> subjects [
-	"Complex conditions require this method to compute their own subjects"
-	^ subjects
-]
-
 { #category : 'displaying' }
 RBNewAbstractCondition >> violationMessageOn: aStream [
 	self subclassResponsibility 

src/Refactoring-Core/ReClassHasSubclassesCondition.class.st

@@ -12,8 +12,7 @@ Class {
 { #category : 'accessing' }
 ReClassHasSubclassesCondition >> class: aClass [
 
-	classes := { aClass }.
-	subjects := classes
+	classes := { aClass }
 ]
 
 { #category : 'accessing' }
@@ -36,5 +35,5 @@ ReClassHasSubclassesCondition >> violators [
 		  violators := ((self theClass allSubclasses collect: [ :each |
 			                 each name ]) includesAll: subclasses)
 			               ifTrue: [ {  } ]
-			               ifFalse: [ subjects ] ]
+			               ifFalse: [ self candidates ] ]
 ]

src/Refactoring-Core/ReClassesAreNotMetaClassCondition.class.st

@@ -37,5 +37,5 @@ ReClassesAreNotMetaClassCondition >> violationMessageOn: aStream [
 ReClassesAreNotMetaClassCondition >> violators [
 
 	^ violators ifNil: [
-		  violators := subjects select: [ :class | class isMeta ] ]
+		  violators := self candidates select: [ :class | class isMeta ] ]
 ]

src/Refactoring-Core/ReClassesCondition.class.st

@@ -10,6 +10,11 @@ Class {
 	#tag : 'Conditions'
 }
 
+{ #category : 'accessing - private' }
+ReClassesCondition >> candidates [
+	^ classes
+]
+
 { #category : 'checking' }
 ReClassesCondition >> check [ 
 
@@ -19,6 +24,5 @@ ReClassesCondition >> check [
 { #category : 'accessing' }
 ReClassesCondition >> classes: aRBClassCollection [
 
-	classes := aRBClassCollection.
-	subjects := classes.
+	classes := aRBClassCollection
 ]

src/Refactoring-Core/ReClassesExistCondition.class.st

@@ -6,12 +6,12 @@ Class {
 	#tag : 'Conditions'
 }
 
-{ #category : 'accessing' }
-ReClassesExistCondition >> classes: aCollection [
-	"I must override `classes:` because violators are symbols instead of classes.
-	To correctly compute `nonViolators`, subjects and violators must have matching types."
-	classes := aCollection.
-	subjects := classes keys 
+{ #category : 'accessing - private' }
+ReClassesExistCondition >> candidates [
+	"I must override `candidates` because my violators are symbols instead of classes.
+	To correctly compute `nonViolators`, candidates and violators must have matching types."
+	^ classes keys
+
 ]
 
 { #category : 'displaying' }