You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/03/22 23:47:50 UTC
[GitHub] [lucene] rmuir opened a new pull request #34: LUCENE-9856: fail precommit on unused local variables
rmuir opened a new pull request #34:
URL: https://github.com/apache/lucene/pull/34
See the JIRA for more details.
This turns on ecj option: `ecj.javadocs.prefs:org.eclipse.jdt.core.compiler.problem.unusedLocal=error`
But some modules have some generated code, and there is no way to `SuppressWarnings` or otherwise exclude specific files, that I know of. So we disable the option for those modules for now, at least to enforce it everywhere else.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599482948
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/core/TestAnalyzers.java
##########
@@ -109,7 +109,9 @@ public void _testStandardConstants() {
x = StandardTokenizer.HIRAGANA;
x = StandardTokenizer.KATAKANA;
x = StandardTokenizer.HANGUL;
+ assert x >= 0;
Review comment:
the entire test is crazy, recommend looking at it in context. It is a "compile-time" "test" to ensure the constants are "public". We can remove the asserts, now that `SuppressWarnings("unused")` does the right thing
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599486860
##########
File path: lucene/core/src/test/org/apache/lucene/search/TestTotalHits.java
##########
@@ -22,6 +22,7 @@
public class TestTotalHits extends LuceneTestCase {
+ @SuppressWarnings("unlikely-arg-type")
Review comment:
This one detects `equals(typeA, typeB)` where typeA is different than typeB.
In `src/java` every single thing it found was a bug (fixed in this PR). Stuff like code calling equals(BytesRefBuilder, BytesRef), which probably was broken during a refactoring.
In `src/test` it also found similar bugs. But there are a few tests that intentionally do something like `assertFalse(myObj.equals("a string or something"))` to test their `equals` method on a different class.
only a few tests needed the suppression, so I just added it to those equals tests.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-805065787
When you fixed the stuff, maybe reopen a new pull request with same branch?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804487699
OK i reverted the gradle changes completely, i found a secret option so that `SuppressWarnings` can be used. This fixes the expressions module, but we need to apply `SuppressWarnings("unused")` to some of the queryparser generated code. I am looking into it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599500002
##########
File path: gradle/generation/javacc.gradle
##########
@@ -95,6 +95,12 @@ def commonCleanups = { FileTree generatedFiles ->
text = text.replace(
"public void setDebugStream(java.io.PrintStream ds) { debugStream = ds; }",
"// (setDebugStream omitted).")
+ text = text.replace(
+ "public class QueryParserTokenManager ",
+ "@SuppressWarnings(\"unused\") public class QueryParserTokenManager ")
Review comment:
That's fine. I generally try to avoid double quotes in groovy, because I hate the case that it expands variables inside. Personally, I see languages expanding variables inside strings as a security risk.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599501947
##########
File path: gradle/generation/javacc.gradle
##########
@@ -95,6 +95,12 @@ def commonCleanups = { FileTree generatedFiles ->
text = text.replace(
"public void setDebugStream(java.io.PrintStream ds) { debugStream = ds; }",
"// (setDebugStream omitted).")
+ text = text.replace(
+ "public class QueryParserTokenManager ",
+ "@SuppressWarnings(\"unused\") public class QueryParserTokenManager ")
Review comment:
I will fix it here, but we must wait a bit. This change requires regenerating the huge DFA :(
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599514548
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
/** Test that bogus arguments result in exception */
public void testBogusArguments() throws Exception {
- IllegalArgumentException expected =
Review comment:
Obviously it comes from copy'n'paste = shit'n'waste. We have many places in code where the exception's message is checked. Code like this one here does not do it. Because this syntax with the lambda for exception checking was "new" at this point in time lot of people shit'n'wasted. 😱
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] zacharymorn edited a comment on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
zacharymorn edited a comment on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-805464492
> > Is the plan to have both ecj & error prone to co-exist in Lucene to catch different kinds of bugs?
>
> I think the union of all these tools is different than their intersection so they help catch issues, even if they do some duplicate work?
> > Thanks Robert for working on this! And yeah I didn't quite anticipate so much dead code lingering around when I created the issue! From your latest comment this PR is very much ready now? Any aspect I can still look into in this PR to help out as well at this point? (and sorry for coming in late in the discussion!)
>
> We may always followup with more checks. I mentioned a good one in my comment above, i'd really like to enforce that `@Override` annotation is used everywhere, to help prevent dead `public` and `protected` methods in the future?
>
> In the preferences file, we would set `org.eclipse.jdt.core.compiler.problem.missingOverrideAnnotation=error` and then fix all the problems. It would be good as a standalone followup issue
>
> > In addition, I have a newbie question. It appears error prone was integrated fairly recently this year https://issues.apache.org/jira/browse/LUCENE-9497, but the issue itself didn't seems to mention why error prone was needed when there's already ecj lint, and which one should be preferred for which scenario. Is the plan to have both ecj & error prone to co-exist in Lucene to catch different kinds of bugs?
>
> I don't know the error-prone well, but it is more of a "static analysis tool": I immediately think that it leans towards heuristics and "false positives".
>
> On the other hand, ECJ is a "compiler", but with a lot of nice checks. Maybe they are not as fancy as the kind of checks that error-prone might be able to do, instead they are simpler, more like "additional compiler warnings". We've used ECJ for a long time: It runs efficiently, has a TON more eyes looking at its output every day, and provides great error messages. IMO, for solving your problem of "my IDE can detect it is dead code, why doesn't the build", it is the perfect tool: since it is the thing doing that in your IDE (to my knowledge not just eclipse but intellij may use it too).
> > On the other hand, ECJ is a "compiler", but with a lot of nice checks. Maybe they are not as fancy as the kind of checks that error-prone might be able to do, instead they are simpler, more like "additional compiler warnings". We've used ECJ for a long time: It runs efficiently, has a TON more eyes looking at its output every day, and provides great error messages. IMO, for solving your problem of "my IDE can detect it is dead code, why doesn't the build", it is the perfect tool: since it is the thing doing that in your IDE (to my knowledge not just eclipse but intellij may use it too).
>
> I fully agree with that. I was also thinking about further improving compile time in Java by using ECJ as the official compiler and move it from "Lint" stage. We can maybe reimplement compileJava tasks in Gradle to use ECJ (maybe it works out of box!?). So people would see those errors as soon as they compile or run tests. ECJ is just an alternative compiler and may work as replacement for javac.
>
> This is no suggestion, but something to discuss on a separate issue. There's also a Grade plugin to do this: https://github.com/TwoStone/gradle-eclipse-compiler-plugin (and it allows to supply the properties file with warning/error config).
Thanks for the explanation and insight on ecj vs. error prone. They make a lot of sense! To keep the fun going, I have opened another jira issue (https://issues.apache.org/jira/browse/LUCENE-9864) to enforce the use of `@Override` with `org.eclipse.jdt.core.compiler.problem.missingOverrideAnnotation=error` as suggested, and will try to see if I can get a PR out soon.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599491781
##########
File path: gradle/generation/javacc.gradle
##########
@@ -95,6 +95,12 @@ def commonCleanups = { FileTree generatedFiles ->
text = text.replace(
"public void setDebugStream(java.io.PrintStream ds) { debugStream = ds; }",
"// (setDebugStream omitted).")
+ text = text.replace(
+ "public class QueryParserTokenManager ",
+ "@SuppressWarnings(\"unused\") public class QueryParserTokenManager ")
Review comment:
I opened https://issues.apache.org/jira/browse/LUCENE-9863 to fix this
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804870224
I was also pleasantly surprised to see how well maintained this compiler is. It is nice to see things like JDK16 support, as opposed to error-prone which reaches into java internals and seemingly has no plan for JDK16 support at all! Also, they seem to pay attention to the performance especially, as it impacts a lot of IDE users. You can see this from the commits: https://github.com/eclipse/eclipse.jdt.core
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599506794
##########
File path: gradle/generation/javacc.gradle
##########
@@ -95,6 +95,12 @@ def commonCleanups = { FileTree generatedFiles ->
text = text.replace(
"public void setDebugStream(java.io.PrintStream ds) { debugStream = ds; }",
"// (setDebugStream omitted).")
+ text = text.replace(
+ "public class QueryParserTokenManager ",
+ "@SuppressWarnings(\"unused\") public class QueryParserTokenManager ")
Review comment:
And i had to close a bunch of apps and restart it already :)
`Mar 23 07:59:25 kernel: Out of memory: Killed process 410597 (java) total-vm:15959272kB, anon-rss:11760000kB, file-rss:0kB, shmem-rss:32kB, UID:1000 pgtables:23356kB oom_score_adj:0`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] zacharymorn commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-805464492
> > Is the plan to have both ecj & error prone to co-exist in Lucene to catch different kinds of bugs?
>
> I think the union of all these tools is different than their intersection so they help catch issues, even if they do some duplicate work?
> > Thanks Robert for working on this! And yeah I didn't quite anticipate so much dead code lingering around when I created the issue! From your latest comment this PR is very much ready now? Any aspect I can still look into in this PR to help out as well at this point? (and sorry for coming in late in the discussion!)
>
> We may always followup with more checks. I mentioned a good one in my comment above, i'd really like to enforce that `@Override` annotation is used everywhere, to help prevent dead `public` and `protected` methods in the future?
>
> In the preferences file, we would set `org.eclipse.jdt.core.compiler.problem.missingOverrideAnnotation=error` and then fix all the problems. It would be good as a standalone followup issue
>
> > In addition, I have a newbie question. It appears error prone was integrated fairly recently this year https://issues.apache.org/jira/browse/LUCENE-9497, but the issue itself didn't seems to mention why error prone was needed when there's already ecj lint, and which one should be preferred for which scenario. Is the plan to have both ecj & error prone to co-exist in Lucene to catch different kinds of bugs?
>
> I don't know the error-prone well, but it is more of a "static analysis tool": I immediately think that it leans towards heuristics and "false positives".
>
> On the other hand, ECJ is a "compiler", but with a lot of nice checks. Maybe they are not as fancy as the kind of checks that error-prone might be able to do, instead they are simpler, more like "additional compiler warnings". We've used ECJ for a long time: It runs efficiently, has a TON more eyes looking at its output every day, and provides great error messages. IMO, for solving your problem of "my IDE can detect it is dead code, why doesn't the build", it is the perfect tool: since it is the thing doing that in your IDE (to my knowledge not just eclipse but intellij may use it too).
> > On the other hand, ECJ is a "compiler", but with a lot of nice checks. Maybe they are not as fancy as the kind of checks that error-prone might be able to do, instead they are simpler, more like "additional compiler warnings". We've used ECJ for a long time: It runs efficiently, has a TON more eyes looking at its output every day, and provides great error messages. IMO, for solving your problem of "my IDE can detect it is dead code, why doesn't the build", it is the perfect tool: since it is the thing doing that in your IDE (to my knowledge not just eclipse but intellij may use it too).
>
> I fully agree with that. I was also thinking about further improving compile time in Java by using ECJ as the official compiler and move it from "Lint" stage. We can maybe reimplement compileJava tasks in Gradle to use ECJ (maybe it works out of box!?). So people would see those errors as soon as they compile or run tests. ECJ is just an alternative compiler and may work as replacement for javac.
>
> This is no suggestion, but something to discuss on a separate issue. There's also a Grade plugin to do this: https://github.com/TwoStone/gradle-eclipse-compiler-plugin (and it allows to supply the properties file with warning/error config).
Thanks for the explanation and insight on ecj vs. error prone. They make a lot of sense! To keep the fun going, I have opened another Jira to enforce the use of `@Override` with `org.eclipse.jdt.core.compiler.problem.missingOverrideAnnotation=error` suggested, and will try to see if I can get a PR out soon.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599502396
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
/** Test that bogus arguments result in exception */
public void testBogusArguments() throws Exception {
- IllegalArgumentException expected =
Review comment:
So I just mentioned it here, so people that may complain are aware that we "noticed" that intention.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599489397
##########
File path: gradle/generation/javacc.gradle
##########
@@ -95,6 +95,12 @@ def commonCleanups = { FileTree generatedFiles ->
text = text.replace(
"public void setDebugStream(java.io.PrintStream ds) { debugStream = ds; }",
"// (setDebugStream omitted).")
+ text = text.replace(
+ "public class QueryParserTokenManager ",
+ "@SuppressWarnings(\"unused\") public class QueryParserTokenManager ")
Review comment:
I knew this would come up: but don't see single quotes being used much in our gradle build.. is this really consensus?
Why do we run spotless across the java code not but the groovy code too which is an even crazier language?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599501850
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
/** Test that bogus arguments result in exception */
public void testBogusArguments() throws Exception {
- IllegalArgumentException expected =
Review comment:
This was just a general comment about "why do people this?". If it was you, no problem. It just looked like "I want debugging, so I assign everything to a local variable".
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir merged pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir merged pull request #34:
URL: https://github.com/apache/lucene/pull/34
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804474683
This change is largely mechanical: simply remove the unused local variables. Try to preserve any method calls that may have side effects (e.g. throw exception) in tests.
In general, it is usually a pretty bad smell to have such unused variables: the idea is not to go sniffing around that and debug deeply, but just to get the stuff fixed so the linter can stop new stuff from coming in.
If you want to help fix the issues (or make the gradle better!!!), please, feel free to just push commits to the branch!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599417385
##########
File path: lucene/core/src/test/org/apache/lucene/search/TestTotalHits.java
##########
@@ -22,6 +22,7 @@
public class TestTotalHits extends LuceneTestCase {
+ @SuppressWarnings("unlikely-arg-type")
Review comment:
Just for interest: what does this complain about?
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/core/TestAnalyzers.java
##########
@@ -109,7 +109,9 @@ public void _testStandardConstants() {
x = StandardTokenizer.HIRAGANA;
x = StandardTokenizer.KATAKANA;
x = StandardTokenizer.HANGUL;
+ assert x >= 0;
Review comment:
Should't this be a real assertTrue() of JUnit?
##########
File path: lucene/core/src/test/org/apache/lucene/store/TestMultiMMap.java
##########
@@ -181,7 +181,7 @@ public void testSeekZero() throws Exception {
public void testSeekSliceZero() throws Exception {
int upto = TEST_NIGHTLY ? 31 : 3;
- for (int i = 0; i < 3; i++) {
+ for (int i = 0; i < upto; i++) {
Review comment:
woah, thanks for fixing!
##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/miscellaneous/ConcatenateGraphFilter.java
##########
@@ -348,7 +348,7 @@ private static Automaton replaceSep(Automaton a, Character tokenSeparator) {
* @lucene.internal
*/
public static final class BytesRefBuilderTermAttributeImpl extends AttributeImpl
- implements BytesRefBuilderTermAttribute, TermToBytesRefAttribute {
+ implements BytesRefBuilderTermAttribute {
Review comment:
LOL
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
/** Test that bogus arguments result in exception */
public void testBogusArguments() throws Exception {
- IllegalArgumentException expected =
Review comment:
I think those are indeed unused, but the reason why they are there is a different one: This allows step-by-step debugging. By declaring an unused variable, you can step through the code / set breakpoints and see what the variable contains. Without it, most debuggers don't show the return value (depend on your IDE).
I agree to remove them, but this may annoy people.
##########
File path: gradle/generation/javacc.gradle
##########
@@ -95,6 +95,12 @@ def commonCleanups = { FileTree generatedFiles ->
text = text.replace(
"public void setDebugStream(java.io.PrintStream ds) { debugStream = ds; }",
"// (setDebugStream omitted).")
+ text = text.replace(
+ "public class QueryParserTokenManager ",
+ "@SuppressWarnings(\"unused\") public class QueryParserTokenManager ")
Review comment:
Hi, To get rid of the stupid escapes for quotes, use single quotes around:
```
'@SuppressWarnings("unused") public class QueryParserTokenManager'
```
IMHO I like single quoes in Groovy more, as they don't do expansion of properties/variables inside. I always use single quotes in Grooy to prevent expansion :-)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804866653
> On the other hand, ECJ is a "compiler", but with a lot of nice checks. Maybe they are not as fancy as the kind of checks that error-prone might be able to do, instead they are simpler, more like "additional compiler warnings". We've used ECJ for a long time: It runs efficiently, has a TON more eyes looking at its output every day, and provides great error messages. IMO, for solving your problem of "my IDE can detect it is dead code, why doesn't the build", it is the perfect tool: since it is the thing doing that in your IDE (to my knowledge not just eclipse but intellij may use it too).
I fully agree with that. I was also thinking about further improving compile time in Java by using ECJ as the official compiler and move it from "Lint" stage. We can maybe reimplement compileJava tasks in Gradle to use ECJ (maybe it works out of box!?). So people would see those errors as soon as they compile or run tests. ECJ is just an alternative compiler and may work as replacement for javac.
This is no suggestion, but something to discuss on a separate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler edited a comment on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler edited a comment on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804866653
> On the other hand, ECJ is a "compiler", but with a lot of nice checks. Maybe they are not as fancy as the kind of checks that error-prone might be able to do, instead they are simpler, more like "additional compiler warnings". We've used ECJ for a long time: It runs efficiently, has a TON more eyes looking at its output every day, and provides great error messages. IMO, for solving your problem of "my IDE can detect it is dead code, why doesn't the build", it is the perfect tool: since it is the thing doing that in your IDE (to my knowledge not just eclipse but intellij may use it too).
I fully agree with that. I was also thinking about further improving compile time in Java by using ECJ as the official compiler and move it from "Lint" stage. We can maybe reimplement compileJava tasks in Gradle to use ECJ (maybe it works out of box!?). So people would see those errors as soon as they compile or run tests. ECJ is just an alternative compiler and may work as replacement for javac.
This is no suggestion, but something to discuss on a separate issue. There's also a Grade plugin to do this: https://github.com/TwoStone/gradle-eclipse-compiler-plugin (and it allows to supply the properties file with warning/error config).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804606020
ok, I think ecj linter is now in better shape, especially as far as dead code goes.
But wow, the amount of dead `private` members and methods that we had was really surprising.
We should followup by enabling the check to enforce `@Override` annotations, so that when public/protected methods get removed, dangling bits aren't left around. That's a good way to improve it to the future. But, another issue. I am tired.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] zacharymorn commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
zacharymorn commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804683838
Thanks Robert for working on this! And yeah I didn't quite anticipate so much dead code lingering around when I created the issue! From your latest comment this PR is very much ready now? Any aspect I can still look into in this PR to help out as well at this point? (and sorry for coming in late in the discussion!)
In addition, I have a newbie question. It appears error prone was integrated fairly recently this year https://issues.apache.org/jira/browse/LUCENE-9497, but the issue itself didn't seems to mention why error prone was needed when there's already ecj lint, and which one should be preferred for which scenario. Is the plan to have both ecj & error prone to co-exist in Lucene to catch different kinds of bugs?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-805086733
I still have the branch locally. I think the fix is to revert this commit: https://github.com/apache/lucene/pull/34/commits/7d823c84e3f7b6bebd25eda76f9e482016c78868
Redundant superinterface was too aggressive on my part. I was trying to enable checks around "unnecessary code" and this was one of them, but I think it borked the attributes? Seems it should be deferred for now, the gain isn't worth the pain :)
We may still enable the `unusedLabel` check from that commit, I think it is safe.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599482236
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
/** Test that bogus arguments result in exception */
public void testBogusArguments() throws Exception {
- IllegalArgumentException expected =
Review comment:
I am pretty sure I added this test and I don't use debuggers. If someone wants unused, they can add `SuppressWarnings("unused")` to their test.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804859947
> Thanks Robert for working on this! And yeah I didn't quite anticipate so much dead code lingering around when I created the issue! From your latest comment this PR is very much ready now? Any aspect I can still look into in this PR to help out as well at this point? (and sorry for coming in late in the discussion!)
>
We may always followup with more checks. I mentioned a good one in my comment above, i'd really like to enforce that `@Override` annotation is used everywhere, to help prevent dead `public` and `protected` methods in the future?
In the preferences file, we would set `org.eclipse.jdt.core.compiler.problem.missingOverrideAnnotation=error` and then fix all the problems. It would be good as a standalone followup issue
> In addition, I have a newbie question. It appears error prone was integrated fairly recently this year https://issues.apache.org/jira/browse/LUCENE-9497, but the issue itself didn't seems to mention why error prone was needed when there's already ecj lint, and which one should be preferred for which scenario. Is the plan to have both ecj & error prone to co-exist in Lucene to catch different kinds of bugs?
I don't know the error-prone well, but it is more of a "static analysis tool": I immediately think that it leans towards heuristics and "false positives".
On the other hand, ECJ is a "compiler", but with a lot of nice checks. Maybe they are not as fancy as the kind of checks that error-prone might be able to do, instead they are simpler, more like "additional compiler warnings". We've used ECJ for a long time: It runs efficiently, has a TON more eyes looking at its output every day, and provides great error messages. IMO, for solving your problem of "my IDE can detect it is dead code, why doesn't the build", it is the perfect tool: since it is the thing doing that in your IDE (to my knowledge not just eclipse but intellij may use it too).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599502396
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
/** Test that bogus arguments result in exception */
public void testBogusArguments() throws Exception {
- IllegalArgumentException expected =
Review comment:
So I just mentioned it here, so people that may complain are aware that we "noticed" that behaviour.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
dweiss commented on pull request #34:
URL: https://github.com/apache/lucene/pull/34#issuecomment-804690276
> Is the plan to have both ecj & error prone to co-exist in Lucene to catch different kinds of bugs?
I think the union of all these tools is different than their intersection so they help catch issues, even if they do some duplicate work?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #34: LUCENE-9856: fail precommit on unused local variables
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #34:
URL: https://github.com/apache/lucene/pull/34#discussion_r599507851
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/cjk/TestCJKBigramFilterFactory.java
##########
@@ -56,11 +56,10 @@ public void testHanOnlyUnigrams() throws Exception {
/** Test that bogus arguments result in exception */
public void testBogusArguments() throws Exception {
- IllegalArgumentException expected =
Review comment:
I honestly don't think there is a lot of the behavior going on. Maybe because IDEs tend to issue warnings when you do this?
We fixed only about 140 violations for this check total across the whole codebase including tests.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org