Parametrized class `Operation` is used without a type parameter

[algomaster99]

https://github.com/SpoonLabs/gumtree-spoon-ast-diff/blob/master/src/main/java/gumtree/spoon/diff/DiffImpl.java#L38

I feel that the use of the Operation class in the above line, and maybe in the entire file, should be parametrized with ? extends Operation<?>. This will reduce the number of warnings in projects which use gumtree-spoon as a package.

EDIT: missed a very important detail to parametrize Operation class.

[monperrus]

thanks for the suggestion, PR welcome :)

[slarse]

Do you mean List<? extends Operation> or List<Operation<? extends Operation>>? The former won't solve the problem, and the latter doesn't really make sense. Probably, it should be List<Operation<?>>, but that will cause compile errors elsewhere.

But I totally agree with @algomaster99 that this is a bit of a painpoint in using gumtree-spoon and should be addressed.

I see two options.

Remove the parameterization of Operation

Within gumtree-spoon itself, there is very little actual utility of the parameterization. It's only used in 2-3 places, and always with a cast involved anyway. It is therefore easy to remove the parameterization of Operation by replacing the type parameter with Action. This makes it necessary to cast the Action instead of the Operation when you want something specific from it. Here's a summary of all changes required to completely remove the parameterization:

1       1       src/main/java/gumtree/spoon/builder/jsonsupport/OperationNodePainter.java
1       1       src/main/java/gumtree/spoon/diff/ActionClassifier.java
2       2       src/main/java/gumtree/spoon/diff/DiffImpl.java
2       2       src/main/java/gumtree/spoon/diff/operations/AdditionOperation.java
1       1       src/main/java/gumtree/spoon/diff/operations/DeleteOperation.java
1       1       src/main/java/gumtree/spoon/diff/operations/InsertOperation.java
1       1       src/main/java/gumtree/spoon/diff/operations/MoveOperation.java
4       4       src/main/java/gumtree/spoon/diff/operations/Operation.java
1       1       src/main/java/gumtree/spoon/diff/operations/UpdateOperation.java
1       1       src/test/java/gumtree/spoon/TreeTest.java

Obviously, this is breaking to clients, and so for that reason it's not desirable until a major revision of gumtree-spoon.

Actually utilizing the parameterized types; especially for clients!

This is harder. With the current API, clients benefit little from the parameterization of Operation. There are casts required anyway, and so the parameterization doesn't add anything but complexity. Perhaps it could be useful if the API was augmented to specifically return e.g. UpdateOperation. Something with a method like this on Diff:

	@Override
	public <T extends Operation<?>> List<T> getRootOperations(Class<T> operationClass) {
		return getRootOperations().stream()
				.filter(op -> operationClass.isAssignableFrom(op.getClass()))
				.map(operationClass::cast)
				.collect(Collectors.toList());
	}

Used like so:

// call to <T  List<T extends Operation<
List<UpdateOperation> updates = diff.getRootOperations(UpdateOperation.class);

That would possibly make the parameterization worthwile, as end-users can then avoid casting when they need a specific type of operation. It would also be simple to adapt DiffImpl to avoid the cast in gumtree-spoon as well. And to boot, it would be backwards-compatible.

@martinezmatias Any thoughts on this?

[algomaster99]

I completely missed this comment over the multiple updates in diffmin this week.

I meant this <? extends Operation<?>>. I thought this would remove the warnings if we make this the return type of variable here.

Perhaps it could be useful if the API was augmented to specifically return e.g. UpdateOperation

But this still wouldn't solve the problem. We would still have to suppress the use of rawTypes to get all root operations at once because this method would just be adding a new API which diffmin doesn't require. diffmin needs an API only for getting all root operations at once.

[slarse]

I meant this extends Operation>>. I thought this would remove the warnings if we make this the return type of variable here.

That is a private instance variable, it's not part of the API. It's the return types of getOperations and getRootOperations that need to be parameterized, but in order to avoid unsafe casts inside of gumtree-spoon, there's quite a few changes that need to be made. And all other raw types used in the Diff interface as well.

We would still have to suppress the use of rawTypes to get all root operations at once because this method would just be adding a new API which diffmin doesn't require.

It's meant as a justification for keeping the parameterization, because right now it does not seem to add anything for the end user as casts are required anyway. Going with this idea would also entail fixing the raw types in the API.

diffmin needs an API only for getting all root operations at once.

I disagree. Diffmin does different things depending on the type of operation. All instance type checks could be removed from this loop if it was possible to specifically get one kind of operation. Then it would be simplified to:

for (UpdateOperation update : diff.getRootOperations(UpdateOperation.class) {
    // add update
}
for (InsertOperation insert : diff.getRootOperations(InsertOperation.class) {
    // add insert
}
[ ... etc etc ... ]

To me, that's cleaner code, as it avoids both type checking and type casting.

[algomaster99]

but in order to avoid unsafe casts inside of gumtree-spoon

For this, I was suggesting to change the return type of https://github.com/SpoonLabs/gumtree-spoon-ast-diff/blob/3e392ca49e09a43eb48e618d54bf98a2b97fb99e/src/main/java/gumtree/spoon/diff/DiffImpl.java#L88 to List<? extends Operation<?>>. It would still require the casts but I think it would remove the warnings. Of course, eliminating casts would be best altogether.

All instance type checks could be removed from this loop

Seems like a better idea to me too! And since we know that theoretically the order of applying patches should not matter, this would not pose a problem as well.