Skip to content

Conversation

@tyronecai
Copy link

@tyronecai tyronecai commented Feb 12, 2026

Description

because of variable shadowing, an npe may produce here. detected by Intellj ide.

final Counter bytesUsed = bytesStartArray.bytesUsed();
this.bytesUsed = bytesUsed == null ? Counter.newCounter() : bytesUsed;
bytesUsed.addAndGet(hashSize * (long) Integer.BYTES);
~~~~~~

ref: #15701

apache#15701

because of variable shadowing, an npe may produce here
Copy link
Contributor

@dweiss dweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we never hit this NPE... maybe it's just not possible and we should remove the code entirely and replace it with an assertion that bytesUsed != null?

@dweiss dweiss added the skip-changelog Apply to PRs that don't need a changelog entry, stopping the automated changelog check. label Feb 12, 2026
@tyronecai
Copy link
Author

tyronecai commented Feb 12, 2026

I wonder why we never hit this NPE... maybe it's just not possible and we should remove the code entirely and replace it with an assertion that bytesUsed != null?

Is it necessary to assume that bytesStartArray.bytesUsed() never null ?

just a variable shadowing issue introduced by https://github.com/apache/lucene/pull/13032/changes

final Counter bytesUsed = bytesStartArray.bytesUsed();
this.bytesUsed = bytesUsed == null ? Counter.newCounter() : bytesUsed;
bytesUsed.addAndGet(hashSize * (long) Integer.BYTES);
this.bytesUsed.addAndGet(hashSize * (long) Integer.BYTES);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa, nice catch, thank you @tyronecai and IntelliJ! I wonder if there is a low-false-positive (high signal to noise ratio) static checker we could turn on in Lucene's CI builds to catch this kind of lurking bug?

I think it indeed must be that all the BytesStartArrays used here have a non-null bytesUsed? In which case +1 to make it required not null (change to assert). This is technically a public API, but @lucene.internal (well, BytesRefHash is), so we are free to suddenly change API/requirement here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:core/other skip-changelog Apply to PRs that don't need a changelog entry, stopping the automated changelog check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants