[Pharo-project] Classes that implement unsafe #=
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations: testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false." | wrongClasses | wrongClasses := IdentityDictionary new. ((SystemNavigation default allImplementorsOf: #=) collect: [:each | each methodClass]) do: [:each | | instance | "Some classes like CompiledMethod override basicNew to throw an error. In any case, the comparison will be false, so no problem" [instance := each basicNew] on: Error do: []. [instance = Object new] on: Error do: [: err | wrongClasses at: each put: err ]]. self assert: wrongClasses isEmpty The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot} So, my I think we should change #= in those classes by adding: self == aKeymap ifTrue: [ ^ true ]. self class = aKeymap class ifFalse: [ ^ false ]. at the beginning of the #= what do you think? if agree, I open an issue. -- Mariano http://marianopeck.wordpress.com
On 30.05.2012 14:05, Mariano Martinez Peck wrote:
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false." | wrongClasses | wrongClasses := IdentityDictionary new. ((SystemNavigation default allImplementorsOf: #=) collect: [:each | each methodClass]) do: [:each | | instance | "Some classes like CompiledMethod override basicNew to throw an error. In any case, the comparison will be false, so no problem" [instance := each basicNew] on: Error do: []. [instance = Object new] on: Error do: [: err | wrongClasses at: each put: err ]]. self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
self == aKeymap ifTrue: [ ^ true ]. self class = aKeymap class ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.com
I'd test against self species rather than self class, since it gives greater flexibility for user-defined instances to potentially = classes in the system. In other word, #class has a very strict meaning which would hinder extensibility, #species less so. Otherwise I agree. Cheers, Henry
On Wed, May 30, 2012 at 2:37 PM, Henrik Sperre Johansen < henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 14:05, Mariano Martinez Peck wrote:
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqu**alsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false." | wrongClasses | wrongClasses := IdentityDictionary new. ((SystemNavigation default allImplementorsOf: #=) collect: [:each | each methodClass]) do: [:each | | instance | "Some classes like CompiledMethod override basicNew to throw an error. In any case, the comparison will be false, so no problem" [instance := each basicNew] on: Error do: []. [instance = Object new] on: Error do: [: err | wrongClasses at: each put: err ]]. self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
self == aKeymap ifTrue: [ ^ true ]. self class = aKeymap class ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.**com <http://marianopeck.wordpress.com>
I'd test against self species rather than self class, since it gives greater flexibility for user-defined instances to potentially = classes in the system.
In other word, #class has a very strict meaning which would hinder extensibility, #species less so.
Ok, I see. But the classes are only a few (five). So you recommend you use self species instead of self class even if we "analyze" those 5 classes to see if it is worth it or not (for example, we check if they do override #species or not) ? and btw...the automatic #hash and #= generator, shouldn't use #species rather then #classes to? thanks
Otherwise I agree.
Cheers, Henry
-- Mariano http://marianopeck.wordpress.com
On 30.05.2012 15:02, Mariano Martinez Peck wrote:
On Wed, May 30, 2012 at 2:37 PM, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no <mailto:henrik.s.johansen@veloxit.no>> wrote:
On 30.05.2012 14:05, Mariano Martinez Peck wrote:
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false." | wrongClasses | wrongClasses := IdentityDictionary new. ((SystemNavigation default allImplementorsOf: #=) collect: [:each | each methodClass]) do: [:each | | instance | "Some classes like CompiledMethod override basicNew to throw an error. In any case, the comparison will be false, so no problem" [instance := each basicNew] on: Error do: []. [instance = Object new] on: Error do: [: err | wrongClasses at: each put: err ]]. self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
self == aKeymap ifTrue: [ ^ true ]. self class = aKeymap class ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.com
I'd test against self species rather than self class, since it gives greater flexibility for user-defined instances to potentially = classes in the system.
In other word, #class has a very strict meaning which would hinder extensibility, #species less so.
Ok, I see. But the classes are only a few (five). So you recommend you use self species instead of self class even if we "analyze" those 5 classes to see if it is worth it or not (for example, we check if they do override #species or not) ?
Yes. The default implementation of #species in Object uses the class primitive directly, so there's no overhead. The benefit is that I could write a custom MyMagnitude class, override species: species ^Magnitude and not have to override the base system Magnitude = implementation in order to compare with MyMagnitude instances and have a true result. In other words, using species in #= even if it's not needed, is setting a good example.
and btw...the automatic #hash and #= generator, shouldn't use #species rather then #classes to?
thanks
Yes, I've never used those :) Cheers, Henry
On Wed, May 30, 2012 at 3:32 PM, Henrik Sperre Johansen < henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 15:02, Mariano Martinez Peck wrote:
On Wed, May 30, 2012 at 2:37 PM, Henrik Sperre Johansen < henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 14:05, Mariano Martinez Peck wrote:
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false." | wrongClasses | wrongClasses := IdentityDictionary new. ((SystemNavigation default allImplementorsOf: #=) collect: [:each | each methodClass]) do: [:each | | instance | "Some classes like CompiledMethod override basicNew to throw an error. In any case, the comparison will be false, so no problem" [instance := each basicNew] on: Error do: []. [instance = Object new] on: Error do: [: err | wrongClasses at: each put: err ]]. self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
self == aKeymap ifTrue: [ ^ true ]. self class = aKeymap class ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.com
I'd test against self species rather than self class, since it gives greater flexibility for user-defined instances to potentially = classes in the system.
In other word, #class has a very strict meaning which would hinder extensibility, #species less so.
Ok, I see. But the classes are only a few (five). So you recommend you use self species instead of self class even if we "analyze" those 5 classes to see if it is worth it or not (for example, we check if they do override #species or not) ?
Yes. The default implementation of #species in Object uses the class primitive directly, so there's no overhead.
The benefit is that I could write a custom MyMagnitude class, override species: species ^Magnitude
and not have to override the base system Magnitude = implementation in order to compare with MyMagnitude instances and have a true result. In other words, using species in #= even if it's not needed, is setting a good example.
Ok, I will do that. Now, I would like to keep #testAllClassesImplementSafeEqualsMethod somewhere for the future. But I cannot find a good existing class/package for it? ideas?
and btw...the automatic #hash and #= generator, shouldn't use #species rather then #classes to?
thanks
Yes, I've never used those :)
Cheers, Henry
-- Mariano http://marianopeck.wordpress.com
Ok, I will do that. Now, I would like to keep #testAllClassesImplementSafeEqualsMethod somewhere for the future. But I cannot find a good existing class/package for it? ideas?
forget about the test since it is not 100% accurate. We still need human intervation. Anywya, I publish a slice with the fixes to those classes I think that need to be fixed. If anyone can review: http://code.google.com/p/pharo/issues/detail?id=5997 cheers
and btw...the automatic #hash and #= generator, shouldn't use #species rather then #classes to?
thanks
Yes, I've never used those :)
Cheers, Henry
-- Mariano http://marianopeck.wordpress.com
-- Mariano http://marianopeck.wordpress.com
Another flexible option could be to test it respondsTo: each attribute being used for comparison. On Wed, May 30, 2012 at 8:32 AM, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 15:02, Mariano Martinez Peck wrote:
On Wed, May 30, 2012 at 2:37 PM, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 14:05, Mariano Martinez Peck wrote:
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false."   | wrongClasses |   wrongClasses := IdentityDictionary  new.   ((SystemNavigation default allImplementorsOf: #=)       collect: [:each | each methodClass])       do: [:each |         | instance |         "Some classes like CompiledMethod override basicNew to throw an error.         In any case, the comparison will be false, so no problem"         [instance := each basicNew] on: Error do: [].         [instance = Object new]           on: Error           do: [: err | wrongClasses at: each put: err ]].   self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
  self == aKeymap     ifTrue: [ ^ true ].   self class = aKeymap class     ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? Â if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.com
I'd test against self species rather than self class, since it gives greater flexibility for user-defined instances to potentially = classes in the system.
In other word, #class has a very strict meaning which would hinder extensibility, #species less so.
Ok, I see. But the classes are only a few (five). So you recommend you use self species instead of self class even if we "analyze" those 5 classes to see if it is worth it or not (for example, we check if they do override #species or not) ?
Yes. The default implementation of #species in Object uses the class primitive directly, so there's no overhead.
The benefit is that I could write a custom MyMagnitude class, override species: species    ^Magnitude
and not have to override the base system Magnitude = implementation in order to compare with MyMagnitude instances and have a true result. In other words, using species in #= even if it's not needed, is setting a good example.
and btw...the automatic #hash and #= generator, shouldn't use #species rather then #classes to?
thanks
Yes, I've never used those :)
Cheers, Henry
On 31 May 2012 04:37, Chris Muller <asqueaker@gmail.com> wrote:
Another flexible option could be to test it respondsTo: each attribute being used for comparison.
This works quite well, and preserves "dynamic-ness": instead of checking for class membership or set-of-class membership (#species), you're saying "things that speak the same protocol/language might be equal, but definitely not if they don't speak the same language". That would permit non-transparent proxies to be #= to the things they proxy, for instance. (I use non-transparent proxies quite a lot, for instance as a means of providing an immutable pointer (Ref) to something _apparently_ immutable, but which I can change willy nilly without affecting anything pointing to the Ref.) frank
On Wed, May 30, 2012 at 8:32 AM, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 15:02, Mariano Martinez Peck wrote:
On Wed, May 30, 2012 at 2:37 PM, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 14:05, Mariano Martinez Peck wrote:
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false."   | wrongClasses |   wrongClasses := IdentityDictionary  new.   ((SystemNavigation default allImplementorsOf: #=)       collect: [:each | each methodClass])       do: [:each |         | instance |         "Some classes like CompiledMethod override basicNew to throw an error.         In any case, the comparison will be false, so no problem"         [instance := each basicNew] on: Error do: [].         [instance = Object new]           on: Error           do: [: err | wrongClasses at: each put: err ]].   self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
  self == aKeymap     ifTrue: [ ^ true ].   self class = aKeymap class     ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? Â if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.com
I'd test against self species rather than self class, since it gives greater flexibility for user-defined instances to potentially = classes in the system.
In other word, #class has a very strict meaning which would hinder extensibility, #species less so.
Ok, I see. But the classes are only a few (five). So you recommend you use self species instead of self class even if we "analyze" those 5 classes to see if it is worth it or not (for example, we check if they do override #species or not) ?
Yes. The default implementation of #species in Object uses the class primitive directly, so there's no overhead.
The benefit is that I could write a custom MyMagnitude class, override species: species    ^Magnitude
and not have to override the base system Magnitude = implementation in order to compare with MyMagnitude instances and have a true result. In other words, using species in #= even if it's not needed, is setting a good example.
and btw...the automatic #hash and #= generator, shouldn't use #species rather then #classes to?
thanks
Yes, I've never used those :)
Cheers, Henry
On Thu, May 31, 2012 at 5:37 AM, Chris Muller <asqueaker@gmail.com> wrote:
Another flexible option could be to test it respondsTo: each attribute being used for comparison.
Indeed, but on the other hand it could be slow ;)
On Wed, May 30, 2012 at 8:32 AM, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 15:02, Mariano Martinez Peck wrote:
On Wed, May 30, 2012 at 2:37 PM, Henrik Sperre Johansen <henrik.s.johansen@veloxit.no> wrote:
On 30.05.2012 14:05, Mariano Martinez Peck wrote:
Hi guys. I found a problem while working with FuelPreview where we
store
objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false." | wrongClasses | wrongClasses := IdentityDictionary new. ((SystemNavigation default allImplementorsOf: #=) collect: [:each | each methodClass]) do: [:each | | instance | "Some classes like CompiledMethod override basicNew to throw an error. In any case, the comparison will be false, so no problem" [instance := each basicNew] on: Error do: []. [instance = Object new] on: Error do: [: err | wrongClasses at: each put: err ]]. self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
self == aKeymap ifTrue: [ ^ true ]. self class = aKeymap class ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.com
I'd test against self species rather than self class, since it gives greater flexibility for user-defined instances to potentially = classes in the system.
In other word, #class has a very strict meaning which would hinder extensibility, #species less so.
Ok, I see. But the classes are only a few (five). So you recommend you use self species instead of self class even if we "analyze" those 5 classes to see if it is worth it or not (for example, we check if they do override #species or not) ?
Yes. The default implementation of #species in Object uses the class primitive directly, so there's no overhead.
The benefit is that I could write a custom MyMagnitude class, override species: species ^Magnitude
and not have to override the base system Magnitude = implementation in order to compare with MyMagnitude instances and have a true result. In other words, using species in #= even if it's not needed, is setting a good example.
and btw...the automatic #hash and #= generator, shouldn't use #species rather then #classes to?
thanks
Yes, I've never used those :)
Cheers, Henry
-- Mariano http://marianopeck.wordpress.com
Good catch Mariano -- I have run your test on Squeak and fixed a couple of instances there too. On Wed, May 30, 2012 at 7:05 AM, Mariano Martinez Peck <marianopeck@gmail.com> wrote:
Hi guys. I found a problem while working with FuelPreview where we store objects of the graph in a Set. While doing a collect, there was an error because an implementation of #= was assuming that the argument was of a certain shape. Of course, that should answer false instead. So I wrote this test to see all existing wrong implementations:
testAllClassesImplementSafeEqualsMethod "This tests that all classes in the system that implements #= do it in a way that they don't throw error when passing as an argument something different from expected. The correct behavior is that #= answers false."    | wrongClasses |    wrongClasses := IdentityDictionary new.     ((SystemNavigation default allImplementorsOf: #=)          collect: [:each | each methodClass])          do: [:each |             | instance |             "Some classes like CompiledMethod override basicNew to throw an error.             In any case, the comparison will be false, so no problem"             [instance := each basicNew] on: Error do: [].             [instance = Object new]                on: Error                do: [: err | wrongClasses at: each put: err ]].    self assert: wrongClasses isEmpty
The result is: {Magnitude. WideCharacterSet. KMKeymap. MCMockDefinition. ScaledDecimal. MCSnapshot}
So, my I think we should change #= in those classes by adding:
   self == aKeymap       ifTrue: [ ^ true ].    self class = aKeymap class       ifFalse: [ ^ false ].
at the beginning of the #=
what do you think? if agree, I open an issue.
-- Mariano http://marianopeck.wordpress.com
participants (4)
-
Chris Muller -
Frank Shearar -
Henrik Sperre Johansen -
Mariano Martinez Peck