pharo-project / pharo

Pharo is a dynamic reflective pure object-oriented language supporting live programming inspired by Smalltalk.
http://pharo.org
Other
1.19k stars 353 forks source link

Add an instance variable to Class does not work #17006

Open PalumboN opened 1 month ago

PalumboN commented 1 month ago

Bug description

I'm trying to add a new instance variable to Class definition but several errors appear.

To Reproduce

  1. Go to Class definition and add a new instance variable:

    ClassDescription << #Class
    slots: { ... #packageTag . #newVariable "<<<" };
    sharedVariables: { #ObsoleteClasses };
    tag: 'Classes';
    package: 'Kernel-CodeModel'
  2. As this is an important class for the VM, a notification warning appears. But it's ok, it is part of an experiment for the VM.

  3. On continue you get a first MNU (in Pharo 12): image

I defined this method in Metaclass to continue

classInstaller
    ^ self soleInstance classInstaller
  1. Then, I have a similar problem with getName message: image

Again, I defined the method in Metaclass to continue

getName
    ^ self name
  1. And then I'm stuck in this validation 😞 I don0t know how to continue.

image

Expected behavior

At the end I want to have newVariable as instance variable in all the classes in the system.

Version information:

hernanmd commented 1 month ago

Something like this badre:

validateClassName
    | nameToValidate |

    name ifNil: [ ^self ].

    "I try to convert to symbol, if there is an error the next guard will catch it"
    [ name := name asSymbol ] on: Error do: [  ].

    name isSymbol ifFalse:[InvalidGlobalName
                signal: 'Global names should be symbols'
                for: name].

    nameToValidate :=   (name indexOfSubCollection: ' class' startingAt: 1) > 0
        ifTrue: [ name copyUpToSubstring: ' class'  ]
        ifFalse: [ name ].
    nameToValidate isValidGlobalName ifFalse: [
        InvalidGlobalName signal: 'Class name is not a valid global name. It must start with uppercase letter and continue with alphanumeric characters or underscore. Default names used in class or trait templates are not allowed.' for: name ].

    DangerousClassNotifier check: name

and good luck with the Segmentation fault :)

jordanmontt commented 1 week ago

Have you tried of directly using the fluid builder and installer to change the definition of Class instead of doing it using Calypso? It might help

astares commented 1 week ago

If the fluid builder approach does not work and fails too: maybe changing it in source files via text editor (locally in git), commiting and loading then via Iceberg works

MarcusDenker commented 1 week ago

I fear this for now requires a full boostrap. That is, do the PR manually and let the CI bootrap the image.

The question is: can we fix the class builder so it can do this? Adding an ivar to Class is ok from a VM perspective (only the first three ivars are directly accessed, and they are defined in Behavior).

But for sure the standard instance migration might be a problem if it is Class that we are changing.

PalumboN commented 1 week ago

Yes, I tried with the fluid builder first, it has the same behaviour.

I didn't try editing the source file and loading it via Iceberg, I'll try.

But I think that the problem is what @MarcusDenker says. I would like to fix the current instance migration algorithm and be able to do this from the image (as with any other class). I'll try to check it in the next VMDojo / Sprint.