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 2023/01/01 14:47:19 UTC
[GitHub] [netbeans] jlahoda opened a new pull request, #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
jlahoda opened a new pull request, #5173:
URL: https://github.com/apache/netbeans/pull/5173
Recently, I ran into to issues with the unused element hint:
- the detection of unused package private elements sometimes causes problems, e.g. when the index is (for some reason) broken. Overall, it may not be possible to make this work 100%. Suggestion here is to add an option to disable the package private unused detection, with default "enabled".
- some code (OpenJDK, specifically), has private methods that are then looked up using `MethodHandles.Lookup`. The unused detection marks them as unused, which is not quite true. The idea here is to use the method names to (heuristically) try to find out which may be looked up using the `MH.Lookup`, and not mark them as unused.
---
**^Add meaningful description above**
By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
- are all your own work, and you have the right to contribute them.
- are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).
Please make sure (eg. `git log`) that all commits have a valid name and email address for you in the Author field.
If you're a first time contributor, see the Contributing guidelines for more information.
If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.
--
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 #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1223859565
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +619,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
+ }
+ return super.visitLiteral(node, p);
+ }
+
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree node, Void p) {
+ Element invoked = info.getTrees().getElement(new TreePath(getCurrentPath(), node.getMethodSelect()));
+ if (invoked != null && invoked.getEnclosingElement() == methodHandlesLookup && node.getArguments().size() > 0) {
+ ExpressionTree clazz = node.getArguments().get(0);
+ Element lookupType = null;
+ if (clazz.getKind() == Kind.MEMBER_SELECT) {
+ MemberSelectTree mst = (MemberSelectTree) clazz;
+ if (mst.getIdentifier().contentEquals("class")) {
+ lookupType = info.getTrees().getElement(new TreePath(new TreePath(getCurrentPath(), clazz), mst.getExpression()));
+ }
+ }
+ String lookupName = null;
+ if (node.getArguments().size() > 1) {
+ ExpressionTree name = node.getArguments().get(1);
+ if (name.getKind() == Kind.STRING_LITERAL) {
+ lookupName = (String) ((LiteralTree) name).getValue();
+ }
+ }
+ switch (invoked.getSimpleName().toString()) {
+ case "findStatic": case "findVirtual": case "findSpecial":
+ type2LookedUpMethods.computeIfAbsent(lookupType, t -> new HashSet<>()).add(lookupName);
Review Comment:
nitpick:
`type2LookedUpMethods.computeIfAbsent(lookupType, t -> new HashSet<>()).add(lookupName);`
->
`type2LookedUpMethods.putIfAbsent(lookupType, new HashSet<>()).add(lookupName);`
potentially clearer?
(multiple occurrences in file)
--
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] jlahoda commented on a diff in pull request #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "jlahoda (via GitHub)" <gi...@apache.org>.
jlahoda commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1111309179
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +619,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
Review Comment:
Sorry for late reply. Yes, this is for indirect lookups - if the lookup contains a method name, the `allStringLiterals` should not be used. This is all a heuristics (e.g. it never uses the `MethodType`, so if there are several private overloads, they will all be considered used, even those that will never be looked up). But, this is just a warning, I think it is fine to use sensible heuristics, and I am more fine with some false negatives (not found unused methods) than with false positives (used methods marked as unused).
If the code is in a recognized pattern (`$lookup.findSomething($className.class, ...)`, the class in which the lookup is performed will be taken into consideration. If the pattern is different, the type won't be considered, and more methods will be potentially marked as used. This is again mostly a heuristics that should find a relatively typical pattern.
This should all have zero effect on files which don't use `MethodHandles.Lookup`, which is a vast majority of files, and the proposed heuristics seemed like a reasonable tradeoff to me.
--
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 #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1223861628
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +619,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
+ }
+ return super.visitLiteral(node, p);
+ }
+
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree node, Void p) {
+ Element invoked = info.getTrees().getElement(new TreePath(getCurrentPath(), node.getMethodSelect()));
+ if (invoked != null && invoked.getEnclosingElement() == methodHandlesLookup && node.getArguments().size() > 0) {
+ ExpressionTree clazz = node.getArguments().get(0);
+ Element lookupType = null;
+ if (clazz.getKind() == Kind.MEMBER_SELECT) {
+ MemberSelectTree mst = (MemberSelectTree) clazz;
+ if (mst.getIdentifier().contentEquals("class")) {
+ lookupType = info.getTrees().getElement(new TreePath(new TreePath(getCurrentPath(), clazz), mst.getExpression()));
+ }
+ }
+ String lookupName = null;
+ if (node.getArguments().size() > 1) {
+ ExpressionTree name = node.getArguments().get(1);
+ if (name.getKind() == Kind.STRING_LITERAL) {
+ lookupName = (String) ((LiteralTree) name).getValue();
+ }
+ }
+ switch (invoked.getSimpleName().toString()) {
+ case "findStatic": case "findVirtual": case "findSpecial":
+ type2LookedUpMethods.computeIfAbsent(lookupType, t -> new HashSet<>()).add(lookupName);
Review Comment:
nevermind, I believe the lambda is there to allocate the AL lazily. My mistake.
--
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] matthiasblaesing commented on pull request #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#issuecomment-1481863503
@jlahoda should this be merged?
--
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] lbownik commented on a diff in pull request #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1059784644
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +615,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
+ }
+ return super.visitLiteral(node, p);
+ }
+
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree node, Void p) {
+ Element invoked = info.getTrees().getElement(new TreePath(getCurrentPath(), node.getMethodSelect()));
+ if (invoked != null && invoked.getEnclosingElement() == methodHandlesLookup && node.getArguments().size() > 0) {
+ ExpressionTree clazz = node.getArguments().get(0);
+ Element lookupType = null;
+ if (clazz.getKind() == Kind.MEMBER_SELECT) {
+ MemberSelectTree mst = (MemberSelectTree) clazz;
+ if (mst.getIdentifier().contentEquals("class")) {
+ lookupType = info.getTrees().getElement(new TreePath(new TreePath(getCurrentPath(), clazz), mst.getExpression()));
+ }
+ }
+ String lookupName = null;
+ if (node.getArguments().size() > 1) {
Review Comment:
lines 640 - 645 could be extracted to a separate method
--
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] matthiasblaesing commented on a diff in pull request #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1111316734
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +619,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
Review Comment:
Think you for the explanation. With that in mind it makes sense to me. I overlooked the combined effect of:
https://github.com/apache/netbeans/pull/5173/files/7c57e4b4d7faf2b094e67ffab43100182f4f45bb#diff-908c30e8d355bd77779d4bfffcd966667091f27f2ab26299889fcceb659478c6R643-R662
and
https://github.com/apache/netbeans/pull/5173/files/7c57e4b4d7faf2b094e67ffab43100182f4f45bb#diff-908c30e8d355bd77779d4bfffcd966667091f27f2ab26299889fcceb659478c6R297
--
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] matthiasblaesing commented on a diff in pull request #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1103689305
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +619,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
Review Comment:
The whole PR is conclusive to me, this part I don't understand. My initial thought was, that you are trying to get the strings used in lookups, but these are already covered by the explicit invocation in `visitMethodInvocation`. Or is this to find "indirect" lookups? If so, should this customizable?
If I'm right - does this only match the unqualified name of a method or a field?
--
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 #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1223861628
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +619,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
+ }
+ return super.visitLiteral(node, p);
+ }
+
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree node, Void p) {
+ Element invoked = info.getTrees().getElement(new TreePath(getCurrentPath(), node.getMethodSelect()));
+ if (invoked != null && invoked.getEnclosingElement() == methodHandlesLookup && node.getArguments().size() > 0) {
+ ExpressionTree clazz = node.getArguments().get(0);
+ Element lookupType = null;
+ if (clazz.getKind() == Kind.MEMBER_SELECT) {
+ MemberSelectTree mst = (MemberSelectTree) clazz;
+ if (mst.getIdentifier().contentEquals("class")) {
+ lookupType = info.getTrees().getElement(new TreePath(new TreePath(getCurrentPath(), clazz), mst.getExpression()));
+ }
+ }
+ String lookupName = null;
+ if (node.getArguments().size() > 1) {
+ ExpressionTree name = node.getArguments().get(1);
+ if (name.getKind() == Kind.STRING_LITERAL) {
+ lookupName = (String) ((LiteralTree) name).getValue();
+ }
+ }
+ switch (invoked.getSimpleName().toString()) {
+ case "findStatic": case "findVirtual": case "findSpecial":
+ type2LookedUpMethods.computeIfAbsent(lookupType, t -> new HashSet<>()).add(lookupName);
Review Comment:
please ignore, I the lambda is there to allocate the collection lazily. My mistake.
--
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 #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #5173:
URL: https://github.com/apache/netbeans/pull/5173#discussion_r1223861628
##########
java/java.editor.base/src/org/netbeans/modules/java/editor/base/semantic/UnusedDetector.java:
##########
@@ -597,5 +619,49 @@ private void handleDeclaration(TreePath path) {
}
}
}
+
+ @Override
+ public Void visitLiteral(LiteralTree node, Void p) {
+ if (node.getKind() == Kind.STRING_LITERAL) {
+ allStringLiterals.add((String) ((LiteralTree) node).getValue());
+ }
+ return super.visitLiteral(node, p);
+ }
+
+ @Override
+ public Void visitMethodInvocation(MethodInvocationTree node, Void p) {
+ Element invoked = info.getTrees().getElement(new TreePath(getCurrentPath(), node.getMethodSelect()));
+ if (invoked != null && invoked.getEnclosingElement() == methodHandlesLookup && node.getArguments().size() > 0) {
+ ExpressionTree clazz = node.getArguments().get(0);
+ Element lookupType = null;
+ if (clazz.getKind() == Kind.MEMBER_SELECT) {
+ MemberSelectTree mst = (MemberSelectTree) clazz;
+ if (mst.getIdentifier().contentEquals("class")) {
+ lookupType = info.getTrees().getElement(new TreePath(new TreePath(getCurrentPath(), clazz), mst.getExpression()));
+ }
+ }
+ String lookupName = null;
+ if (node.getArguments().size() > 1) {
+ ExpressionTree name = node.getArguments().get(1);
+ if (name.getKind() == Kind.STRING_LITERAL) {
+ lookupName = (String) ((LiteralTree) name).getValue();
+ }
+ }
+ switch (invoked.getSimpleName().toString()) {
+ case "findStatic": case "findVirtual": case "findSpecial":
+ type2LookedUpMethods.computeIfAbsent(lookupType, t -> new HashSet<>()).add(lookupName);
Review Comment:
nevermind, I believe the lambda is there to allocate the collection lazily. My mistake.
--
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] jlahoda merged pull request #5173: Adding an option to disable unused hint for package private elements, ignoring elements that possibly are looked up using MethodHandles.Lookup.
Posted by "jlahoda (via GitHub)" <gi...@apache.org>.
jlahoda merged PR #5173:
URL: https://github.com/apache/netbeans/pull/5173
--
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