SourcePositions are incorrect in some cases

[pvojtechovsky]

In the PR #1950 I added check for consistency of source position start/end offsets:

class BodyHolderSourcePositionImpl
	public BodyHolderSourcePositionImpl(...) {
		super(...);
		checkArgsAreAscending(declarationSourceStart, modifierSourceStart, modifierSourceEnd + 1, sourceStart, sourceEnd + 1, bodyStart, bodyEnd + 1, declarationSourceEnd + 1);
...
}
...
protected static void checkArgsAreAscending(int...values) {
	int last = -1;
	for (int value : values) {
		if (value < 0) {
			throw new SpoonException("SourcePosition value must not be negative");
		}
		if (last > value) {
			throw new SpoonException("SourcePosition values must be ascending or equal");
		}
		last = value;
	}
}

It is needed for sniper mode (source code refactored by spoon has minimum differences comparing to origin source code).

But this SourcePosition consistency check causes that many tests fail now.

[tdurieux]

I think it is mostly due to PartialSourcePositionImpl that have negative values.
That is used in CompilationUnitFactory.getOrCreate(...)

[tdurieux]

I tried to fix some of the issues, I have now only 10 tests that do not succeed.
But I found this case that I think we will not be able to fix:

@GlobalAnnotation
public @Deprecated final @TypeAnnotation Void m() {
}

Because getModifierSourceStart and getModifierSourceEnd mean nothing in this case, because there is the annotation between them. Should we move the position of the modifiers in CtExtendedModifier?

[monperrus]

Can we change the convention to MAX_VALUE?

[monperrus]

We can do so. Would it be backward compatible?

[pvojtechovsky]

Source position section of modifiers already contains annotations in other cases ... and I am OK with that personally. Only the name of this section is confusing. Otherwise it is OK. The types behaves same. E.g this one:

public @Deprecated class Xxx {}

And many thanks Thomas for fixing that! I have to learn to create more issues ... and not to do it on my own.... looks like we are not only 3 on github ;-))

[tdurieux]

I made all the test pass.
But I still need to make nice PRs and some test.
I added the position on each modifier.

I put in modifiersSourceStart and modifiersSourceEnd the min and the max position of the modifiers.
What do you think?
I think it can be some issue with there are different annotation or comments between modifiers. Like in this example where the modifiersSourceStart and modifiersSourceEnd will content public @Deprecated final and not @GlobalAnnotation public @Deprecated final @TypeAnnotation

@GlobalAnnotation
public @Deprecated final @TypeAnnotation Void m() {
}

[pvojtechovsky]

I put in modifiersSourceStart and modifiersSourceEnd the min and the max position of the modifiers.
What do you think?

You cannot avoid mixing of modifiers and annotations, so may be the solution is simply to document that source fragment of modifiers contains all modifiers including all annotations and keep the code as it is. I think that is good solution and it is backward compatible because it already behaves like that on other places.

[tdurieux]

The issue is that for the moment, it doesn't not include all the modifiers

[pvojtechovsky]

The issue is that for the moment, it doesn't not include all the modifiers

Can I debug it somewhere? Which test, which branch?

[tdurieux]

Sorry I meant annotations.

But currently with this code the modifier final is not inluded in the interval.

@GlobalAnnotation
public @Deprecated final Void m() {
}

[pvojtechovsky]

Thanks for explanation. I understand.
I would like if "@GlobalAnnotation\npublic @deprecated final" would be included in the interval of modifiers. OK ?

[tdurieux]

Do you mean that you would prefer to include all the annotations in the interval?

Even in this case @annot public void m(){}?

[pvojtechovsky]

Do you mean that you would prefer to include all the annotations in the interval?

yes exactly. Even in that case. It already behaves like this. It fits to needs of #1927

[pvojtechovsky]

I have checked the code and I was wrong. I have found the PositionBuilder code which tries to search for place where annotations ends and modifiers start. So Spoon actually tries to separated annotations and modifiers (of type and method) into different source fragments.

So my sentences in the comments above: "It already behaves like this." are wrong.

But I still think that we cannot separate annotations and modifiers, because they can be mixed in arbitrary order, so the only reliable solution is to have them in one one shared "modifiers" source interval. May be we can rename getModifiersStart/End to getModifiersAndAnnotationsStart/End ... so it is clear what is inside.

[tdurieux]

I think it is the same with comments :/

[pvojtechovsky]

yes and no. Comments are special. They behaves like a whitespace in source code.

Note:

• comments can be everywhere - it is clear that comments are everywhere, so all algorithms which wants to handle them must count with that
• annotations can be only in source fragment modifiers - so we have to document it in SourcePosition and adapt the code little bit...

[monperrus]

can we close this issue (the title is not really focused)?

[pvojtechovsky]

ok, I agree to close it. There are still probably some Source position problems, but it makes sense to create individual issues for them. I will do so in next days after checking the state of sniper mode and related failing tests