Hi guys
We got a really strange problem in the classBuilder code. Here is the story
Context =======
When the method name: className inEnvironment: env subclassOf: newSuper type: type instanceVariableNames: instVarString classVariableNames: classVarString poolDictionaries: poolString category: category unsafe: unsafe is redefined as follow. It works and fixes some bugs because Behavior was not flushed correctly.
All the tests are green for example ClassBuilderFormatTests>> testByteVariableSubclass
in particular self should:[self makeIVarsSubclassOf: baseClass] raise: Error. works ie the exception is raised.
name: className inEnvironment: env subclassOf: newSuper type: type instanceVariableNames: instVarString classVariableNames: classVarString poolDictionaries: poolString category: category unsafe: unsafe "Define a new class in the given environment. If unsafe is true do not run any validation checks. This facility is provided to implement important system changes." | oldClass newClass organization instVars classVars force needNew oldCategory copyOfOldClass newCategory | environ _ env. instVars _ Scanner new scanFieldNames: instVarString. classVars _ (Scanner new scanFieldNames: classVarString) collect: [:x | x asSymbol].
"Validate the proposed name" unsafe ifFalse:[(self validateClassName: className) ifFalse: [^nil]]. oldClass _ env at: className ifAbsent:[nil]. oldClass isBehavior ifFalse: [oldClass _ nil] "Already checked in #validateClassName:" ifTrue: [ copyOfOldClass _ oldClass copy. copyOfOldClass superclass addSubclass: copyOfOldClass].
[unsafe ifFalse:[ "Run validation checks so we know that we have a good chance for recompilation" (self validateSuperclass: newSuper forSubclass: oldClass) ifFalse:[^nil]. (self validateInstvars: instVars from: oldClass forSuper: newSuper) ifFalse:[^nil]. (self validateClassvars: classVars from: oldClass forSuper: newSuper) ifFalse:[^nil]. (self validateSubclassFormat: type from: oldClass forSuper: newSuper extra: instVars size) ifFalse:[^nil]]. "self halt: 'after validate'." "See if we need a new subclass" needNew _ self needsSubclassOf: newSuper type: type instanceVariables: instVars from: oldClass. needNew == nil ifTrue:[^nil]. "some error"
(needNew and:[unsafe not]) ifTrue:[ "Make sure we don't redefine any dangerous classes" (self tooDangerousClasses includes: oldClass name) ifTrue:[ self error: oldClass name, ' cannot be changed'. ]. "Check if the receiver should not be redefined" (oldClass ~~ nil and:[oldClass shouldNotBeRedefined]) ifTrue:[ self notify: oldClass name asText allBold, ' should not be redefined. \Proceed to store over it.' withCRs]]. "self halt." needNew ifTrue:[ "Create the new class"
newClass _ self newSubclassOf: newSuper type: type instanceVariables: instVars from: oldClass. newClass == nil ifTrue:[^nil]. "Some error" newClass setName: className. ] ifFalse:[ "Reuse the old class" newClass _ oldClass. ].
"Install the class variables and pool dictionaries... " force _ (newClass declare: classVarString) | (newClass sharing: poolString).
"... classify ..." newCategory _ category asSymbol. organization _ environ ifNotNil:[environ organization]. oldClass isNil ifFalse: [oldCategory _ (organization categoryOfElement: oldClass name) asSymbol]. organization classify: newClass name under: newCategory. newClass environment: environ.
"... recompile ..." newClass _ self recompile: force from: oldClass to: newClass mutate: false.
"... export if not yet done ..." (environ at: newClass name ifAbsent:[nil]) == newClass ifFalse:[ [environ at: newClass name put: newClass] on: AttemptToWriteReadOnlyGlobal do:[:ex| ex resume: true]. Smalltalk flushClassNameCache. ].
newClass doneCompiling. "... notify interested clients ..." oldClass isNil ifTrue: [ SystemChangeNotifier uniqueInstance classAdded: newClass inCategory: newCategory. ^ newClass]. SystemChangeNotifier uniqueInstance classDefinitionChangedFrom: copyOfOldClass to: newClass. newCategory ~= oldCategory ifTrue: [SystemChangeNotifier uniqueInstance class: newClass recategorizedFrom: oldCategory to: category]. ] ensure: [copyOfOldClass ifNotNil: [copyOfOldClass superclass removeSubclass: copyOfOldClass]. Behavior flushObsoleteSubclasses.]. "apparently when the following expression is inside the ensure block the exception is not propagated. One tiny examples we could not reproduce this behavior." ^newClass
Problem: ======
Now if you move the ^newClass into the ensure block as
ensure: [copyOfOldClass ifNotNil: [copyOfOldClass superclass removeSubclass: copyOfOldClass]. Behavior flushObsoleteSubclasses. ^newClass ].
The test does not work self should:[self makeIVarsSubclassOf: baseClass] raise: Error. works ie the exception is not raised.
So we check that ensure: propagate exception as shown below
testsOne "self debug: #testsOne"
self should: [ [1 foo] ensure: [^1]] raise: MessageNotUnderstood
testsTwo "self debug: #testsTwo"
self should: [ [1 error: 'hello'] ensure: [^1]] raise: Error. self should: [ [1+2 ] ensure: [1 error: 'hello']] raise: Error
So we do not really understand.
Now our fixes in the classBuilder works but we are perplexed and not satisfied not to understand.
Stef and adrian
Now if you move the ^newClass into the ensure block as
As I just happen to have worked with Smalllint I know that you should never put a return into a block being the argument of #ensure:, #valueNowOrOnUnwindDo:, #ifCurtailed: or #valueOnUnwindDo:. There is a Smalllint-Rule "Contains a return in an ensure: block".
I think your test get never really executed, because you quit them with ^1 before SUnit is actually able to check something in #should:raise ... The ^1 will quit your method immediately, and it will executed all the time because it is within the ensure.
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
On 28 oct. 05, at 15:45, Lukas Renggli wrote:
Now if you move the ^newClass into the ensure block as
As I just happen to have worked with Smalllint I know that you should never put a return into a block being the argument of #ensure:, #valueNowOrOnUnwindDo:, #ifCurtailed: or #valueOnUnwindDo:. There is a Smalllint-Rule "Contains a return in an ensure: block".
I think your test get never really executed, because you quit them with ^1 before SUnit is actually able to check something in #should:raise ... The ^1 will quit your method immediately, and it will executed all the time because it is within the ensure.
cool. Thanks for that feedback. I should write a better test then.
Stef
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
I was stupid.... Now I'm reading the ExceptionTest.... this is excellent! I'm learning a lot just reading these tests! Thanks andreas :)
I saw that doubleOuterTest does not have a doubleOuterTestResults
Stef
Now if you move the ^newClass into the ensure block as
As I just happen to have worked with Smalllint I know that you should never put a return into a block being the argument of #ensure:, #valueNowOrOnUnwindDo:, #ifCurtailed: or #valueOnUnwindDo:. There is a Smalllint-Rule "Contains a return in an ensure: block".
I think your test get never really executed, because you quit them with ^1 before SUnit is actually able to check something in #should:raise ... The ^1 will quit your method immediately, and it will executed all the time because it is within the ensure.
cool. Thanks for that feedback. I should write a better test then.
Stef
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
squeak-dev@lists.squeakfoundation.org