Skip to content

Conversation

@PalumboN
Copy link
Collaborator

@PalumboN PalumboN commented Jul 25, 2025

This PR changes the behaviour of installing/unistalling extension methods.

The case I don't like

Now if you have a method in a package:

"Package: MyPackage"
MyClass >> m
  ^ 'original'

And then you install another package with an extension method:

"Package: *MyPackageExtension"
MyClass >> m
  ^ 'extension'

The original method is lost forever.

Then, for example, if you delete or unload the extension package, you object keeps without any method #m 👎


What is done here?

This PR changes the compilation of extension methods to:

  1. Save the original method into the compiled method literals
  2. When it is removed, the original method is recovered

Something cool that you can do with this:

We could define the extension method based on the replaced method.
We still need a syntax for this, but I could make this extension method using the IRBuilder support:

m
	<opalBytecodeMethod>
	^ OCIRBuilder buildIR: [ :builder |
		builder
				pushLiteral: 'extension of ';
				
				pushLiteralAt: 5; "AdditionalMethodState"
				pushLiteral: #extendedMethod;
				send: #propertyAt:;
				pushLiteral: nil;
				send: #valueWithReceiver:;
				
				send: #,;
				returnTop
		 ]

With result:

MyClass new m   "'extension of original'"

Other stuff

  • Also, the original method was configured as extension when it is replaced. I changed it (see tests)

What is missing?

There are still some cases to test/support:

  • Editing the extended method make it lose the original method
  • We could have an "extension chain" of methods, we should think how it works
  • What happen when an extended method is called and it uses super?

Done in the sprint 🎉

@PalumboN
Copy link
Collaborator Author

There are failing tests that I cannot reproduce locally 😢

I'll check them at some day...

@jecisc
Copy link
Member

jecisc commented Jul 25, 2025

testClassMethodDefinition is a random failure happening from time to time

@jecisc
Copy link
Member

jecisc commented Jul 25, 2025

The ones about unloading are not random failures

@jecisc
Copy link
Member

jecisc commented Jul 25, 2025

To preserve the method that was lost when we edit the extension method you can check something I implemented:
AdditionalMethodState class>>#propertiesToPersist and CompiledMethod>>#migratePersistingPropertiesIn:.

We can tag some properties as "properties to persist through recompilation" and it will migrate them from the previous instance to the new compiled method

@jecisc
Copy link
Member

jecisc commented Jul 25, 2025

This should fix your first "What is missing" easily ;) You just need to add a symbol to #propertiesToPersist

@Ducasse
Copy link
Member

Ducasse commented Jul 26, 2025

Nice. Now I would not do anything with the hidden method. No such dependency or we will obtain chaos.

@estebanlm
Copy link
Member

while this is nice, I fear the excessive "magic" used here. We need to adapt the tooling to make this explicit/visible and then... why not same behavior to each method override ?

In the past, I played with the idea of implementing method extension as "Inversed Traits": we add e.g. a StringExtensionFromMyPackage kind of trait to String, which will add method extensions in a very explicit way, because if than you have :

ArrayedCollection << #String
    traits: { StringExtensionFromMyPackage1 + StringExtensionFromMyPackage2 }
    ....

there is clear where it belongs the extension method (which, by the way, will be able have a protocol more clear than *MyPackage1)
... and if we remove StringExtensionFromMyPackage2, then the StringExtensionFromMyPackage1 method will take over.

And why this are "Inversed traits" ?
Because in a trait, if there is already a method defined in class, it will not override it (and if there is m1 defined in two traits it will conflict)... we need to "inverse" this behavior to just "keep last method".
So, instead of having regular traits, because behvior is different, we should have something like:

ArrayedCollection << #String
    extensions: { StringExtensionFromMyPackage1 + StringExtensionFromMyPackage2 }
    ....

Of course, order of load here becomes a thing, but also in your solution ;)

NOTE: I am not saying we need to implement this, I just wanted to share the solution I had in mind for the same problem (and some others around) ;)

@PalumboN
Copy link
Collaborator Author

Yes, I was thinking some ideas similar to Inversed Traits. But first I wanted to play a bit with extension (methods) and overrides. So my first approach is to not loose the existing method from one extended one. It should be useful for methods with super in an Inversed Traits.

Anyway, there are many ways to implement that. I just extended the mechanism that is already implemented :)

@PalumboN PalumboN force-pushed the improve-extension-methods branch from 0ebc3ec to cba6dfa Compare September 26, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants