`compilerClass` is ignored when changing ivars for the class

[dvmason]

Bug description Defining compilerClass in a class allows alternate syntax, etc. A trait can be used to define this class-side method. However, in some cases changes to the class (changing ivars, for example) recompiles methods of the class, and when this is happening, the compilerClass seems to be ignored. It doesn't seem to matter whether it is in a trait or not.

To Reproduce Steps to reproduce the behavior:

1. Define class-side compilerClass for some class
2. Add a method that requires the alternate compiler
3. add a new ivar to the class
4. See error

Expected behavior The defined compilerClass should be used and all re-compilation should utilize that compiler.

Screenshots This shows what happened when I added the isNested ivar and pressed CMD-S

Version information:

• OS: Mac OS X
• Version: 10.14.6
• Pharo Version 9

Expected development cost Probably less than an hour for someone who knows the compiler. Days for anyone else.

Additional context This arose using https://github.com/dvmason/Pharo-Functional with the CompileWithCompose package, which defines a parrot operator (;>). This is what is shown in the screenshot, and using that package, the screenshot has all the code necessary to trigger the bug.

I also note there is no test for compilerClass

[dvmason]

A workaround is to define the compilerClass in Object class - in which case it takes effect properly.

But obviously you generally don't want to replace the compiler everywhere (although it is safe for the CompileWithCompose package, because the new operator was chosen very carefully.

[MarcusDenker]

I think I found the problem: Class>>#classSideCompilerClass is wrongly defined. It has to be:

classSideCompilerClass 
	"Redefine this method if you want to customize the compiler class for class-side methods"
	^ self compilerClass

That is, it has to fall back to the compiler class define by the class, not the global compiler.

Now I need to find a way to test that...

[MarcusDenker]

ah, I think we do not need to test compilation itself, just check that the method returns the right compiler if we have a class where it is changed.

[dvmason]

Thanks, Marcus, for looking at this. It raises a few concerns about how tricky this (fabulous feature) is to get right!

Yes, that does look like an error, but unfortunately fixing it doesn't solve the problem. Senders of compilerClass shows several place that send to Smalltalk, whereas, I think that Behavior>>#compilerClass is the only place that should (not sure about tests or Behavior>>#subclassDefinerClass). Senders of compiler raises some questions for me about interactions with methodClass, in particular in CompiledCode and CompiledMethod. DebugContext>>#evaluate: uses Smalltalk compiler as does DoItChunk.

Actually using the finder and a source-search for Smalltalk compiler finds all these cases and a few more that I don't know how to reason about.

[MarcusDenker]

There are places where we do not have a class in hand, or, where it just makes no sense to use the compiler of the class (self class). The notion of a default system compiler does make sense.

CompiledMethod does not know its compiler, it has to rely on the class that it is installed in to know it (something that we should improve).

But all this should not be the reason for this problem...

Can we come up with a test for this problem?

[MarcusDenker]

another idea: the class builder uses #recompile:from:

inside this method we use "self compiler", but it should be "oldClass compiler". This might be the problem because when we compile the class side, the new class is empty and the re-defined #compilerClass method is not there, thus is creates a normal compiler.

If we change it to "oldClass" it will use the changed compiler.

(this bug is then there for quite a while.. it is the same in Pharo8)

[Ducasse]

Hi dave We need more tests for this feature which is barely used. Please provide some. S

[dvmason]

I have a test almost working... should have a PR for it (and possibly a fix) by end-of-day Tuesday.

[MarcusDenker]

Great! I was exploring the issue a bit.

If you change the compiler to something that can not even compile the #compilerClass method on the class side, what do we do? I think this was the reason for the #classSideCompilerClass just using the global compiler, but that means that class side methods can not use another compiler at all.

lt might be that the notion to use just a method on the class itself that is supposed to use another compiler is maybe too naive.

[dvmason]

Yes, changing self to oldClass in #recompile:from: seems to fix it. I only changed it in Behavior, but there are 4 implementations. It should probably be changed in all (in particular, the originating bug came from a use of this in a Trait (the trait defined #compilerClass, and the error occurred when adding a slot to a class that used that trait)) - but I don't understand all the trait interactions well enough to know for sure.

It doesn't fix it class-side.

I see the complications. My first thought is that there should be a #compilerClassForClass and if anybody defines that, then they have a responsibility to make sure that those 2 methods are compiled normally. Or you could treat those two methods so they are always compiled with the standard Smalltalk compiler. But there definitely should be a way to compile class-side with an alternate compiler.

Note that PRs and CI can interact in interesting ways... I'm not sure that the CI captured my fix, or if it ran with that case as an expectedFailure

[dvmason]

In fact, class-side doesn't seem to pay attention to #compilerClass even on a straight compile. See the tests.

[MarcusDenker]

The problem in general: we use code to define which compiler to use. This is a bad idea, as of course that method has to be compiled, too. Which compiler to use there?

• the first time it has to be the standard compiler (or the method is not compilable)
• when I update that method, I should use the new compiler. Which now can not compile the method, as it was written for the standard compiler.

I think we need to declaratively specify the compiler and parser of a class. That is, it should be state of the class, not a methods (that needs a compiler to be compiled).

a workaround could be to force the compiler for certain selectors to be the standard compiler, but that sounds not nice either.

[MarcusDenker]

So I implemented #classSideCompilerClass, and the tests are working after that.