review: refactor: mutable collections of Spoon model handles parent and fire change events

[pvojtechovsky]

First step towards #1633
... at the beginning only for CtBlock#statements
... later the same solution should be applied to each List, Set and Map of Spoon model

WDYT?

[surli]

This method should be changed to be generic

[pvojtechovsky]

why? It is private and it uses generic argument from ListModel class

[surli]

Actually no you the name says it's for statements and you have this: list == CtElementImpl.<CtStatement>emptyList()

It should use a T and have a name like ensureModifiableList

[pvojtechovsky]

Now I see it ;-), Thanks. I will fix it probably today in the evening

[surli]

Why does this method return a mutable list? It seems on purpose regarding the previous call of ensureModifiableStatementsList but it feels wrong...

[pvojtechovsky]

It returns a mutable ModelList, which can be mutated using List API and which assures that spoon model is kept consistent (parent-child is kept) and that modification events are sent.

This change is the CORE idea of this PR ;-)

[surli]

This change is the CORE idea of this PR ;-)

Hmmm. It looks a bit dangerous for me: old users won't change their API and won't beneficiate the changes, but it might disturb new users because they won't necessarily expect the API to return a pointer on the model.

WDYT about keeping a getStatements that returns an immutable list, and adding a getModifiableStatements like the one you proposed?

[pvojtechovsky]

Note: that this method already returned modifiable list before. I just assured that this modifiable list behaves correctly.

[surli]

that this method already returned modifiable list before

My point is that I believe we should have the same consistent behaviour everywhere in Spoon. And I think that almost everywhere - but maybe not here - this kind of method is currently returning a new ArrayList, detached from the model (see for example CtClass#getConstructors or CtType#getDeclaredExecutables): so they are mutable, but not linked to the model. If we make those methods returning a mutable element which is now linked to the model, it really can bring unexpected behaviours for the clients.

That's why I suggest to start here to fix a definitive behaviour that we'll use everywhere. And I'll definitively prefer to change those methods to return an immutable object and to create another methods which return explicitely a mutable linked to the model: we might break some clients, but the fix is easy and explicit.

[surli]

We already call clear below in the method: this could now be simlified by just adding a if not null around the final addAll

[pvojtechovsky]

you are right

[surli]

There's some minor issues but it looks much more elegant than before ;)

[pvojtechovsky]

My point is that I believe we should have the same consistent behaviour everywhere in Spoon.

I understand the problem and agree that spoon should behave consistent. So there are two questions:

1. what to do until all spoon properties are switched to mutable ModelList/ModelSet/ModelMap - can we expect that we are able to change whole spoon model until next spoon release?
2. let's say we changed all properties to mutable and safe ModelList/ModelSet/ModelMap: Do we want
   A) to have each property duplicated like getStatements and getMutableStatements
   B) or we can simply use only getStatements which will return mutable list.

I vote for B), because it ends with lower maintenance, nicer API and good compatibility with old clients.

Note: there are actually these implementations in spoon model:
A) getter returns immutable collection - OK it is compatible if we return mutable collection now
B) getter returns muttable collection, whose mutation adds values into model, but which does not sets parent and does not call change events - OK it is compatible too, because we just fixed the bug and spoon finally works as it was already expected by clients
C) getter returns detached muttable copy of collection - whose mutations has no influence to model. Here we are incompatible. I am not aware of this code yet, but if there is really some then we need some solution ... the easier is to break compatibility here and to keep nice and simple model API for future.

[surli]

Maybe a good way to prepare this is to setup a test architecture to check what we want:

1. Get all methods returning collections and check if the returning object is an immutable or not, then we might have an idea of what we will break in case C
2. Assert that the returning object should be a ModifiableCollection for the future implementation: then we release only when this test pass

And to take into account the problem C an easy way is to release it as major release and document why. @monperrus won't be happy but it looks like the easiest way to avoid maintaining methods with mutable and immutable.

[monperrus]

Get all methods returning collections and check if the returning object is an immutable or not, then we might have an idea of what we will break in case C

Probably none of them if it's not the empty list.

[monperrus]

And I think that almost everywhere - but maybe not here - this kind of method is currently returning a new ArrayList, detached from the model (see for example CtClass#getConstructors or CtType#getDeclaredExecutables)

IMHO it's the opposite. It's mostly bound to the model but for a few specific cases such as the ones you mention. Anyway, we all agree that this should be 1) consistent and 2) specified and checked in a test. Let's write the test to measure the quantity of immutable / mutable-detached / mutable-attached collection non-derived getters.

[monperrus]

https://github.com/INRIA/spoon/blob/0b45cf90bd380b8cdfc8dd2c2d29cf3f74a445d7/src/main/java/spoon/support/reflect/declaration/CtTypeImpl.java#L589 This is a derived getter. This is one point of confusion, here we are talking about non-derived getters. (updated my previous comment accordingly)

[surli]

This is one point of confusion, here we are talking about non-derived getters. (updated my previous comment accordingly)

Actually that's interesting: if all getters become mutable, how users will do the distinction between derived ones and the others, attached to the model? Without reading the doc I mean.

CtType myType = // a way to get my type

 // change nothing
myType.getAllFields().add(myNewField);
myType.getAllMethods().add(myNewMethod);

 // change the model and fire events
myType.getAllTypeMembers().add(myNewField);

[pvojtechovsky]

Actually that's interesting: if all getters become mutable, how users will do the distinction between derived ones and the others, attached to the model?

interesting question...

I vote to make immutable these methods (derived) whose mutation has no influence to the model. So it fails soon and client is immediately notified that it is not the correct way how to modify the model

[monperrus]

if all getters become mutable

almost all non-derived getters are already mutable :-) (my personal bet!) the derived ones should be immutable, and we won't break anything serious if we do so.

[pvojtechovsky]

The tests are actually failing on this old test code:

    CtBlock<?> body = ...

    for (CtStatement s : body) {
    	body.removeStatement(s);
    }

the body contains two statements, but it processes only one of them, because concurrent modification is actually not check correctly in ModelList. I can fix it, but the question is how it should behave???

A) it should throw concurrent modification exception - standard java behavior
B) it should create a copy of Array before iterator is created. And the iterator should be unmodifiable
C) it should create a copy of Array before iterator is created. But the iterator should be still modifiable

if A), then we break compatibility of Spoon API
if B), then we limit possibility to modify content of statements using iterator - I would prefer to have new collections fully compatible with java collections
if C), then it tries some compromise between expected java behavior and backward compatibility ... and I am not sure what problems will come later...

I actually vote for A). WDYT?

[pvojtechovsky]

I have created a test (see #1922) which checks collection types. Here is the detailed results
collection_check.txt
... rename it to CSV and let Excel sort it and count it

A summary:

• 18 tested derived collections are all mutable-detached
• 3 collections are mutable attached
  • CtBlock#statement
  • CtPackage#containedType
  • CtPackage#subPackage
• 1 collection is mutable detached
  • CtJavaDoc#commentTag
• 210 collections are read only

Test actually fails on

• CtIntersectionTypeReference#interface
• CtTypeParameterReference#interface
• CtWildcardReference#interface

[pvojtechovsky]

I have fixed remaining non-derived mutable attached/detached collections of spoon model. There were

• CtBlock#statements - was mutable attached
• CTPackage#packs - was mutable attached
• CTPackage#types - was mutable attached
• CtJavaDoc#tags - was mutable detached

So now there should be not possible to create a model with children, which has unitialized parents.

[surli]

Could you document that this internalList purpose is to manage the ConcurrentModificationException through modCount, maybe linking the URL: https://docs.oracle.com/javase/7/docs/api/java/util/AbstractList.html#modCount

It will ease the future maintaining ;)

And more generally I'm not sure why you update this value only when calling clear method: shouldn't you manage also the add/remove cases?

[pvojtechovsky]

Could you document

ok, I will do so.

And more generally I'm not sure why you update modCount only when calling clear method

I am directly changing it in clear method because this one changes content of internal list, but doesn't calls any method of that internal list. In all other cases the internal method of list is called so the modCount of that internal list is managed automatically. Then I just call updateModCount to read list.modCount and write it into this.modCount.

[surli]

Thanks for the explanation ;)

[pvojtechovsky]

This PR now fails on many places because of this ModelList code:

	static void linkToParent(CtElement owner, CtElement element) {
		if (owner.getFactory().getEnvironment().checksAreSkipped() == false && element.isParentInitialized() && element.getParent() != owner) {
			//the `e` already has an different parent. Check if it is still linked to that parent
			if (element.getRoleInParent() != null) {
				throw new SpoonException("The element " + element + " is already used by another part of SpoonModel. Remove it from previous model or clone it before.");
			}
		}
		element.setParent(owner);
	}

I think we should check whether added element is not already used on some other part of spoon model. Because otherwise you can add one element on two places and you will be not warned that you made invalid model....
But there are some cases when this technique is used by spoon internally ... for example creates helper CtStatementsList to insert some statements ... and then this linkToParent reports a problem...
How to solve it?
A) to remove check from linkToParent
B) to improve check form linkToParent by way it detects whether element is connected to real model - the one which starts with CtUnnamedModule and to report error only in that case. When there is just a model fragment, then it is OK.
C) to refactor spoon code to not use this technique or to remove the elements from CtStatementsList before they are inserted into another part of model.
D) ?

WDYT?

[surli]

How to solve it?
C) to refactor spoon code to not use this technique or to remove the elements from CtStatementsList before they are inserted into another part of model.

I think we should keep the contract explicit as it avoid unexpected side effects in Spoon model. So first let's try refactoring the current Spoon to assert that, and maybe then - if it makes the code harder to understand/maintain - add a flag to skip the contract in some very specific cases?

[pvojtechovsky]

The test FactoryTest#testIncrementalModel merges two models together. There are several inconsistencies:

• the produced target model contains elements whose getFactory() returns different values
• the source model becomes inconsistent, because that model still points to a children elements, but getParent of these elements points to different model.
• the model change notifications will not work well in such mixed model
• ...?

@monperrus you seems to be the author. Could you please fix this test somehow?

[pvojtechovsky]

Please have a look at CtPackageAssertTest#testEqualityBetweenTwoCtPackage test too. It seems to be broken multiple times

• actually assertSame(factory.getModel().getRootPackage();, factory.Package().getOrCreate("")) pass, but it was not so at the time when author made that test
• the CtPackage#addType adds type directly into this package. It ignores origin package of the type. So qualified name "spoon.testing.testclasses.Foo" becomes "Foo"

So there is question what is the intended behavior of CtPackage#addType
A) it should move type from it's current package to new package (to change qualified name)
B) it should add the type into package where it actually is

WDYT?

[pvojtechovsky]

ping @monperrus, please have a look at failing tests here. I have no idea how to fix them

[monperrus]

ok, I will have a look at it.

[monperrus]

This change:

• is good for explicit behavior
• is bad for concise API (must always delete in parent before intercession)
• will break a lot of client code (it already breaks the test code, see the changes in src/test)

I would prefer to revert it, so that it remains fully backward compatible, and that while keeping the other awesome advantages of the rest of this PR.

WDYT?

[monperrus]

This exception was essential for finding errors. But should we keep it now? It will break a lot of client code.

[pvojtechovsky]

it is already configurable here owner.getFactory().getEnvironment().checksAreSkipped() so one can switch it off

break a lot of client code.

It breaks only client code which is already broken, because that code creates invalid AST, which can cause unexpected behavior of spoon and tricky issues.

[monperrus]

Good news! when one removes:

//			if (element.getRoleInParent() != null) {
//				throw new SpoonException("The element " + element + " is already used by another part of SpoonModel. Remove it from previous model or clone it before.");
//			}

1. We restore backward compatibility, see https://github.com/INRIA/spoon/pull/1917/files#r178425580
2. testIncrementalModel passes again.

Double win!

To me that's the way to go.

[pvojtechovsky]

@monperrus, I copied the comments from above so it is easily visible what is the problem:

The test FactoryTest#testIncrementalModel merges two models together. Problems:

• the produced target model contains elements whose getFactory() returns different values
• the source model becomes inconsistent, because that model still points to a children elements, but getParent of these elements points to different model.
• the model change notifications will not work well in such mixed model
• ...?

Test CtPackageAssertTest#testEqualityBetweenTwoCtPackage has these problems:

• actually assertSame(factory.getModel().getRootPackage();, factory.Package().getOrCreate("")) pass, but it was not so at the time when author made that test
• the CtPackage#addType adds type directly into this package. It ignores origin package of the type. So qualified name "spoon.testing.testclasses.Foo" becomes "Foo"

So there is question what is the intended behavior of CtPackage#addType
A) it should move type from it's current package to new package (to change qualified name)
B) it should add the type into package where it actually is

[monperrus]

So there is question what is the intended behavior of CtPackage#addType

I would go for A) it should move type from it's current package to new package (to change qualified name)

when can do this first in a separate baby PR.

[pvojtechovsky]

The test in #1922 applied to this PR now reports

• 210 read only collections

They should be transformed to attached collections to be consistent. But in different PRs.

Test in #1922 was not able to check:

• CtIntersectionTypeReference#interface
• CtTypeParameterReference#interface

It is ready for merge from my point of view

