Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing class side variable after reloading a Trait which used another Trait, from GitHub + image unstability and crashs #14749

Closed
labordep opened this issue Sep 22, 2023 · 19 comments · Fixed by #14766 or #14768

Comments

@labordep
Copy link
Collaborator

labordep commented Sep 22, 2023

Bug description
Class side variable of a Trait is removed after reloading the Trait.
Due to this problem, my Pharo image is unstable and a lot of crashes can appears.
When I'm reloading the package manually, the var is appears! But the system is not stable

The Trait on GitHub (look at anInstanceVariable) :

Trait {
	#name : #MolDirtyType,
	#classInstVars : [
		'anInstanceVariable'
	],
	#traits : 'MolComponentType',
	#classTraits : 'MolComponentType classTrait',
	#category : #'Molecule-Tests-Resources - Contracts'
}

The Trait after loading the project into Pharo :
image
Look at the red methods which are using anInstanceVariable.

The Trait after re-loading manually the package :
image
image
Red methods are always here.
image
image
The image is always unstable, var is here but not considered.

To Reproduce
Steps to reproduce the behavior:

  1. Create a Trait1
  2. Define another Trait2 which uses Trait1
  3. Define a class side variable into Trait2
  4. Publish the code into GitHub
  5. Create a new image
  6. Reload the project : the var in Trait2 is missing

The problem is present when loading manually (Add button) or from a baseline script.

About crashes and unstability
The image is unstable at the moment of a Trait1 with a class side var uses a Trait2, or at the moment of we create a class side var in a Trait 1 uses Trait 2.

Expected behavior
Variable should be exists, but missing.

Screenshots
If applicable, add screenshots to help explain your problem.

Version information:

  • OS: Windows 11 Pro 64 bits & Windows 10 Pro 64 bits
  • Version: Pharo 12 & Pharo 11 & Pharo 10
@jecisc
Copy link
Member

jecisc commented Sep 22, 2023

There was some updates in loading code in Pharo 12 but if it also happens in Pharo 11 it is probably not related to that. I'll try to find some time next week to look into it since I recently learned more about the code managing loading classes and traits,

@labordep
Copy link
Collaborator Author

Thanks @jecisc :) this problem is impacting our architecture framework at the core of all our applications :(

@Ducasse
Copy link
Member

Ducasse commented Sep 24, 2023

Pierre do I guess correctly that it was working correctly in P10?

@Ducasse
Copy link
Member

Ducasse commented Sep 24, 2023

This is really strange because Moose is heavily using traits and they do not experience this problem so I'm really curious.
May be this is a metaclass level problem.

@labordep
Copy link
Collaborator Author

Pierre do I guess correctly that it was working correctly in P10?

I have tested to load my project in Pharo 10, you can try with this script :

Metacello new
   baseline: 'Molecule';
   repository: 'github://OpenSmock/Molecule:dev-99';
   load.

And this is the same problem :/

image

And on GitHub :

image

Very strange! And the commit window is curious because it seams that the system know there is a difference but it cannot show it correctly :

image

@labordep
Copy link
Collaborator Author

labordep commented Sep 24, 2023

May be Molecule can impact in a way this bug, can you reproduce this problem ?
=> No (see below)

@labordep
Copy link
Collaborator Author

labordep commented Sep 24, 2023

I writted a project to reproduce : https://github.com/labordep/Project14749
Add manually the project with the "Add" button works and my Trait is not yet using another Trait.

GitHub
Test project to reproduce issue 14749. Contribute to labordep/Project14749 development by creating an account on GitHub.

@labordep
Copy link
Collaborator Author

labordep commented Sep 24, 2023

Ok I reproduced it clearly !

image

I update my issue, the problem appears when the Trait uses another Trait.
The image was instable and crash at the instant of my Trait (with a class side var) uses another Trait.
I update my issue with this information.

@labordep labordep changed the title Missing class side variable after reloading a Trait from GitHub + image unstability and crashs Missing class side variable after reloading a Trait which used another Trait, from GitHub + image unstability and crashs Sep 24, 2023
jecisc added a commit to jecisc/Bazard that referenced this issue Sep 25, 2023
jecisc added a commit to jecisc/pharo that referenced this issue Sep 25, 2023
- MCClassDefinition>>createClass  should not hardcode a global access
- MCTraitDefinition>>createClass should not set the class slots and comment outside the class builder
- MCTraitDefinition>>createClass should catch the same warning as the overriden method (I guess? Or it should be removed in both? If someone knows this part of the system better than me let me know what you think :) )

I found this looking at pharo-project#14749 but this does not fix the problem
@jecisc
Copy link
Member

jecisc commented Sep 25, 2023

I found the problem but I do not know yet how to fix it.

The problem is that for traits we have MCTraitDefinition and MCClassTraitDefinition in Monticello.

Things such as class slots and managed by MCTraitDefinition and are installed correctly. But the class trait composition is set by MCClassTraitDefinition afterward and this removes anything else configured in the class side.

I don't understand why we have both those classes and not just one. I need to talk to Pablo to see if we can unify. But anyway, knowing the root of the problem I'll at least be able to produce a workaround.

jecisc added a commit to jecisc/pharo that referenced this issue Sep 25, 2023
This is a fix for issue pharo-project#14749 in Pharo 11. 

The fix is different than in Pharo 12 because in Pharo 12 I�did cleaner version of the clean by removing the usage of MCClassTraitDefinition alltogether since all infos are already on MCTraitDefinition.

The workaround here is to not recompile entirelly the class trait if the base trait is already present in the image. Instead it will just update the trait composition, thus it will not remove the instance variables.
@jecisc
Copy link
Member

jecisc commented Sep 25, 2023

So I did a workaround for Pharo 11: #14768

And for Pharo 12 I have a PR on Pharo and a PR on Tonel with a better cleaning and both are passing:
#14766
pharo-vcs/tonel#112

I am now waiting for reviews and this should fix your problem

@labordep
Copy link
Collaborator Author

So I did a workaround for Pharo 11: #14768

And for Pharo 12 I have a PR on Pharo and a PR on Tonel with a better cleaning and both are passing: #14766 pharo-vcs/tonel#112

I am now waiting for reviews and this should fix your problem

Thanks @jecisc! Keep me informed :)

@MarcusDenker MarcusDenker linked a pull request Sep 25, 2023 that will close this issue
@labordep
Copy link
Collaborator Author

labordep commented Sep 25, 2023

It looks good!
But a lot of crashs :/

image

@jecisc
Copy link
Member

jecisc commented Sep 25, 2023

This should also be fixed in Pharo12 tomorrow

@labordep
Copy link
Collaborator Author

labordep commented Sep 25, 2023

This should also be fixed in Pharo12 tomorrow

I have a lot of crashs in P11 :/
Especially when I'm playing with Iceberg, for example to load/view/explore/just select a branch with this specific Trait.

@jecisc
Copy link
Member

jecisc commented Sep 26, 2023

Note: The crashes are coming from a VM that is too old probably

@jecisc
Copy link
Member

jecisc commented Sep 26, 2023

@labordep This issue should be fixed in Pharo 12 too now

@labordep
Copy link
Collaborator Author

Note: The crashes are coming from a VM that is too old probably

Thanks @jecisc, I check this problem and if necessary open a new issue.

@labordep
Copy link
Collaborator Author

@jecisc we cannot change the VM because of that : #13726

@labordep
Copy link
Collaborator Author

labordep commented Oct 1, 2023

@jecisc this is ok in the lastest VM from my Home, I haven't crashes 👍 .
At work we need to solve #13726.
Thanks for the fix !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants