Bug in #instVarAt:put: for read-only objects

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Bug in #instVarAt:put: for read-only objects

NorbertHartl
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Clément Béra
 
Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Eliot Miranda-2
 
Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.

_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Clément Béra
 


On Fri, Jan 5, 2018 at 2:34 PM, Eliot Miranda <[hidden email]> wrote:
 
Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.


That was discussed on the Pharo mailing list. 

I used the same name and it was changed by other contributors.

 
_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

NorbertHartl
In reply to this post by Clément Béra
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Clément Béra
 


On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <[hidden email]> wrote:
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

yes something like that.


Same thing for Object>>#at:put:, floatAt:put: and so on.
 

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Eliot Miranda-2
 
Hi Norbert,


On Jan 5, 2018, at 7:53 AM, Clément Bera <[hidden email]> wrote:



On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <[hidden email]> wrote:
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

The index and bounds validation should also occur, probably first.  So something like

Object>>instVarAt: index put: anObject
    <primitive: 174 error: ec>
    (index isInteger
    and: [index between: 1 and: self class instSize + self basicSize]) ifFalse:
        [self badIndexError: index].
    self isReadOnlyObject ifTrue:
         [^(NoModificationError for: self atInstVar: index with: anObject) signal].
    self primitiveFailed


yes something like that.


Same thing for Object>>#at:put:, floatAt:put: and so on.
 

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Eliot Miranda-2
In reply to this post by Clément Béra
 
Hi Clément,

On Jan 5, 2018, at 5:50 AM, Clément Bera <[hidden email]> wrote:



On Fri, Jan 5, 2018 at 2:34 PM, Eliot Miranda <[hidden email]> wrote:
 
Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.


That was discussed on the Pharo mailing list. 

I used the same name and it was changed by other contributors.

Those contributors made an assertion nine and arrogant decision.  I encourage you too override them and maintaining n should me kind of compatibility for the benefit of Gemstone.  Working well with Gemstone could benefit greatly the Pharo community.  This refusal to learn or follow the experience of others will not serve the country mum it's well in the long term.  It is arrogant and exclusionary.


 
_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

timrowledge
In reply to this post by Eliot Miranda-2
 

> On 05-01-2018, at 9:17 AM, Eliot Miranda <[hidden email]> wrote:
>
>
> The index and bounds validation should also occur, probably first.  So something like
>
> Object>>instVarAt: index put: anObject
>     <primitive: 174 error: ec>
>     (index isInteger
>     and: [index between: 1 and: self class instSize + self basicSize]) ifFalse:
>         [self badIndexError: index].
>     self isReadOnlyObject ifTrue:
>          [^(NoModificationError for: self atInstVar: index with: anObject) signal].
>     self primitiveFailed

Surely the no-modification check ought to come first since the index is completely irrelevant in that case?

And obviously we ought to be doing cleverer things with the returned error codes; we could avoid the all the tests here since the prim already provides error codes for bad argument (the index isInteger test), bad index (index bounds) and bad rcvr (immutability - though might we not want a specific error here instead?) I know, I know; somebody has to find the time to do the work.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Never forget: 2 + 2 = 5 for extremely large values of 2.


Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Eliot Miranda-2
 
Hi Tim,


> On Jan 5, 2018, at 11:45 AM, tim Rowledge <[hidden email]> wrote:
>
>
>
>> On 05-01-2018, at 9:17 AM, Eliot Miranda <[hidden email]> wrote:
>>
>>
>> The index and bounds validation should also occur, probably first.  So something like
>>
>> Object>>instVarAt: index put: anObject
>>    <primitive: 174 error: ec>
>>    (index isInteger
>>    and: [index between: 1 and: self class instSize + self basicSize]) ifFalse:
>>        [self badIndexError: index].
>>    self isReadOnlyObject ifTrue:
>>         [^(NoModificationError for: self atInstVar: index with: anObject) signal].
>>    self primitiveFailed
>
> Surely the no-modification check ought to come first since the index is completely irrelevant in that case?

No.  That would mean that an attempted out-of-bounds access on a read-only object would raise the no modification error before the bounds violation which could cause a plausible implementation to turn the object into write mode but not be able that complete the write.

>
> And obviously we ought to be doing cleverer things with the returned error codes; we could avoid the all the tests here since the prim already provides error codes for bad argument (the index isInteger test), bad index (index bounds) and bad rcvr (immutability - though might we not want a specific error here instead?) I know, I know; somebody has to find the time to do the work.

:-)

>
> tim
> --
> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
> Never forget: 2 + 2 = 5 for extremely large values of 2.
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

NorbertHartl
In reply to this post by Clément Béra
 


Am 05.01.2018 um 16:53 schrieb Clément Bera <[hidden email]>:



On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <[hidden email]> wrote:
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

yes something like that.


Same thing for Object>>#at:put:, floatAt:put: and so on.

I’m working on it and came by

Object>>#primitiveChangeClassTo:

and I wonder what would be the approach here. The ModificationForbidden/NoModificationError seems to be tight to an index. What would be the case for a class change? Another Exception class? How is that solved in VW? 

Norbert

 

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

NorbertHartl
In reply to this post by Eliot Miranda-2
 
Btw. why is the name NoModificationError? Isn’t the purpose of an Error that it is not resumable? And is NoModificationError very likely to be resumed? Like in #retryModification?

Norbert

Am 05.01.2018 um 14:34 schrieb Eliot Miranda <[hidden email]>:

Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.

_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Denis Kudriashov
In reply to this post by NorbertHartl
 
Hi

2018-01-06 13:06 GMT+01:00 Norbert Hartl <[hidden email]>:
Am 05.01.2018 um 16:53 schrieb Clément Bera <[hidden email]>:
On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <[hidden email]> wrote:
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

yes something like that.


Same thing for Object>>#at:put:, floatAt:put: and so on.

I’m working on it and came by

Object>>#primitiveChangeClassTo:

and I wonder what would be the approach here. The ModificationForbidden/NoModificationError seems to be tight to an index. What would be the case for a class change? Another Exception class?

In that case I would introduce hierarchy:
ModificationForbidden
   StateModificationForbidden
   ClassModificationForbidden 

How is that solved in VW? 

Norbert

 

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq



Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Clément Béra
In reply to this post by NorbertHartl
 


On Sat, Jan 6, 2018 at 1:32 PM, Norbert Hartl <[hidden email]> wrote:
 
Btw. why is the name NoModificationError? Isn’t the purpose of an Error that it is not resumable? And is NoModificationError very likely to be resumed? Like in #retryModification?

Normally the NoModificationError should be able to retry the modification, and then resumes execution just after the primitive call / the inst var store. 

In the case of primitive failure I guess either the error should resume with the value to return or the primitive fall-back code should return the correct value. In the case of inst var store, we need to resume execution using #jump (See #attemptToAssign: value withIndex: index)
 
For at:put: we could have:

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [^ (ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]

OR 

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal. ^ anObject]
ifFalse: [ self primitiveFailed ]

+ the index bound check mentioned by Eliot and asInteger trick (based on at: primitive failure code)

So that when the exception resumes the correct behavior happens.


Norbert

Am 05.01.2018 um 14:34 schrieb Eliot Miranda <[hidden email]>:

Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.

_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

NorbertHartl
 
Thanks Clement!

Am 06.01.2018 um 14:44 schrieb Clément Bera <[hidden email]>:



On Sat, Jan 6, 2018 at 1:32 PM, Norbert Hartl <[hidden email]> wrote:
 
Btw. why is the name NoModificationError? Isn’t the purpose of an Error that it is not resumable? And is NoModificationError very likely to be resumed? Like in #retryModification?

Normally the NoModificationError should be able to retry the modification, and then resumes execution just after the primitive call / the inst var store. 

I was actually asking why the mentioned NoModificationError is an error and not a NoModificationException. 

In the case of primitive failure I guess either the error should resume with the value to return or the primitive fall-back code should return the correct value. In the case of inst var store, we need to resume execution using #jump (See #attemptToAssign: value withIndex: index)
 
For at:put: we could have:

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [^ (ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]

OR 

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal. ^ anObject]
ifFalse: [ self primitiveFailed ]

+ the index bound check mentioned by Eliot and asInteger trick (based on at: primitive failure code)

So that when the exception resumes the correct behavior happens.

I have now

at: index put: value 
"Primitive. Assumes receiver is indexable. Store the argument value in 
the indexable element of the receiver indicated by index. Fail if the 
index is not an Integer or is out of bounds. Or fail if the value is not of 
the right type for this kind of collection. Answer the value that was 
stored. Essential. See Object documentation whatIsAPrimitive."

<primitive: 61>
index isInteger 
ifTrue: [self class isVariable
ifTrue: [(index between: 1 and: self size)
ifFalse: [self errorSubscriptBounds: index]]
ifFalse: [self errorNotIndexable]]
ifFalse: [  
index isNumber
ifTrue: [^self at: index asInteger put: value]
ifFalse: [self errorNonIntegerIndex] ].
self isReadOnlyObject 
ifTrue: [ ^ (InstVarModificationForbidden for: self atInstVar: index with: value) signal ].

Norbert


Norbert

Am 05.01.2018 um 14:34 schrieb Eliot Miranda <[hidden email]>:

Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.

_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Eliot Miranda-2
In reply to this post by NorbertHartl
 
Hi Norbert,

On Sat, Jan 6, 2018 at 4:32 AM, Norbert Hartl <[hidden email]> wrote:
 
Btw. why is the name NoModificationError? Isn’t the purpose of an Error that it is not resumable? And is NoModificationError very likely to be resumed? Like in #retryModification?

At least for me, errors are orthogonal to reusability.  In fact, that so many errors are not resumable is extremely annoying.  Take the example of reading a directory.  If one doesn't have the permissions to read a directory then attempting to do so is definitely an error.  But if one is trying to write a find-like utility that searches a directory hierarchy then one definitely wants to resume from such an error by answering empty contents.  Otherwise one's search terminates at the first unreadable directory.  So very much,reusability is about an operation's results and continuation, not about it being an error or a warning.

I don't think that NoModificationErrora are very likely to be resumed.  It depends on context.  If one were to try and modify a literal then one would not expect that error to be resumed.  If one is using a, object-to-database mapping scheme then yes, one would expect it to be resumed.  But it depends on what the application is doing, and so I don't think one can say anything about likelihood.

The name is NoModificationError since that's exactly what it is.  The per-object read-only bit stipulates that the object is not modifiable and any attempt to modify its state (which includes what its class is) causes an error.  It's not a NoModificationWarning.  It's a hard error. And in the case of a literal (or any other object we might want one not to be able to modify inadvertently such as a method) we would not expect any kind of resumption.

I hope this is clearer :-)


Norbert

Am 05.01.2018 um 14:34 schrieb Eliot Miranda <[hidden email]>:

Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.

_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




--
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





--
_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Eliot Miranda-2
In reply to this post by NorbertHartl
 
Hi Norbert, Hi Denis,

On Sat, Jan 6, 2018 at 4:06 AM, Norbert Hartl <[hidden email]> wrote:
 


Am 05.01.2018 um 16:53 schrieb Clément Bera <[hidden email]>:



On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <[hidden email]> wrote:
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

yes something like that.


Same thing for Object>>#at:put:, floatAt:put: and so on.

I’m working on it and came by

Object>>#primitiveChangeClassTo:

and I wonder what would be the approach here. The ModificationForbidden/NoModificationError seems to be tight to an index. What would be the case for a class change? Another Exception class? How is that solved in VW? 

I don't think that adding variants of NoModificationError is wise.  One can imagine adding other state changes that read-onliness should prevent, such as unpinning.  So one could end up having to add a lot of subclasses of NoModificationError, each rather inflexible and able to respond to only one specific error.  We would end up with at least InstanceVariableModificationError IndexedVariableModificationError ClassModificationError BecomeIdentotyChangeError etc.  And even then IndexedVariableModificationError would need a message since there are many different kinds of at:put:s, at:put:, byteAt:put: unsignedLongAt:put: etc.

In VW it is handled by the NoModificationError holding a Message that can be used for the retry operation.  i.e.

Object>>noModificationErrorFor: selector index: index value: value
^(NoModificationError receiver: self selector: selector index: index value: value) raiseRequest

(N.B. the following is sent by the VM when an inst var assignment fails due to read-onlyness; see recreateSpecialObjectsArray)
Object>>attemptToAssign: aValue withIndex: index
^self noModificationErrorFor: #instVarAt:put: index: index value: aValue

Object>>at: index put: value
"Store the argument value in the indexable field of the receiver indicated by
index. Fail if the index is not an Integer or is out of bounds. Fail if the
value is not of the right type for this kind of collection. Answer the
value that was stored."

<primitive: 61 errorCode: ec>
index isInteger ifTrue:
[(index >= 1 and: [index <= self basicSize])
ifTrue:
[self isImmutable
ifTrue: [^self noModificationErrorFor: #at:put: index: index value: value]
ifFalse: [^self improperStoreError]]
ifFalse: [^self subscriptBoundsErrorFor: #at:put: index: index value: value]].
index respondsToArithmetic
ifTrue: [^self at: index asSmallInteger put: value]
ifFalse: [^self nonIntegerIndexError: index]

Behavior>>adoptInstance: anObject
"Changes the class of the argument to be that of the receiver provided that
 a) anObject is not immutable (which includes immediates)
 b) anObject is bits and the receiver is a bits class, or
 c) anObject is pointers and the receiver is pointers and anObject has the same
number of inst vars as specified by the receiver, or the receiver is variable and
anObject has at least as many inst vars as specified by the receiver.
Answers the receiver."

<primitive: 536 errorCode: errCode>
^(errCode ~~ nil
  and: [errCode name = #'no modification'])
ifTrue: [anObject noModificationErrorFor: #changeClassTo: index: nil value: self]
ifFalse: [self primitiveFailed]

etc.  This is much more flexible and works well in practice.  In the vw7.7nc image I looked at there are 20 senders of #noModificationErrorFor:index:value: for variants of 5 operations:

- attempting to assign to an inst var of a read-only object, either directly in code via an inst var assignment or via an instVarAt:put: send
- attempting to assign to an indexed inst var of a read-only object, via an at:put: send (at:put: byteAt:put: unsignedLongAt:put: etc)
- attempting to assign to an indexed inst var of a read-only object via a BitBlt primitive (e.g. copyBitsClippedStride:width:atX:y:from:stride:width:atX:y:width:height:rule:)
- attempting to become a read-only object via two-way become; forwarding become is not considered a modification.
- attempting to change the class of a read-only object



Digression:

Looking at the above the obvious convenience is to add

Object>>noModificationErrorFor: selector value: value
^(NoModificationError receiver: self selector: selector index: nil value: value) raiseRequest

which would be applicable in 8 of the 20 uses, but it would imply adding overrides in subclasses.  #noModificationErrorFor:index:value: is implemented in ProtoObect, Object, Symbol and VariableBinding. The implementations in Symbol and VariableBinding simply provide more explanatory error messages.

HTH


Norbert

 

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





--
_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

Eliot Miranda-2
In reply to this post by NorbertHartl
 
Hi Norbert,

On Sat, Jan 6, 2018 at 6:09 AM, Norbert Hartl <[hidden email]> wrote:
 
Thanks Clement!

Am 06.01.2018 um 14:44 schrieb Clément Bera <[hidden email]>:



On Sat, Jan 6, 2018 at 1:32 PM, Norbert Hartl <[hidden email]> wrote:
 
Btw. why is the name NoModificationError? Isn’t the purpose of an Error that it is not resumable? And is NoModificationError very likely to be resumed? Like in #retryModification?

Normally the NoModificationError should be able to retry the modification, and then resumes execution just after the primitive call / the inst var store. 

I was actually asking why the mentioned NoModificationError is an error and not a NoModificationException. 

Ah, that's a great distinction.  Indeed NoModificationException is a better name.  So ignore my comments about orthogonality.  I like the distinction between exceptions being resumable and errors not,.  I've learned something.  Thank you.
 

In the case of primitive failure I guess either the error should resume with the value to return or the primitive fall-back code should return the correct value. In the case of inst var store, we need to resume execution using #jump (See #attemptToAssign: value withIndex: index)
 
For at:put: we could have:

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [^ (ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]

OR 

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal. ^ anObject]
ifFalse: [ self primitiveFailed ]

+ the index bound check mentioned by Eliot and asInteger trick (based on at: primitive failure code)

So that when the exception resumes the correct behavior happens.

I have now

at: index put: value 
"Primitive. Assumes receiver is indexable. Store the argument value in 
the indexable element of the receiver indicated by index. Fail if the 
index is not an Integer or is out of bounds. Or fail if the value is not of 
the right type for this kind of collection. Answer the value that was 
stored. Essential. See Object documentation whatIsAPrimitive."

<primitive: 61>
index isInteger 
ifTrue: [self class isVariable
ifTrue: [(index between: 1 and: self size)
ifFalse: [self errorSubscriptBounds: index]]
ifFalse: [self errorNotIndexable]]
ifFalse: [  
index isNumber
ifTrue: [^self at: index asInteger put: value]
ifFalse: [self errorNonIntegerIndex] ].
self isReadOnlyObject 
ifTrue: [ ^ (InstVarModificationForbidden for: self atInstVar: index with: value) signal ].

Norbert


Norbert

Am 05.01.2018 um 14:34 schrieb Eliot Miranda <[hidden email]>:

Hi Clément,

    is it too late to take a look at the VisualWorks code and use the same class names and selectors they use?  IIRC it is NoMidificationError.  It may make e.g. Gemstone's job easier if there is some consistency.

_,,,^..^,,,_ (phone)

On Jan 5, 2018, at 5:22 AM, Clément Bera <[hidden email]> wrote:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





--
_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

NorbertHartl
In reply to this post by Eliot Miranda-2
 


Am 06.01.2018 um 19:45 schrieb Eliot Miranda <[hidden email]>:

Hi Norbert, Hi Denis,

On Sat, Jan 6, 2018 at 4:06 AM, Norbert Hartl <[hidden email]> wrote:
 


Am 05.01.2018 um 16:53 schrieb Clément Bera <[hidden email]>:



On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <[hidden email]> wrote:
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

yes something like that.


Same thing for Object>>#at:put:, floatAt:put: and so on.

I’m working on it and came by

Object>>#primitiveChangeClassTo:

and I wonder what would be the approach here. The ModificationForbidden/NoModificationError seems to be tight to an index. What would be the case for a class change? Another Exception class? How is that solved in VW? 

I don't think that adding variants of NoModificationError is wise.  One can imagine adding other state changes that read-onliness should prevent, such as unpinning.  So one could end up having to add a lot of subclasses of NoModificationError, each rather inflexible and able to respond to only one specific error.  We would end up with at least InstanceVariableModificationError IndexedVariableModificationError ClassModificationError BecomeIdentotyChangeError etc.  And even then IndexedVariableModificationError would need a message since there are many different kinds of at:put:s, at:put:, byteAt:put: unsignedLongAt:put: etc.

In VW it is handled by the NoModificationError holding a Message that can be used for the retry operation.  i.e.

Object>>noModificationErrorFor: selector index: index value: value
^(NoModificationError receiver: self selector: selector index: index value: value) raiseRequest

(N.B. the following is sent by the VM when an inst var assignment fails due to read-onlyness; see recreateSpecialObjectsArray)
Object>>attemptToAssign: aValue withIndex: index
^self noModificationErrorFor: #instVarAt:put: index: index value: aValue

Object>>at: index put: value
"Store the argument value in the indexable field of the receiver indicated by
 index. Fail if the index is not an Integer or is out of bounds. Fail if the
 value is not of the right type for this kind of collection. Answer the
 value that was stored."

<primitive: 61 errorCode: ec>
index isInteger ifTrue:
[(index >= 1 and: [index <= self basicSize])
ifTrue:
[self isImmutable
ifTrue: [^self noModificationErrorFor: #at:put: index: index value: value]
ifFalse: [^self improperStoreError]]
ifFalse: [^self subscriptBoundsErrorFor: #at:put: index: index value: value]].
index respondsToArithmetic
ifTrue: [^self at: index asSmallInteger put: value]
ifFalse: [^self nonIntegerIndexError: index]

Behavior>>adoptInstance: anObject
"Changes the class of the argument to be that of the receiver provided that
  a) anObject is not immutable (which includes immediates)
  b) anObject is bits and the receiver is a bits class, or
  c) anObject is pointers and the receiver is pointers and anObject has the same
number of inst vars as specified by the receiver, or the receiver is variable and
anObject has at least as many inst vars as specified by the receiver.
 Answers the receiver."

<primitive: 536 errorCode: errCode>
^(errCode ~~ nil
   and: [errCode name = #'no modification'])
ifTrue: [anObject noModificationErrorFor: #changeClassTo: index: nil value: self]
ifFalse: [self primitiveFailed]

etc.  This is much more flexible and works well in practice.  In the vw7.7nc image I looked at there are 20 senders of #noModificationErrorFor:index:value: for variants of 5 operations:

- attempting to assign to an inst var of a read-only object, either directly in code via an inst var assignment or via an instVarAt:put: send
- attempting to assign to an indexed inst var of a read-only object, via an at:put: send (at:put: byteAt:put: unsignedLongAt:put: etc)
- attempting to assign to an indexed inst var of a read-only object via a BitBlt primitive (e.g. copyBitsClippedStride:width:atX:y:from:stride:width:atX:y:width:height:rule:)
- attempting to become a read-only object via two-way become; forwarding become is not considered a modification.
- attempting to change the class of a read-only object



Digression:

Looking at the above the obvious convenience is to add

Object>>noModificationErrorFor: selector value: value
^(NoModificationError receiver: self selector: selector index: nil value: value) raiseRequest

which would be applicable in 8 of the 20 uses, but it would imply adding overrides in subclasses.  #noModificationErrorFor:index:value: is implemented in ProtoObect, Object, Symbol and VariableBinding. The implementations in Symbol and VariableBinding simply provide more explanatory error messages.

Thanks for the detailed answer. I’m not fully convinced that stripping off the quality for variants of forbidden modification is the right way to go. But I also had the gut feeling multiple classes could be over-done. I try and see if I’m satisfied with the single exception. Why I would favor the sublcasses is the fact that modifify the position of an object, modifying the shape of an object and modifying the state of an object are separated concerns that usually don’t go together. So the potential that each of the three can be treated differently could be helpful. 

thanks again,

Norbert

HTH


Norbert

 

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
_,,,^..^,,,_
best, Eliot

Reply | Threaded
Open this post in threaded view
|

Re: Bug in #instVarAt:put: for read-only objects

NorbertHartl
In reply to this post by Eliot Miranda-2
 


Am 06.01.2018 um 19:45 schrieb Eliot Miranda <[hidden email]>:

Hi Norbert, Hi Denis,

On Sat, Jan 6, 2018 at 4:06 AM, Norbert Hartl <[hidden email]> wrote:
 


Am 05.01.2018 um 16:53 schrieb Clément Bera <[hidden email]>:



On Fri, Jan 5, 2018 at 4:05 PM, Norbert Hartl <[hidden email]> wrote:
 
You mean

<primitive: 174 error: ec>
self isReadOnlyObject 
ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
ifFalse: [ self primitiveFailed ]
?

Norbert

yes something like that.


Same thing for Object>>#at:put:, floatAt:put: and so on.

I’m working on it and came by

Object>>#primitiveChangeClassTo:

and I wonder what would be the approach here. The ModificationForbidden/NoModificationError seems to be tight to an index. What would be the case for a class change? Another Exception class? How is that solved in VW? 

I don't think that adding variants of NoModificationError is wise.  One can imagine adding other state changes that read-onliness should prevent, such as unpinning.  So one could end up having to add a lot of subclasses of NoModificationError, each rather inflexible and able to respond to only one specific error.  We would end up with at least InstanceVariableModificationError IndexedVariableModificationError ClassModificationError BecomeIdentotyChangeError etc.  And even then IndexedVariableModificationError would need a message since there are many different kinds of at:put:s, at:put:, byteAt:put: unsignedLongAt:put: etc.

In VW it is handled by the NoModificationError holding a Message that can be used for the retry operation.  i.e.

Object>>noModificationErrorFor: selector index: index value: value
^(NoModificationError receiver: self selector: selector index: index value: value) raiseRequest

(N.B. the following is sent by the VM when an inst var assignment fails due to read-onlyness; see recreateSpecialObjectsArray)
Object>>attemptToAssign: aValue withIndex: index
^self noModificationErrorFor: #instVarAt:put: index: index value: aValue

Object>>at: index put: value
"Store the argument value in the indexable field of the receiver indicated by
 index. Fail if the index is not an Integer or is out of bounds. Fail if the
 value is not of the right type for this kind of collection. Answer the
 value that was stored."

<primitive: 61 errorCode: ec>
index isInteger ifTrue:
[(index >= 1 and: [index <= self basicSize])
ifTrue:
[self isImmutable
ifTrue: [^self noModificationErrorFor: #at:put: index: index value: value]
ifFalse: [^self improperStoreError]]
ifFalse: [^self subscriptBoundsErrorFor: #at:put: index: index value: value]].
index respondsToArithmetic
ifTrue: [^self at: index asSmallInteger put: value]
ifFalse: [^self nonIntegerIndexError: index]

Behavior>>adoptInstance: anObject
"Changes the class of the argument to be that of the receiver provided that
  a) anObject is not immutable (which includes immediates)
  b) anObject is bits and the receiver is a bits class, or
  c) anObject is pointers and the receiver is pointers and anObject has the same
number of inst vars as specified by the receiver, or the receiver is variable and
anObject has at least as many inst vars as specified by the receiver.
 Answers the receiver."

<primitive: 536 errorCode: errCode>
^(errCode ~~ nil
   and: [errCode name = #'no modification'])
ifTrue: [anObject noModificationErrorFor: #changeClassTo: index: nil value: self]
ifFalse: [self primitiveFailed]

etc.  This is much more flexible and works well in practice.  In the vw7.7nc image I looked at there are 20 senders of #noModificationErrorFor:index:value: for variants of 5 operations:

- attempting to assign to an inst var of a read-only object, either directly in code via an inst var assignment or via an instVarAt:put: send
- attempting to assign to an indexed inst var of a read-only object, via an at:put: send (at:put: byteAt:put: unsignedLongAt:put: etc)
- attempting to assign to an indexed inst var of a read-only object via a BitBlt primitive (e.g. copyBitsClippedStride:width:atX:y:from:stride:width:atX:y:width:height:rule:)
- attempting to become a read-only object via two-way become; forwarding become is not considered a modification.
- attempting to change the class of a read-only object



Digression:

Looking at the above the obvious convenience is to add

Object>>noModificationErrorFor: selector value: value
^(NoModificationError receiver: self selector: selector index: nil value: value) raiseRequest

which would be applicable in 8 of the 20 uses, but it would imply adding overrides in subclasses.  #noModificationErrorFor:index:value: is implemented in ProtoObect, Object, Symbol and VariableBinding. The implementations in Symbol and VariableBinding simply provide more explanatory error messages.


I forgot the ask why the implementation should be in both ProtoObject and Object.

Norbert

HTH


Norbert

 

Am 05.01.2018 um 14:22 schrieb Clément Bera <[hidden email]>:

Hi,

No this is not a bug.

This needs to be handled in the primitive failure code in the image, the method should be something like that :

Object>>instVarAt: index put: anObject
<primitive: 174 error: ec>
        self isReadOnlyObject ifTrue: [(ModificationForbidden for: self atInstVar: index with: anObject) signal]
self primitiveFailed

All primitive fall-back code triggering object mutation should be rewritten this way, especially primitives such as #at:put:, #instVarAt:put:, etc.

Cheers

On Fri, Jan 5, 2018 at 1:42 PM, Norbert Hartl <[hidden email]> wrote:
 
If I do 

(#foo -> #bar)
beReadOnlyObject;
value: #baz

it throws

ModificationForbidden:  #foo->#bar is read-only, hence its field 2 cannot be modified with #baz

 which is as expected. But if I do

(#foo -> #bar)
beReadOnlyObject;
instVarNamed: #value put: #baz

I get

PrimitiveFailed: primitive #instVarAt:put: in Association failed

I think this a bug, no?

Norbert




-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
Clément Béra
Pharo consortium engineer
Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq





-- 
_,,,^..^,,,_
best, Eliot

12