[monperrus]

Thanks for the progress on this.

To review a PR, there are several cases:

• Case 1: if there are only new lines in the tests, and no removed lines, one knows that no specified behavior is changed and review is generally easy
• Case 2: if there are deleted lines in the tests, or changed assertions, one knows that one potentially breaks a lot of code and code-review is hard.

Here, this is clearly a case 2, and I am afraid that we break a lot of things. I suggest to document all the changes at the line level, in the tests:

• AstCheckerTest line 152: here, AFAIU, we basically discard the check for plenty of nodes, correct? If yes, we should do it directly before the assertions (and not in toBeProcessed) to clearly understand what we change.
• ImportTest: is it related to this PR?
• RemoveTest: I don't really understand, could we add a comment to explain the reason of the change?
• SignatureTest: what happens there?
• InsertBlockProcessor: getStatements().something is bad practice (we may fix it in another PR=, yet we keep it, what does this change mean?
• CtPackageAssertTest: why do lines 19,20 and 21 and 33 are removed?

Thanks, --Martin

[pvojtechovsky]

AstCheckerTest line 152: here, AFAIU, we basically discard the check for plenty of nodes, correct? If yes, we should do it directly before the assertions (and not in toBeProcessed) to clearly understand what we change.

It skips assertion for these nodes:
CtBlock#statement;MUTABLE_ATTACHED_CORRECT
CtJavaDoc#commentTag;MUTABLE_ATTACHED_CORRECT
CtPackage#containedType;MUTABLE_ATTACHED_CORRECT
CtPackage#subPackage;MUTABLE_ATTACHED_CORRECT

ImportTest: is it related to this PR?

no, it is not related. I made #2006 out of that

RemoveTest

added comment: iterate on copy of list of statements, otherwise it fails with concurrent modification exception

SignatureTest: what happens there?

createCodeSnippetStatement now returns a new element, which is ready to be added into some AST tree. It means that new element is actually not part of any other AST (otherwise consistency constraint will complain). That is the change. Before the returned element was part of the wrapper AST and after adding into some AST there was an inconsistency - new element was a child of two AST nodes.

I do not know why SignatureTest#testLiteralSignature expects that there exist some parent of the new element, ... I do not thing we should support that. So I removed that, because it failed with this PR.

InsertBlockProcessor: getStatements() ... what does this change mean?
block.getStatements().add(element.getBody());

the line above causes model inconsistency. The body of element is child of two nodes. So AST is no more tree ... This PR (by default) fails on such operations to warn about that inconsistency early. So test was fixed.

CtPackageAssertTest: why do lines 19,20 and 21 and 33 are removed?

because they would fail following the contract you agreed in #2000.
Note: factory.Class().create("spoon.testing.testclasses.Foo") creates a type, which is already assigned to the package spoon.testing.testclasses, whose parents are already root package.
So it makes no sense to re-add that type into root package.

[monperrus]

Thanks a lot for the explanation.

AstCheckerTest: why do we need to skip those nodes now?

RemoveTest: why this worked before? and this does not work anymore now?

[monperrus]

I still don't feel comfortable with this PR. It seems to me that we are trying to do two different things here:

1. A) a refactoring, as stated in the title of the PR "mutable collections of Spoon model handles parent and fire change events"
2. B) a behavior change related to not being in two parents at the same time.

Correct?

[pvojtechovsky]

yes, correct.

[pvojtechovsky]

I moved the consistency check and related fixes to #2009, so this one is simpler now

[monperrus]

Thanks a lot. So now we only have two changed tests:

AstCheckerTest: why do we need to skip those AST nodes now?

RemoveTest: why iteration+remove worked before? why does this stop working now?

[pvojtechovsky]

AST checker test checks that each Spoon model property modifier (set/add/remove,....) method calls model change notification ... the new implementation with ModelList and ModelSet is now different case, so it must be ignored by this test

remove test iterates over collection and then removes from this collection. Before it passed because it worked on the copy of the collection. Now it would fail because of concurrent modification exception.

[monperrus]

Now, I remember, AST checker works by pure static analysis. We have other tests which check the behavior dynamically (which is better and less fragile), such as ParentTest. Do we also have a dynamic test for checking the presence of change events?

[pvojtechovsky]

Do we also have a dynamic test for checking the presence of change events?

I do not know. But we will probably change all of them to ModelList and ModelSet, so they will be mutable attached... and then the correct behavior is tested by future #1922

[monperrus]

Thanks a lot for this step towards #1922