From 4de5d95e7dec5913d1b5bb84a9fa8d99bb781e79 Mon Sep 17 00:00:00 2001 From: CyrilFerlicot Date: Sat, 9 May 2020 02:17:30 +0200 Subject: [PATCH 1/3] Add cleaner extracting return from conditions. Also disable Epicea to run tests Fixes #77 Fixes #94 --- resources/doc/documentation.md | 24 ++ .../ChanelAbstractCleanerTest.class.st | 5 + ...ifyUnclassifiedMethodsCleanerTest.class.st | 11 + ...tReturnFromAllBranchesCleanerTest.class.st | 285 ++++++++++++++++++ ...tractReturnFromAllBranchesCleaner.class.st | 33 ++ src/Chanel/RBBlockNode.extension.st | 6 + src/Chanel/RBMessageNode.extension.st | 12 + src/Chanel/RBReturnNode.extension.st | 5 + 8 files changed, 381 insertions(+) create mode 100644 src/Chanel-Tests/ChanelExtractReturnFromAllBranchesCleanerTest.class.st create mode 100644 src/Chanel/ChanelExtractReturnFromAllBranchesCleaner.class.st create mode 100644 src/Chanel/RBBlockNode.extension.st diff --git a/resources/doc/documentation.md b/resources/doc/documentation.md index 8d080c4..e58ebc6 100644 --- a/resources/doc/documentation.md +++ b/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. + diff --git a/src/Chanel-Tests/ChanelAbstractCleanerTest.class.st b/src/Chanel-Tests/ChanelAbstractCleanerTest.class.st index af3b12b..5d8bc1b 100644 --- a/src/Chanel-Tests/ChanelAbstractCleanerTest.class.st +++ b/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} diff --git a/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st b/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st index 8274499..3fff33d 100644 --- a/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st +++ b/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st @@ -76,3 +76,14 @@ ChanelClassifyUnclassifiedMethodsCleanerTest >> testDontCategorizeExtensionMetho self assert: (class >> #initialize) protocol equals: self extensionProtocol ] + +{ #category : #tests } +ChanelClassifyUnclassifiedMethodsCleanerTest >> testRemoveUselessBlock [ + class compile: 'method + self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]'. + + self runCleaner. + + self assert: (class >> #method) sourceCode equals: 'method + ^1' +] diff --git a/src/Chanel-Tests/ChanelExtractReturnFromAllBranchesCleanerTest.class.st b/src/Chanel-Tests/ChanelExtractReturnFromAllBranchesCleanerTest.class.st new file mode 100644 index 0000000..dafbf86 --- /dev/null +++ b/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 ]' +] diff --git a/src/Chanel/ChanelExtractReturnFromAllBranchesCleaner.class.st b/src/Chanel/ChanelExtractReturnFromAllBranchesCleaner.class.st new file mode 100644 index 0000000..754ab6f --- /dev/null +++ b/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 +] diff --git a/src/Chanel/RBBlockNode.extension.st b/src/Chanel/RBBlockNode.extension.st new file mode 100644 index 0000000..1d1616d --- /dev/null +++ b/src/Chanel/RBBlockNode.extension.st @@ -0,0 +1,6 @@ +Extension { #name : #RBBlockNode } + +{ #category : #'*Chanel' } +RBBlockNode >> inlineLastReturn [ + self statements last inline +] diff --git a/src/Chanel/RBMessageNode.extension.st b/src/Chanel/RBMessageNode.extension.st index eee30d2..5f74459 100644 --- a/src/Chanel/RBMessageNode.extension.st +++ b/src/Chanel/RBMessageNode.extension.st @@ -5,6 +5,11 @@ RBMessageNode >> canHaveUselessChildren [ ^ false ] +{ #category : #'*Chanel' } +RBMessageNode >> isConditionNecessarilyExecutingABranch [ + ^ #(#ifTrue:ifFalse: #ifFalse:ifTrue: #ifNil:ifNotNil: #ifNotNil:ifNil: #ifEmpty:ifNotEmpty: #ifNotEmpty:ifEmpty: #ifExists:ifAbsent:) includes: self selector +] + { #category : #'*Chanel' } RBMessageNode >> isSelfSendTo: aSelector [ ^ self isSelfSend and: [ self selector = aSelector ] @@ -19,3 +24,10 @@ RBMessageNode >> isSuperSendTo: aSelector [ RBMessageNode class >> superSendTo: aSelector [ ^ self receiver: (RBVariableNode named: #super) selector: aSelector ] + +{ #category : #'*Chanel' } +RBMessageNode >> wrapsInReturn [ + self parent isReturn ifTrue: [ ^ self ]. + + self replaceWith: (RBReturnNode value: self copy) +] diff --git a/src/Chanel/RBReturnNode.extension.st b/src/Chanel/RBReturnNode.extension.st index 287c88f..a65a474 100644 --- a/src/Chanel/RBReturnNode.extension.st +++ b/src/Chanel/RBReturnNode.extension.st @@ -4,3 +4,8 @@ Extension { #name : #RBReturnNode } RBReturnNode >> canHaveUselessChildren [ ^ false ] + +{ #category : #'*Chanel' } +RBReturnNode >> inline [ + self replaceWith: self value +] From 911063e1e2ed8d6284ecc3ee67e339acbaf9090e Mon Sep 17 00:00:00 2001 From: CyrilFerlicot Date: Sat, 9 May 2020 02:27:14 +0200 Subject: [PATCH 2/3] Add PharoBackwardCompatibility to P7 --- src/BaselineOfChanel/BaselineOfChanel.class.st | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/BaselineOfChanel/BaselineOfChanel.class.st b/src/BaselineOfChanel/BaselineOfChanel.class.st index 962d10b..b0add51 100644 --- a/src/BaselineOfChanel/BaselineOfChanel.class.st +++ b/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 From bfdb99d93b804d7c856792ae39b1605b26592494 Mon Sep 17 00:00:00 2001 From: CyrilFerlicot Date: Sat, 9 May 2020 02:28:26 +0200 Subject: [PATCH 3/3] Remove method that should not be here. This method was adding when I was playing around. --- ...nelClassifyUnclassifiedMethodsCleanerTest.class.st | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st b/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st index 3fff33d..8274499 100644 --- a/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st +++ b/src/Chanel-Tests/ChanelClassifyUnclassifiedMethodsCleanerTest.class.st @@ -76,14 +76,3 @@ ChanelClassifyUnclassifiedMethodsCleanerTest >> testDontCategorizeExtensionMetho self assert: (class >> #initialize) protocol equals: self extensionProtocol ] - -{ #category : #tests } -ChanelClassifyUnclassifiedMethodsCleanerTest >> testRemoveUselessBlock [ - class compile: 'method - self toto ifTrue: [ ^ 1 ] ifFalse: [ ^ 2 ]'. - - self runCleaner. - - self assert: (class >> #method) sourceCode equals: 'method - ^1' -]