groovy

I was trying a way to fix this issue but I have hit a little stumbling block. The issue is related to adding new phase operations in the list that is being iterated.

So Grab annotation gets processed in phase CONVERSION and spock has this transform called EarlyTransform, which is also to be added in the phase CONVERSION.

Now the list that contains phase operations of CONVERSION is getting iterated when it calls GrabAnnotationTransformation, which then results in addition of new spock transform EarlyTransform to the same list causing ConcurrentModificationException.

Any suggestions there?

Jochen, I am attaching the patch here for review.

Here is some information about the patch:

• ASTTransformationVisitor is modified to support multiple rounds of scanning for AST transforms.

• While the first round of transforms scanning is triggered usually, 2nd one is invoked by GrabAnnotationTransformation after it has successfully fetched dependencies.

• CompilationUnit has been modified to support a parallel list of phase operations that the new AST transforms (introduced by newly grabbed dependencies) could add phase operations to. And then after doing the standard phase operations, this extended set of phase operations are also processed.

Do you see any issues with the solution being introduced?

Comments?

hmm... let me think about this a bit...

You need the new phases because else you get those nasty concurrent modififaction exceptions. If, by chance, a newly added transform would like to add another one, then we get the problem again.

I don't know, I don't like the design, but I have problems comming up with a better solution. Maybe I would solve that with a kind of stack... the same style as new sources are handled. That means using the normal API you can add phases, but the new phases are kept in a stack. Once the phase ends we process the stack and add the transforms to the correct pahse. If a transform is added ot the same phase, then we have to process the remaining ones from the stack. If we have not started processing yet, then we can add the phases directly. If processing started already, then adding a transform to an operation before that should cause an exception. Well, in case of a global transform it should probably be ignored.

Instead of having an add after grab method I think we should provide a method that can add global transforms and let the grab annotation call it. It is then the responsibility of the annotation to add global transforms. That should remove all the firstScan logic.

What do you think?

I am not sure if I understood your stack suggestion fully.

Are you saying that even the standard (current) phase operations we need to start maintaining in a stack?

Or you are saying that existing operations should be as-is but the newly added ones should go and sit in the stack? If yes, then how can that be done using the existing API for adding phase operations? We will need to add a different method that will store in stack, right?

Also I am not sure how will a newly added transform add another one? I know that will create issues with the data structures as used right now but I can't think how will that happen? We are talking about new ast transforms that are coming in only due to @Grab, right? @Grab will not fetch scripts, right? (which will undergo groovy compilation again on the fly?) Those will be pre-compiled jars, right? Which means that they will not undergo on-the-fly groovy compilation? So how will the cyclic addition of transforms happen?

I might be missing something basic. It's always possible with me.

I also did not understand the suggestion about letting GrabAnnotationTransformation have a method that will add global transforms. The logic for finding out and adding global transforms is already present in ASTTransformationVisitor, so I was trying to reuse that instead of implementing it again in GrabAnnotationTransformation.

It also had the advantage that check for duplicates could be done by remembering the transforms added in firstScan. That way 2nd scan onwards only the new transforms get added as phase operations (by doing a comparison with transforms successfully added in previous scans).

Jochen, attaching patch V2. The change is that I have made it stack based now. So, if there are cyclic additions of AST transforms, it won't run into concurrent modification errors anymore.

As I mentioned earlier, I didn't understand your suggestion about removing the firstScan logic. As far as I see it at this point, it is all required.

Waiting for comments V2.

Jochen, this newer patch is working fine but because I am not getting any feedback, I am not able to proceed.

If no critical review comment is remaining from earlier, shall I go ahead applying it?

Fixed. Confirmation of the fix sought from Peter against original issue GROOVY-3851.