Fix extra space added in try with resources

[neilcsmith-net]

Fix for #3720

The space seems to occur due to the start and end positions reported for the implicit hidden final modifier, which seem to be the same as the type start and end. The issue can't be seen if final is added explicitly.

This adds an extra check for whether the modifiers and type start at the same position. I haven't checked if the reported positions of mods have changed from earlier versions. The might explain why the issue started appearing, and it's possible the whole enclosing if wasn't hit in that case. I've kept localized for now.

The issue with new lines being removed may also be related to this, but haven't managed to track down exactly what's happening - it's possibly related to the token locations. Leaving that for another fix.

[mbien]

the implicit final would have the same start position as the type (which is the next item in the tree I guess) because its hidden?

I am wondering if this is just an impl detail or actually stable behavior, because there would be also the NOPOS constant
https://github.com/openjdk/jdk/blob/05d38604a2c620dcaf8682f02dae2fddab8e0c0b/src/jdk.compiler/share/classes/com/sun/source/util/SourcePositions.java#L41-L43
which could be potentially returned in future?

if implicit items have the same start and end pos we could extract this into:

private static boolean isHidden(CompilationUnitTree cut, Tree tree) {
    long start = sp.getStartPosition(cut, tree);
    long end = sp.getEndPosition(cut, tree);
    return start == end && start != javax.tools.Diagnostic.NOPOS;
}

Maybe that would be less likely to break on a javac update (assuming it works)?

[neilcsmith-net]

if implicit items have the same start and end pos

Thanks @mbien but this hits on the crux of the issue. The implicit final does not have the same start and end position (zero length), or report NOPOS. It reports the same start and end positions as the following type element. It has the same length and overlaps.

Were the element reported empty or at no position, it shouldn't get past the enclosing check at https://github.com/apache/netbeans/blob/master/java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java#L1146 I assume that the reported end position changed which started causing this issue, but haven't checked old versions to be sure.

[neilcsmith-net]

Bizarrely I also think that #4667 might be caused by this same issue.

[mbien]

thats what i feared :(

This means we can hotfix this as you proposed, but it is possible that it might break again since the behavior is not really specified.

Maybe there is a chance for an upstream update @jlahoda? Another return-constant or possibly documented behavior of what is returned for "hidden" elements.

[neilcsmith-net]

Ironically @jlahoda might have fixed this today? https://bugs.openjdk.org/browse/JDK-8293897

The hotfix shouldn't break if this is fixed. And it's covered by tests now I think. Could add a comment to revert when upstream fixed?

[mbien]

The hotfix shouldn't break if this is fixed. And it's covered by tests now I think. Could add a comment to revert when upstream fixed?

Yeah I think so - lets get this in. The tests are indeed important so that we notice regressions quickly. Good that you found that bug report, it clears things up.

If the upstream fix makes it into javac 19.0.1 too, we could adjust/partially revert it later while upgrading nb-javac.

[mbien]

good hotfix, this will make many happy - can be potentially removed again once fixed upstream in the JDK as discussed in the review comments.

[neilcsmith-net]

Thanks @mbien - merging.