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/26 00:49:27 UTC
[GitHub] [lucene] rmuir opened a new pull request #44: LUCENE-9878: enable redundantNullCheck in ecjLint
rmuir opened a new pull request #44:
URL: https://github.com/apache/lucene/pull/44
Detects common cases of unreachable/dead code.
Don't be confused by the name, this isn't enabling annotation-based null analyzer or discouraging safety checks in the code.
This is simple flow analysis to detect cases like the following:
```
foo = null;
if (foo != null) {
// provably dead code
}
```
```
foo.invokeMethod();
if (foo != null) {
// provably dead code, would have thrown exc
}
```
I reviewed all the errors, and the check seems productive. I think a lot of this stuff happens as the code "ages" over time. Lots of WTFs It even found what appears to be a seriously broken `equals()` implementation.
For generated javacc code, the check is disabled via `SuppressWarnings("unused")` because javacc generates strange/bad code such as:
```
if ("" == null)
```
For TestStressNRTReplication's `startNode()` method, the check is also disabled because analysis folds the "test evilness controls" which are static final constants. This itself is a WTF, shouldn't we instead randomize these evil things in our tests rather than hardcoding them to specific values?
--
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] mikemccand commented on a change in pull request #44: LUCENE-9878: enable redundantNullCheck in ecjLint
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #44:
URL: https://github.com/apache/lucene/pull/44#discussion_r602729909
##########
File path: lucene/grouping/src/java/org/apache/lucene/search/grouping/SearchGroup.java
##########
@@ -143,9 +143,9 @@ public boolean equals(Object _other) {
if (_other instanceof MergedGroup) {
MergedGroup<?> other = (MergedGroup<?>) _other;
if (groupValue == null) {
- return other == null;
+ return other.groupValue == null;
Review comment:
Wow, this is an awesome catch and shows how useful static analysis is!!
--
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 #44: LUCENE-9878: enable redundantNullCheck in ecjLint
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #44:
URL: https://github.com/apache/lucene/pull/44#discussion_r601934375
##########
File path: lucene/grouping/src/java/org/apache/lucene/search/grouping/SearchGroup.java
##########
@@ -143,9 +143,9 @@ public boolean equals(Object _other) {
if (_other instanceof MergedGroup) {
MergedGroup<?> other = (MergedGroup<?>) _other;
if (groupValue == null) {
- return other == null;
+ return other.groupValue == null;
Review comment:
I think this stuff was a real bug, causing `equals` to basically always return false. I don't know how it is used by this thing (caching or performance or whatever). Detector knows that `other == _other` and that `_other instanceof MergedGroup`, hence it simply cannot be null here.
But the bigger issue is that we should be comparing against `other.groupValue` in this 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.
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] mikemccand merged pull request #44: LUCENE-9878: enable redundantNullCheck in ecjLint
Posted by GitBox <gi...@apache.org>.
mikemccand merged pull request #44:
URL: https://github.com/apache/lucene/pull/44
--
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] mikemccand commented on a change in pull request #44: LUCENE-9878: enable redundantNullCheck in ecjLint
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #44:
URL: https://github.com/apache/lucene/pull/44#discussion_r602733991
##########
File path: lucene/core/src/java/org/apache/lucene/index/CheckIndex.java
##########
@@ -652,14 +651,6 @@ public Status checkIndex(List<String> onlySegments) throws IOException {
Sort indexSort = info.info.getIndexSort();
if (indexSort != null) {
msg(infoStream, " sort=" + indexSort);
- if (previousIndexSort != null) {
Review comment:
This is indeed dead code, but that's crazy! I think the intention here was to set `previousIndexSort = null` OUTSIDE the `for` loop, but that didn't happen, so this code is clearly dead.
--
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