Executing rewrite on classes using traits then commiting remove class side traits

[jecisc]

Describe the bug

In the current Moose image (5a091ef9709b216816b51941ef104695758864f1) I executed this script:

(r := RBParseTreeRewriter new)
    replace: '`@receiver isNil ifTrue: `@arg' with: '`@receiver ifNil: `@arg'.
    
((#('Moose' 'Famix' 'Fame') flatCollectAsSet: [ :n | (RPackageOrganizer default packages select: [ :p | p name includesSubstring: n ]) ]) flatCollect: #definedClasses)
        do: [ :class | 
            class localMethods
                do: [ :m | 
                    | n |
                    n := m parseTree.
                    (r executeTree: n) ifTrue: [ class compile: n formattedCode ] ] ]
        displayingProgress: [ :c | c printString ]

The I commited. When I commited, I only saw my changes. But in the files, multiple classes lost their class side trait usages.

For example, this line vanished from a lot of files:

	#classTraits : 'TEntityMetaLevelDependency classTrait + TDependencyQueries classTrait',

But the trait usage is still here.

[theseion]

AFAICT this has nothing to do with rewriting. It's just that TonelWriter ignores class side trait definitions for some reason.

[jecisc]

Oh, I see that we integrated already in Moose some PRs that remove some class side definition... This is really bad since it is happening in the stable release.

[theseion]

#isTrait: hasn't changed in three years... Not sure what's causing this suddenly...

[theseion]

Looks like @tesonep recently wrote a fix for this: pharo-vcs/tonel@7df3b4e

[theseion]

Nope, that commit does not fix our issue.

[theseion]

There appear to be two unrelated problems:

1. The one that @jecisc describes: class trait compositions are somehow lost when certain conditions apply (still no clue what those are)
2. Once they have been removed from the repository they keep showing up as additions in the commit view of iceberg but can't be committed. The reason for this is that the base snapshot is actually computed from the working copy on disk, which does not include the class trait composition, so the MCTraitDefinition that would be used to write the class trait definition, doesn't. Monticello uses two classes to represent traits and class traits but Tonel only writes a single file. The class trait definitions show up in the commit view, even though only the trait definitions should. I think this needs to be solved in IceMCSnapshotPatchVisitor where modifications in class trait definitions need to be applied to trait definitions.

[theseion]

Here's a very ugly hack for fixing the MCTraitDefinition entries in the patcher:

IceGitWorkingCopyUpdateVisitor>>visitPackage: anIcePackageDefinition 
	
	"I am a package, I have to export my changes using the repository format.
	Thus, I cut here the recursion on my children, I have to do the recursion myself"
	| mcPackage snapshot patcher version patcherDefinitions |
	mcPackage := MCPackage named: anIcePackageDefinition name.

	"Take a snapshot of the package from the version we want to apply our changes on (usually the commit in disk) and apply them to it.
	This will create a snapshot with only the selected changes"
	snapshot := [diff targetVersion snapshotFor: anIcePackageDefinition]
		on: NotFound do: [ MCSnapshot empty ].
	patcher := MCPatcher snapshot: snapshot.
	currentNode accept: (IceMCSnapshotPatchVisitor new
		patcher: patcher;
		yourself).

	"<----- HACK"
	patcherDefinitions := (patcher instVarNamed: 'definitions') definitions.
	patcherDefinitions
		select: [  :e | e class = MCClassTraitDefinition ]
		thenDo: [  :e |
			patcherDefinitions
				detect: [ :def |
					def class = MCTraitDefinition and: [
						def className = e className ] ]
				ifFound: [ :def |
					def
						instVarNamed: 'classTraitComposition'
						put: e classTraitComposition ]
				ifNone: [ ] ].
	"----->"
	version := MCVersion new
		setPackage: mcPackage
		info: (IceMCVersionInfo package: mcPackage message: 'Internal...')
		snapshot: patcher patchedSnapshot
		dependencies: #().

	"Save version to disk.
	The repository will take care of the correct output format.
	Note: this step does not perform a commit, it just exports changes so the repository can then perform a commit"
	
	index storeVersion: version

[theseion]

Awesome! Thanks!

[georgeganea]

Hi guys, will this also be released in Pharo 8? Currently working with traits in Pharo 8 is a bit challenging without this fix :)

[tesonep]

Yes, I have it in my TODO list to make a back port

…

On Wed, Apr 1, 2020 at 4:35 PM George Ganea ***@***.***> wrote: Hi guys, will this also be released in Pharo 8? Currently working with traits in Pharo 8 is a bit challenging without this fix :) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#5988 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACWY75ZIQVCKJ2PCICVPXLRKNGLPANCNFSM4LSV56TQ> .

-- Pablo Tesone. tesonep@gmail.com