You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2022/09/17 17:12:11 UTC
[GitHub] [netbeans] neilcsmith-net opened a new pull request, #4648: Fix extra space added in try with resources
neilcsmith-net opened a new pull request, #4648:
URL: https://github.com/apache/netbeans/pull/4648
Work in progress potential fix for #3720
The space seems to occur due to the positions reported for the implicit hidden final modifier. Surprisingly the reported end position isn't the same as the start position for a hidden modifier. Instead added extra check for whether the modifiers and type start at the same position. This seems to work - fun one to debug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on code in PR #4648:
URL: https://github.com/apache/netbeans/pull/4648#discussion_r976880281
##########
java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java:
##########
@@ -1151,7 +1151,7 @@ public Boolean visitVariable(VariableTree node, Void p) {
newline();
else
space();
- } else {
+ } else if (sp.getStartPosition(root, mods) != sp.getStartPosition(root, node.getType())) {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on code in PR #4648:
URL: https://github.com/apache/netbeans/pull/4648#discussion_r976184470
##########
java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java:
##########
@@ -1151,7 +1151,7 @@ public Boolean visitVariable(VariableTree node, Void p) {
newline();
else
space();
- } else {
+ } else if (sp.getStartPosition(root, mods) != sp.getStartPosition(root, node.getType())) {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4648:
URL: https://github.com/apache/netbeans/pull/4648#discussion_r975988809
##########
java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java:
##########
@@ -1151,7 +1151,7 @@ public Boolean visitVariable(VariableTree node, Void p) {
newline();
else
space();
- } else {
+ } else if (sp.getStartPosition(root, mods) != sp.getStartPosition(root, node.getType())) {
Review Comment:
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:
```java
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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4648:
URL: https://github.com/apache/netbeans/pull/4648#discussion_r976959934
##########
java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java:
##########
@@ -1151,7 +1151,7 @@ public Boolean visitVariable(VariableTree node, Void p) {
newline();
else
space();
- } else {
+ } else if (sp.getStartPosition(root, mods) != sp.getStartPosition(root, node.getType())) {
Review Comment:
>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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on a diff in pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4648:
URL: https://github.com/apache/netbeans/pull/4648#discussion_r976867040
##########
java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java:
##########
@@ -1151,7 +1151,7 @@ public Boolean visitVariable(VariableTree node, Void p) {
newline();
else
space();
- } else {
+ } else if (sp.getStartPosition(root, mods) != sp.getStartPosition(root, node.getType())) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net merged pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
neilcsmith-net merged PR #4648:
URL: https://github.com/apache/netbeans/pull/4648
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on PR #4648:
URL: https://github.com/apache/netbeans/pull/4648#issuecomment-1254822698
Thanks @mbien - merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #4648: Fix extra space added in try with resources
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on code in PR #4648:
URL: https://github.com/apache/netbeans/pull/4648#discussion_r976466550
##########
java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java:
##########
@@ -1151,7 +1151,7 @@ public Boolean visitVariable(VariableTree node, Void p) {
newline();
else
space();
- } else {
+ } else if (sp.getStartPosition(root, mods) != sp.getStartPosition(root, node.getType())) {
Review Comment:
Bizarrely I also think that #4667 might be caused by this same issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists