Merge pull request #95 from jecisc/77-Add-cleaner-extracting-returns-…

…for-conditionals-with-return-as-last-statement-in-the-two-branches

77-Add-cleaner-extracting-returns-for-conditionals-with-return-as-last-statement-in-the-two-branches

resources/doc/documentation.md

@@ -564,3 +564,27 @@ into:
 *Warnings:*
 This cleaner can be dangerous if you have classes implementing #`isEmpty` without #`isNotEmpty` for example.
 
+### Extract return from conditionals
+
+In case a conditional ends with a return in all its branches, `Chanel` tries to extract it.
+
+List of supported conditionals:
+- `#ifTrue:ifFalse:` 
+- `#ifFalse:ifTrue:` 
+- `#ifNil:ifNotNil:` 
+- `#ifNotNil:ifNil:` 
+- `#ifEmpty:ifNotEmpty:` 
+- `#ifNotEmpty:ifEmpty:`
+-  `#ifExists:ifAbsent:`
+
+*Conditions for the cleanings to by applied:*
+- The AST node is a message.
+- The node is not in a cascade.
+- The node is the last node of the parent (if not, it would not be possible to compile a return at this place).
+- The node selector matches a selector above.
+- All arguments are blocks (none should be symbol).
+- All arguments should have a return as last statement.
+
+*Warnings:*
+This cleaning should not have any counter indication.
+

src/BaselineOfChanel/BaselineOfChanel.class.st

@@ -26,9 +26,10 @@ BaselineOfChanel >> baseline: spec [
 
 	spec
 		for: #(#'pharo7.x')
-		do: [ spec
+		do: [ self pharoBackwardCompatibility: spec.
+			spec
 				package: 'Chanel-Pharo7';
-				package: 'Chanel-Tests' with: [ spec requires: 'Chanel-Pharo7' ] ]
+				package: 'Chanel-Tests' with: [ spec requires: #('Chanel-Pharo7' 'PharoBackwardCompatibility') ] ]
 ]
 
 { #category : #dependencies }
@@ -40,6 +41,13 @@ BaselineOfChanel >> iterators: spec [
 				repository: 'github://juliendelplanque/Iterators:v1.x.x/src' ]
 ]
 
+{ #category : #dependencies }
+BaselineOfChanel >> pharoBackwardCompatibility: spec [
+   spec
+    	baseline: 'PharoBackwardCompatibility'
+    	with: [ spec repository: 'github://jecisc/PharoBackwardCompatibility:v1.x.x/src' ]
+]
+
 { #category : #accessing }
 BaselineOfChanel >> projectClass [
 	^ MetacelloCypressBaselineProject

src/Chanel-Tests/ChanelAbstractCleanerTest.class.st

@@ -97,6 +97,11 @@ ChanelAbstractCleanerTest >> methodBodyFor: aString [
   {2}' format: {self selector . aString}
 ]
 
+{ #category : #running }
+ChanelAbstractCleanerTest >> runCase [
+	EpMonitor disableDuring: [ super runCase ]
+]
+
 { #category : #running }
 ChanelAbstractCleanerTest >> runCleaner [
 	Chanel perfume: {package} using: {self actualClass}

src/Chanel-Tests/ChanelExtractReturnFromAllBranchesCleanerTest.class.st

@@ -0,0 +1,285 @@
+"
+A ChanelExtractReturnFromAllBranchesCleanerTest is a test class for testing the behavior of ChanelExtractReturnFromAllBranchesCleaner
+"
+Class {
+	#name : #ChanelExtractReturnFromAllBranchesCleanerTest,
+	#superclass : #ChanelAbstractCleanerTest,
+	#category : #'Chanel-Tests'
+}
+
+{ #category : #running }
+ChanelExtractReturnFromAllBranchesCleanerTest >> setUp [
+	super setUp.
+	class := self createDefaultClass
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testDoesNotExtractReturnIfABranchIsASymbol [
+	| oldMethod |
+	class
+		compile:
+			'method
+  self toto ifNil: [ ^ 1 ] ifNotNil: #yourself'.
+
+	oldMethod := class >> #method.
+
+	self runCleaner.
+
+	self
+		assert: (class >> #method) sourceCode
+		equals:
+			'method
+  self toto ifNil: [ ^ 1 ] ifNotNil: #yourself'.
+
+	self assert: oldMethod identicalTo: class >> #method
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testDoesNotExtractReturnIfAlreadyOutside [
+	| oldMethod |
+	class
+		compile:
+			'method
+  ^ self toto ifTrue: [ 1 ] ifFalse: [ 2 ]'.
+
+	oldMethod := class >> #method.
+
+	self runCleaner.
+
+	self
+		assert: (class >> #method) sourceCode
+		equals:
+			'method
+  ^ self toto ifTrue: [ 1 ] ifFalse: [ 2 ]'.
+
+	self assert: oldMethod identicalTo: class >> #method
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testDoesNotExtractReturnIfInACascade [
+	| oldMethod |
+	class compile: 'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]; bar'.
+
+	oldMethod := class >> #method.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]; bar'.
+
+	self assert: oldMethod identicalTo: class >> #method
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testDoesNotExtractReturnIfMessageIsNotTheLastStatement [
+	| oldMethod |
+	class
+		compile:
+			'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ].
+  self bar'.
+
+	oldMethod := class >> #method.
+
+	self runCleaner.
+
+	self
+		assert: (class >> #method) sourceCode
+		equals:
+			'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ].
+  self bar'.
+
+	self assert: oldMethod identicalTo: class >> #method
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testDoesNotExtractReturnIfOnBranchDoesNotHaveOne [
+	| oldMethod |
+	class
+		compile:
+			'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ 2 ]'.
+
+	oldMethod := class >> #method.
+
+	self runCleaner.
+
+	self
+		assert: (class >> #method) sourceCode
+		equals:
+			'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ 2 ]'.
+
+	self assert: oldMethod identicalTo: class >> #method
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testDoesNotExtractReturnIfReturnIsNotTheLastStatement [
+	| oldMethod |
+	class
+		compile:
+			'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ self foo ifTrue: [ ^ 2 ] ]'.
+
+	oldMethod := class >> #method.
+
+	self runCleaner.
+
+	self
+		assert: (class >> #method) sourceCode
+		equals:
+			'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ self foo ifTrue: [ ^ 2 ] ]'.
+
+	self assert: oldMethod identicalTo: class >> #method
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnDoesNotFailIfThereIsAlreadyAReturn [
+	class compile: 'method
+  ^self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifTrue: [ 1 ] ifFalse: [ 2 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfEmptyIfNotEmpty [
+	class compile: 'method
+  self toto ifEmpty: [ ^ 2 ] ifNotEmpty: [ ^ 1 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifEmpty: [ 2 ] ifNotEmpty: [ 1 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfExistsIfAbsent [
+	class compile: 'method
+  self toto ifExists: [ ^ 2 ] ifAbsent: [ ^ 1 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifExists: [ 2 ] ifAbsent: [ 1 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfFalseIfTrue [
+	class compile: 'method
+  self toto ifFalse: [ ^ 2 ] ifTrue: [ ^ 1 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifFalse: [ 2 ] ifTrue: [ 1 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfNilIfNotNil [
+	class compile: 'method
+  self toto ifNil: [ ^ 2 ] ifNotNil: [ ^ 1 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifNil: [ 2 ] ifNotNil: [ 1 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfNotEmptyIfEmpty [
+	class compile: 'method
+  self toto ifNotEmpty: [ ^ 2 ] ifEmpty: [ ^ 1 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifNotEmpty: [ 2 ] ifEmpty: [ 1 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfNotNilIfNil [
+	class compile: 'method
+  self toto ifNotNil: [ ^ 2 ] ifNil: [ ^ 1 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifNotNil: [ 2 ] ifNil: [ 1 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfTrueIfFalse [
+	class compile: 'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifTrue: [ 1 ] ifFalse: [ 2 ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnFromIfTrueIfFalseWithOtherStatements [
+	class compile: 'method
+  self toto ifTrue: [ self foo. ^ 1 ] ifFalse: [ ^ self bar ]'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  ^self toto ifTrue: [ self foo.
+        1 ] ifFalse: [ self bar ]'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnIfTheLastStatementOfABlock [
+	class compile: 'method
+  self foo ifTrue: [ self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ] ].
+  self bar'.
+
+	self runCleaner.
+
+	self assert: (class >> #method) sourceCode equals: 'method
+  self foo ifTrue: [ ^self toto ifTrue: [ 1 ] ifFalse: [ 2 ] ].
+  self bar'
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnInTrait [
+	| trait |
+	trait := self createDefaultTrait.
+
+	class setTraitComposition: trait.
+
+	trait
+		compile:
+			'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]'.
+
+	self runCleaner.
+
+	self
+		assert: (trait >> #method) sourceCode
+		equals:
+			'method
+  ^self toto ifTrue: [ 1 ] ifFalse: [ 2 ]'.
+
+	self assert: (trait localSelectors includes: #method).
+	self deny: (class localSelectors includes: #method)
+]
+
+{ #category : #tests }
+ChanelExtractReturnFromAllBranchesCleanerTest >> testExtractReturnOnClassSide [
+	class class compile: 'method
+  self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]'.
+
+	self runCleaner.
+
+	self assert: (class class >> #method) sourceCode equals: 'method
+  ^self toto ifTrue: [ 1 ] ifFalse: [ 2 ]'
+]

src/Chanel/ChanelExtractReturnFromAllBranchesCleaner.class.st

@@ -0,0 +1,33 @@
+"
+Description
+--------------------
+
+I am a cleaner trying to extract returns from conditions.
+"
+Class {
+	#name : #ChanelExtractReturnFromAllBranchesCleaner,
+	#superclass : #ChanelAbstractCleaner,
+	#category : #Chanel
+}
+
+{ #category : #accessing }
+ChanelExtractReturnFromAllBranchesCleaner class >> priority [
+	^ 15000
+]
+
+{ #category : #cleaning }
+ChanelExtractReturnFromAllBranchesCleaner >> clean [
+	(self configuration localMethods iterator
+		| #ast collectIt
+		| #allChildren flatCollectIt
+		| #isMessage selectIt
+		| #isCascaded rejectIt
+		| [ :node | node parent isLast: node ] selectIt
+		| #isConditionNecessarilyExecutingABranch selectIt
+		| [ :node | node arguments allSatisfy: #isBlock ] selectIt
+		| [ :node | node arguments allSatisfy: #hasBlockReturn ] selectIt
+		| [ :node | node arguments do: #inlineLastReturn ] doIt
+		| #wrapsInReturn doIt
+		| #methodNode collectIt
+		> Set) do: #install
+]

src/Chanel/RBBlockNode.extension.st

@@ -0,0 +1,6 @@
+Extension { #name : #RBBlockNode }
+
+{ #category : #'*Chanel' }
+RBBlockNode >> inlineLastReturn [
+	self statements last inline
+]