PushDown refactoring should not replace method in a subclass

[Ducasse]

Imagine

A >> rikiki ^ 42

   B >>rikiki ^ 34

Pushdown A>>rikiki should not blindly replace rikiki ^ 42 in B

[Ducasse]

The following

	refactoring failedApplicabilityPreconditions ifNotEmpty: [
		^ self inform: 'The shared variable does not exist' ].

Does not make sense in the RePushDownMethodDriver

RePushDownMethodDriver >> configureAndRunRefactoring

	self configureRefactoring.

	refactoring failedApplicabilityPreconditions ifNotEmpty: [
		^ self inform: 'The shared variable does not exist' ].
	self setBreakingChangesPreconditions.
	refactoring failedBreakingChangePreconditions
		ifEmpty: [ self applyChanges ]
		ifNotEmpty: [ self handleBreakingChanges ]

[Ducasse]

The refactoring precondition is too weak.

breakingChangePreconditions
	"Check that that none of the subclasses of `class` is doing a supercall to any of the selectors
	that will be pushed down.
	
	Also, to ensure that an instance of the class is not sent a message which is pushed down,  forces that 
	we can only push down methods from abstract class. 
	This should be controlled via a flag on the ui.
	"

	^ OrderedCollection
		  with: self preconditionIsAbstract
		  with: self preconditionSubclassesDontSendSuper

[Ducasse]

@balsa-sarenac What do you think about this one.

[Ducasse]

In the old version (Pharo 10) of the refactoring we got

preconditions

	| condition |
	condition := selectors
		             inject: self emptyCondition
		             into: [ :cond :each | 
			             cond & (RBCondition definesSelector: each in: class)
			             &
			             (RBCondition subclassesOf: class referToSelector: each)
				             not ].
	^ condition & (RBCondition isAbstractClass: class)

subclassesOf: aClass referToSelector: aSelector 
	^self new 
		type: (Array 
				with: #subclassReferences
				with: aClass
				with: aSelector)
		block: 
			[(aClass subclasses detect: 
					[:each | 
					(each selectors detect: 
							[:sel | 
							| tree |
							tree := each parseTreeFor: sel.
							tree notNil and: [tree superMessages includes: aSelector]]
						ifNone: [nil]) notNil]
				ifNone: [nil]) notNil]
		errorString: '<1?:no:a> subclass of ' , aClass printString , ' refers to ' 
				, aSelector printString

[balsa-sarenac]

Good catch Steph, I tracked down the issue and saw that it got changed in this commit two years ago. Then we moved defines method to applicability preconditions and left these two behavior preserving preconditions. Also in this commit we renamed subclassesOf:referToSelector: to subclassesOf:isDoingASuperSendFor:.

This is what we have now with comments next to what we should add:

applicabilityPreconditions
	^ { ReDefinesSelectorsCondition new definesSelectors: selectors in: class }

breakingChangePreconditions

	^ OrderedCollection
          with: self preconditionIsAbstract
          with: self preconditionSuperclassAlreadyDefines "<-- missing, should add"
		  with: self preconditionSubclassesDontSendSuper

And this part is copy-paste mistake:

	refactoring failedApplicabilityPreconditions ifNotEmpty: [
		^ self inform: 'The shared variable does not exist' ].

we should fix it

[Ducasse]

Thanks balsa! @AlexisCnockaert will write some tests and fix it.