From 8d87b76d8963d4f94943f8dd347184ed7c0b6621 Mon Sep 17 00:00:00 2001 From: halibobo1205 Date: Fri, 17 Jul 2026 18:23:28 +0800 Subject: [PATCH 1/2] feat(db): make store sort comparators contract-safe The getAll*/getSpecified* store methods sorted with hand-rolled comparators (`a <= b ? 1 : -1`) that never return 0, violating the Comparator contract: List.sort can throw IllegalArgumentException and the sort intent is unclear. Replace them with Comparator.comparingLong / comparing so equal keys return 0. getSpecifiedProposals must additionally keep equal-expiration proposals in descending-id order. Live execution applies the highest id first and the lowest id last (final value), and the energy/bandwidth price-history loaders rebuild from the tail, so the lowest id must be last. The old never-return-0 comparator produced this order by accident; plain ascending expiration order inverts it and reconstructs the wrong price. Add an explicit descending-id tie-break to preserve the behavior. --- .../org/tron/core/store/AssetIssueStore.java | 12 +-- .../org/tron/core/store/ExchangeStore.java | 7 +- .../org/tron/core/store/ProposalStore.java | 18 ++-- .../tron/core/db/AssetIssueStoreSortTest.java | 53 ++++++++++ ...EnergyPriceHistoryEqualExpirationTest.java | 57 +++++++++++ .../tron/core/db/ExchangeStoreSortTest.java | 74 ++++++++++++++ .../tron/core/db/ProposalStoreSortTest.java | 98 +++++++++++++++++++ 7 files changed, 301 insertions(+), 18 deletions(-) create mode 100644 framework/src/test/java/org/tron/core/db/AssetIssueStoreSortTest.java create mode 100644 framework/src/test/java/org/tron/core/db/EnergyPriceHistoryEqualExpirationTest.java create mode 100644 framework/src/test/java/org/tron/core/db/ExchangeStoreSortTest.java create mode 100644 framework/src/test/java/org/tron/core/db/ProposalStoreSortTest.java diff --git a/chainbase/src/main/java/org/tron/core/store/AssetIssueStore.java b/chainbase/src/main/java/org/tron/core/store/AssetIssueStore.java index 4f69a6c3c66..1f68cf26783 100644 --- a/chainbase/src/main/java/org/tron/core/store/AssetIssueStore.java +++ b/chainbase/src/main/java/org/tron/core/store/AssetIssueStore.java @@ -3,6 +3,8 @@ import static org.tron.common.utils.Commons.ASSET_ISSUE_COUNT_LIMIT_MAX; import com.google.common.collect.Streams; +import com.google.protobuf.ByteString; +import java.util.Comparator; import java.util.List; import java.util.Map.Entry; import java.util.stream.Collectors; @@ -46,12 +48,10 @@ private List getAssetIssuesPaginated(List if (assetIssueList.size() <= offset) { return null; } - assetIssueList.sort((o1, o2) -> { - if (o1.getName() != o2.getName()) { - return o1.getName().toStringUtf8().compareTo(o2.getName().toStringUtf8()); - } - return Long.compare(o1.getOrder(), o2.getOrder()); - }); + assetIssueList.sort( + Comparator.comparing(AssetIssueCapsule::getName, + ByteString.unsignedLexicographicalComparator()) + .thenComparingLong(AssetIssueCapsule::getOrder)); limit = limit > ASSET_ISSUE_COUNT_LIMIT_MAX ? ASSET_ISSUE_COUNT_LIMIT_MAX : limit; long end = offset + limit; end = end > assetIssueList.size() ? assetIssueList.size() : end; diff --git a/chainbase/src/main/java/org/tron/core/store/ExchangeStore.java b/chainbase/src/main/java/org/tron/core/store/ExchangeStore.java index 0cbd1958485..7162dbf7c48 100644 --- a/chainbase/src/main/java/org/tron/core/store/ExchangeStore.java +++ b/chainbase/src/main/java/org/tron/core/store/ExchangeStore.java @@ -1,6 +1,7 @@ package org.tron.core.store; import com.google.common.collect.Streams; +import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.stream.Collectors; @@ -31,9 +32,7 @@ public ExchangeCapsule get(byte[] key) throws ItemNotFoundException { public List getAllExchanges() { return Streams.stream(iterator()) .map(Map.Entry::getValue) - .sorted( - (ExchangeCapsule a, ExchangeCapsule b) -> a.getCreateTime() <= b.getCreateTime() ? 1 - : -1) + .sorted(Comparator.comparingLong(ExchangeCapsule::getCreateTime).reversed()) .collect(Collectors.toList()); } -} \ No newline at end of file +} diff --git a/chainbase/src/main/java/org/tron/core/store/ProposalStore.java b/chainbase/src/main/java/org/tron/core/store/ProposalStore.java index 3d39b717cfc..798e7f3b08c 100644 --- a/chainbase/src/main/java/org/tron/core/store/ProposalStore.java +++ b/chainbase/src/main/java/org/tron/core/store/ProposalStore.java @@ -1,6 +1,7 @@ package org.tron.core.store; import com.google.common.collect.Streams; +import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.stream.Collectors; @@ -32,23 +33,24 @@ public ProposalCapsule get(byte[] key) throws ItemNotFoundException { public List getAllProposals() { return Streams.stream(iterator()) .map(Map.Entry::getValue) - .sorted( - (ProposalCapsule a, ProposalCapsule b) -> a.getCreateTime() <= b.getCreateTime() ? 1 - : -1) + .sorted(Comparator.comparingLong(ProposalCapsule::getCreateTime).reversed()) .collect(Collectors.toList()); } /** - * note: return in asc order by expired time + * Returns proposals in ascending expiration order, ties broken by descending proposal ID. + * + *

The descending-id tie-break preserves the execution order for equal-expiration proposals: + * live execution applies the highest id first and the lowest id last (final value), and the + * energy/bandwidth price-history loaders rebuild from the tail, so the lowest id must be last. */ public List getSpecifiedProposals(State state, long code) { return Streams.stream(iterator()) .map(Map.Entry::getValue) .filter(proposalCapsule -> proposalCapsule.getState().equals(state)) .filter(proposalCapsule -> proposalCapsule.getParameters().containsKey(code)) - .sorted( - (ProposalCapsule a, ProposalCapsule b) -> a.getExpirationTime() > b.getExpirationTime() - ? 1 : -1) + .sorted(Comparator.comparingLong(ProposalCapsule::getExpirationTime) + .thenComparing(Comparator.comparingLong(ProposalCapsule::getID).reversed())) .collect(Collectors.toList()); } -} \ No newline at end of file +} diff --git a/framework/src/test/java/org/tron/core/db/AssetIssueStoreSortTest.java b/framework/src/test/java/org/tron/core/db/AssetIssueStoreSortTest.java new file mode 100644 index 00000000000..6f026a4bc0c --- /dev/null +++ b/framework/src/test/java/org/tron/core/db/AssetIssueStoreSortTest.java @@ -0,0 +1,53 @@ +package org.tron.core.db; + +import static java.util.stream.Collectors.toList; +import static org.junit.Assert.assertEquals; + +import com.google.protobuf.ByteString; +import java.util.Arrays; +import java.util.List; +import javax.annotation.Resource; +import org.junit.Test; +import org.tron.common.BaseTest; +import org.tron.common.TestConstants; +import org.tron.core.capsule.AssetIssueCapsule; +import org.tron.core.config.args.Args; +import org.tron.core.store.AssetIssueStore; +import org.tron.protos.contract.AssetIssueContractOuterClass.AssetIssueContract; + +/** + * Sort-behaviour regression test for {@link AssetIssueStore#getAssetIssuesPaginated(long, long)}. + * Guards the fix that replaced the {@code ByteString} reference-comparison comparator (whose + * secondary {@code order} branch was dead code) with a name-then-order comparator built from + * {@code Comparator.comparing(..., unsignedLexicographicalComparator()).thenComparingLong(...)}. + */ +public class AssetIssueStoreSortTest extends BaseTest { + + @Resource + private AssetIssueStore assetIssueStore; + + static { + Args.setParam(new String[] {"--output-directory", dbPath()}, TestConstants.TEST_CONF); + } + + private void put(String id, String name, long order) { + AssetIssueContract contract = AssetIssueContract.newBuilder() + .setId(id) + .setName(ByteString.copyFromUtf8(name)) + .setOrder(order) + .build(); + assetIssueStore.put(id.getBytes(), new AssetIssueCapsule(contract)); + } + + @Test + public void paginated_ordersByNameThenOrder() { + put("2001", "BBB", 0L); + put("2002", "AAA", 5L); // same name as 2003, larger order -> must sort after it + put("2003", "AAA", 0L); + List ordered = assetIssueStore.getAssetIssuesPaginated(0, 100).stream() + .map(a -> a.getName().toStringUtf8() + "/" + a.getOrder()) + .collect(toList()); + // name ascending, then order ascending within the same name + assertEquals(Arrays.asList("AAA/0", "AAA/5", "BBB/0"), ordered); + } +} diff --git a/framework/src/test/java/org/tron/core/db/EnergyPriceHistoryEqualExpirationTest.java b/framework/src/test/java/org/tron/core/db/EnergyPriceHistoryEqualExpirationTest.java new file mode 100644 index 00000000000..579ef6088dc --- /dev/null +++ b/framework/src/test/java/org/tron/core/db/EnergyPriceHistoryEqualExpirationTest.java @@ -0,0 +1,57 @@ +package org.tron.core.db; + +import static org.tron.core.utils.ProposalUtil.ProposalType.ENERGY_FEE; + +import org.junit.Assert; +import org.junit.Test; +import org.tron.common.BaseTest; +import org.tron.common.TestConstants; +import org.tron.core.capsule.ProposalCapsule; +import org.tron.core.config.args.Args; +import org.tron.core.db.api.EnergyPriceHistoryLoader; +import org.tron.core.services.jsonrpc.JsonRpcApiUtil; +import org.tron.protos.Protocol.Proposal; +import org.tron.protos.Protocol.Proposal.State; + +/** + * End-to-end regression test for equal-expiration proposal ordering. When two ENERGY_FEE proposals + * share an expiration time, the rebuilt price history must resolve to the lowest-id (final) value, + * matching live execution order. Guards the descending-id tie-break in + * {@link org.tron.core.store.ProposalStore#getSpecifiedProposals}. + */ +public class EnergyPriceHistoryEqualExpirationTest extends BaseTest { + + static { + Args.setParam(new String[] {"--output-directory", dbPath()}, TestConstants.TEST_CONF); + } + + private void initProposal(long code, long timestamp, long price, State state) { + long id = chainBaseManager.getDynamicPropertiesStore().getLatestProposalNum() + 1; + Proposal proposal = Proposal.newBuilder() + .putParameters(code, price) + .setExpirationTime(timestamp) + .setState(state) + .setProposalId(id) + .build(); + ProposalCapsule capsule = new ProposalCapsule(proposal); + chainBaseManager.getProposalStore().put(capsule.createDbKey(), capsule); + chainBaseManager.getDynamicPropertiesStore().saveLatestProposalNum(id); + } + + @Test + public void rebuildsLowestIdValueForEqualExpiration() { + long expiration = 1600000000000L; + long lowIdPrice = 15; + long highIdPrice = 99; + initProposal(ENERGY_FEE.getCode(), expiration, lowIdPrice, State.APPROVED); // id N (lower) + initProposal(ENERGY_FEE.getCode(), expiration, highIdPrice, State.APPROVED); // id N+1 (higher) + + EnergyPriceHistoryLoader loader = new EnergyPriceHistoryLoader(chainBaseManager); + loader.getEnergyProposals(); + String history = loader.parseProposalsToStr(); + + // Live execution applies highest id first and lowest id last, so the lowest id is the final + // value. The tail-first parseEnergyFee must resolve a post-expiration timestamp to that price. + Assert.assertEquals(lowIdPrice, JsonRpcApiUtil.parseEnergyFee(expiration + 1, history)); + } +} diff --git a/framework/src/test/java/org/tron/core/db/ExchangeStoreSortTest.java b/framework/src/test/java/org/tron/core/db/ExchangeStoreSortTest.java new file mode 100644 index 00000000000..63984bc5aee --- /dev/null +++ b/framework/src/test/java/org/tron/core/db/ExchangeStoreSortTest.java @@ -0,0 +1,74 @@ +package org.tron.core.db; + +import static java.util.stream.Collectors.toList; +import static org.junit.Assert.assertEquals; + +import com.google.protobuf.ByteString; +import java.util.Arrays; +import java.util.List; +import javax.annotation.Resource; +import org.junit.Test; +import org.tron.common.BaseTest; +import org.tron.common.TestConstants; +import org.tron.core.capsule.ExchangeCapsule; +import org.tron.core.config.args.Args; +import org.tron.core.store.ExchangeStore; +import org.tron.protos.Protocol; + +/** + * Sort-behaviour regression tests for {@link ExchangeStore#getAllExchanges()}. Guards the fix that + * replaced the never-return-0 comparator (`? 1 : -1`) with contract-safe + * {@code Comparator.comparingLong(...).reversed()} (createTime descending). + * + *

Each test uses a disjoint exchange-id range and asserts only on its own ids. + */ +public class ExchangeStoreSortTest extends BaseTest { + + @Resource + private ExchangeStore exchangeStore; + + static { + Args.setParam(new String[] {"--output-directory", dbPath()}, TestConstants.TEST_CONF); + } + + private void put(long id, long createTime) { + Protocol.Exchange exchange = Protocol.Exchange.newBuilder() + .setExchangeId(id) + .setCreateTime(createTime) + .setCreatorAddress(ByteString.copyFromUtf8("Address" + id)) + .build(); + ExchangeCapsule capsule = new ExchangeCapsule(exchange); + exchangeStore.put(capsule.createDbKey(), capsule); + } + + private List idsInRange(List list, long lo, long hi) { + return list.stream() + .map(ExchangeCapsule::getID) + .filter(id -> id >= lo && id <= hi) + .collect(toList()); + } + + @Test + public void getAllExchanges_ordersByCreateTimeDescending() { + put(101, 100L); + put(102, 300L); + put(103, 200L); + // newest (largest createTime) first + assertEquals(Arrays.asList(102L, 103L, 101L), + idsInRange(exchangeStore.getAllExchanges(), 101, 103)); + } + + @Test + public void getAllExchanges_manyEqualCreateTime_returnsAllWithoutThrowing() { + long lo = 1000; + // >= 32 elements exercises TimSort's merge path. All-equal keys form one run (no merge), so + // this does NOT reproduce the old never-return-0 throw -- that form is caught at compile time + // by ComparatorNeverReturnsZero. Here we only lock the runtime invariant: every equal-time + // exchange is returned, no exception. + int n = 40; + for (int i = 0; i < n; i++) { + put(lo + i, 500L); + } + assertEquals(n, idsInRange(exchangeStore.getAllExchanges(), lo, lo + n - 1).size()); + } +} diff --git a/framework/src/test/java/org/tron/core/db/ProposalStoreSortTest.java b/framework/src/test/java/org/tron/core/db/ProposalStoreSortTest.java new file mode 100644 index 00000000000..7b94fa86aa2 --- /dev/null +++ b/framework/src/test/java/org/tron/core/db/ProposalStoreSortTest.java @@ -0,0 +1,98 @@ +package org.tron.core.db; + +import static java.util.stream.Collectors.toList; +import static org.junit.Assert.assertEquals; + +import java.util.Arrays; +import java.util.List; +import javax.annotation.Resource; +import org.junit.Test; +import org.tron.common.BaseTest; +import org.tron.common.TestConstants; +import org.tron.core.capsule.ProposalCapsule; +import org.tron.core.config.args.Args; +import org.tron.core.store.ProposalStore; +import org.tron.protos.Protocol.Proposal; +import org.tron.protos.Protocol.Proposal.State; + +/** + * Sort-behaviour regression tests for {@link ProposalStore}. Guards the fix that replaced the + * never-return-0 comparators (`? 1 : -1`) with contract-safe {@code Comparator.comparingLong}. + * + *

Each test uses a disjoint proposal-id range and asserts only on its own ids, so the tests are + * independent of each other despite BaseTest sharing one DB per class. + */ +public class ProposalStoreSortTest extends BaseTest { + + @Resource + private ProposalStore proposalStore; + + static { + Args.setParam(new String[] {"--output-directory", dbPath()}, TestConstants.TEST_CONF); + } + + private void put(long id, long createTime, long expirationTime, State state, int paramKey) { + Proposal proposal = Proposal.newBuilder() + .setProposalId(id) + .setCreateTime(createTime) + .setExpirationTime(expirationTime) + .setState(state) + .putParameters(paramKey, 1L) + .build(); + proposalStore.put(Long.toString(id).getBytes(), new ProposalCapsule(proposal)); + } + + private List idsInRange(List list, long lo, long hi) { + return list.stream() + .map(ProposalCapsule::getID) + .filter(id -> id >= lo && id <= hi) + .collect(toList()); + } + + @Test + public void getAllProposals_ordersByCreateTimeDescending() { + put(101, 100L, 0L, State.PENDING, 1); + put(102, 300L, 0L, State.PENDING, 1); + put(103, 200L, 0L, State.PENDING, 1); + // newest (largest createTime) first + assertEquals(Arrays.asList(102L, 103L, 101L), + idsInRange(proposalStore.getAllProposals(), 101, 103)); + } + + @Test + public void getSpecifiedProposals_ordersByExpirationAscending() { + put(201, 0L, 300L, State.PENDING, 7); + put(202, 0L, 100L, State.PENDING, 7); + put(203, 0L, 200L, State.PENDING, 7); + // soonest expiration first + assertEquals(Arrays.asList(202L, 203L, 201L), + idsInRange(proposalStore.getSpecifiedProposals(State.PENDING, 7), 201, 203)); + } + + @Test + public void getSpecifiedProposals_equalExpiration_breaksTiesByIdDescending() { + // Equal-expiration proposals must be returned highest-id-first. Live execution applies the + // highest id first and the lowest id last (final value), and the price-history loaders rebuild + // from the tail -- so the lowest id must be last. The pre-fix ascending-id order inverted this + // and reconstructed the wrong energy/bandwidth price. This test fails on that broken order. + put(401, 0L, 500L, State.APPROVED, 9); + put(402, 0L, 500L, State.APPROVED, 9); + put(403, 0L, 500L, State.APPROVED, 9); + assertEquals(Arrays.asList(403L, 402L, 401L), + idsInRange(proposalStore.getSpecifiedProposals(State.APPROVED, 9), 401, 403)); + } + + @Test + public void getAllProposals_manyEqualCreateTime_returnsAllWithoutThrowing() { + long lo = 1000; + // >= 32 elements exercises TimSort's merge path. All-equal keys form one run (no merge), so + // this does NOT reproduce the old never-return-0 throw -- that form is caught at compile time + // by ComparatorNeverReturnsZero. Here we only lock the runtime invariant: every equal-time + // proposal is returned, no exception. + int n = 40; + for (int i = 0; i < n; i++) { + put(lo + i, 500L, 0L, State.PENDING, 1); + } + assertEquals(n, idsInRange(proposalStore.getAllProposals(), lo, lo + n - 1).size()); + } +} From 88f1be127d43bb5f150d0882268b565a82c886f9 Mon Sep 17 00:00:00 2001 From: halibobo1205 Date: Fri, 17 Jul 2026 18:23:28 +0800 Subject: [PATCH 2/2] build(errorprone): reject comparators that never return zero Add a custom ErrorProne BugChecker that flags Comparator/compareTo bodies which can never return 0 (e.g. `? 1 : -1`). The built-in ComparisonContractViolated inspects only method declarations, so it cannot see the lambda comparators this repo actually uses; this checker covers the lambda form and method declarations, matching genuine overrides via findSuperMethods so same-named overloads are not flagged. It is a conservative syntactic check (direct constant or ternary-of- constant returns only, no data-flow). Enable it as ERROR in the allowlist, and add the test-helper dependencies to verification-metadata so the checker tests run under strict dependency verification. --- build.gradle | 1 + errorprone/build.gradle | 23 ++ .../ComparatorNeverReturnsZero.java | 172 ++++++++++++ .../ComparatorNeverReturnsZeroTest.java | 244 ++++++++++++++++++ gradle/verification-metadata.xml | 100 +++++++ 5 files changed, 540 insertions(+) create mode 100644 errorprone/src/main/java/errorprone/ComparatorNeverReturnsZero.java create mode 100644 errorprone/src/test/java/errorprone/ComparatorNeverReturnsZeroTest.java diff --git a/build.gradle b/build.gradle index e143ab3a947..60287ea3447 100644 --- a/build.gradle +++ b/build.gradle @@ -130,6 +130,7 @@ subprojects { errorproneArgs.addAll([ '-Xep:StringCaseLocaleUsage:ERROR', '-Xep:StringCaseLocaleUsageMethodRef:ERROR', + '-Xep:ComparatorNeverReturnsZero:ERROR', ]) } } diff --git a/errorprone/build.gradle b/errorprone/build.gradle index f8a634b7edc..fc0f5940225 100644 --- a/errorprone/build.gradle +++ b/errorprone/build.gradle @@ -2,6 +2,7 @@ if (!JavaVersion.current().isJava11Compatible()) { // ErrorProne core requires JDK 11+; skip this module on JDK 8 tasks.withType(JavaCompile).configureEach { enabled = false } tasks.withType(Jar).configureEach { enabled = false } + tasks.withType(Test).configureEach { enabled = false } } else { dependencies { compileOnly "com.google.errorprone:error_prone_annotations:${errorproneVersion}" @@ -9,5 +10,27 @@ if (!JavaVersion.current().isJava11Compatible()) { compileOnly "com.google.errorprone:error_prone_core:${errorproneVersion}" compileOnly "com.google.auto.service:auto-service:1.1.1" annotationProcessor "com.google.auto.service:auto-service:1.1.1" + + testImplementation "com.google.errorprone:error_prone_check_api:${errorproneVersion}" + testImplementation "com.google.errorprone:error_prone_core:${errorproneVersion}" + testImplementation "com.google.errorprone:error_prone_test_helpers:${errorproneVersion}" + } + + // ErrorProne's CompilationTestHelper drives javac internals; JDK 16+ needs these exports/opens. + tasks.withType(Test).configureEach { + jvmArgs += [ + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + ] } } diff --git a/errorprone/src/main/java/errorprone/ComparatorNeverReturnsZero.java b/errorprone/src/main/java/errorprone/ComparatorNeverReturnsZero.java new file mode 100644 index 00000000000..cb9929d6222 --- /dev/null +++ b/errorprone/src/main/java/errorprone/ComparatorNeverReturnsZero.java @@ -0,0 +1,172 @@ +package errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ConditionalExpressionTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.ReturnTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.UnaryTree; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Symbol; +import java.util.ArrayList; +import java.util.List; + +/** + * Flags {@link java.util.Comparator}/{@link java.lang.Comparable} implementations that can never + * return {@code 0} (e.g. {@code a <= b ? 1 : -1}). + * + *

Such a comparator violates the general contract: when two elements compare "equal" it still + * returns a non-zero value, breaking antisymmetry. At runtime {@code List.sort} (TimSort) throws + * {@code IllegalArgumentException: Comparison method violates its general contract!} once the list + * is large enough and contains equal keys; otherwise it silently produces an undefined order. + * + *

The built-in ErrorProne {@code ComparisonContractViolated} checker only inspects + * {@code compare}/{@code compareTo} method declarations, so it cannot see lambda + * comparators such as {@code list.sort((a, b) -> a.t() <= b.t() ? 1 : -1)}. This checker covers + * both the lambda form and the method-declaration form. + * + *

This is a deliberately conservative syntactic check: it only flags a body whose every + * return is a non-zero {@code int} constant (a literal, or a ternary of such constants). It does no + * data-flow analysis, so a value laundered through a variable + * ({@code int r = c ? -1 : 1; return r;}) is not flagged. + * + *

Fix by returning {@code 0} on equality, e.g. {@code Long.compare(a, b)} or + * {@code Comparator.comparingLong(X::t)} (append {@code .reversed()} for descending order). + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "ComparatorNeverReturnsZero", + summary = "Comparator/compareTo can never return 0 (e.g. `? 1 : -1`), violating the comparison " + + "contract; List.sort may throw IllegalArgumentException. Return 0 on equality, e.g. " + + "Long.compare(a, b) or Comparator.comparingLong(...).", + severity = BugPattern.SeverityLevel.ERROR) +public class ComparatorNeverReturnsZero extends BugChecker + implements BugChecker.LambdaExpressionTreeMatcher, BugChecker.MethodTreeMatcher { + + private static final Matcher IS_COMPARATOR = + Matchers.isSubtypeOf("java.util.Comparator"); + + @Override + public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { + // Only comparator lambdas: the target functional interface is java.util.Comparator. + // (Comparable is not a functional interface, so it can never be a lambda.) + if (!IS_COMPARATOR.matches(tree, state)) { + return Description.NO_MATCH; + } + return neverReturnsZero(tree.getBody()) ? describeMatch(tree) : Description.NO_MATCH; + } + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (tree.getBody() == null || !isComparatorMethod(tree, state)) { + return Description.NO_MATCH; + } + return neverReturnsZero(tree.getBody()) ? describeMatch(tree) : Description.NO_MATCH; + } + + /** + * True if the comparator body ({@code (a,b) -> expr}, {@code (a,b) -> {..}} or a method block) + * provably yields a non-zero {@code int} constant on every path. Conservative: any path whose + * value cannot be proven non-zero (a method call, a subtraction, a plain {@code 0}, ...) makes + * this return {@code false}, so legitimate comparators such as {@code Long.compare(a, b)} are + * never flagged. + */ + private static boolean neverReturnsZero(Tree body) { + if (body instanceof ExpressionTree) { + return alwaysNonZero((ExpressionTree) body); + } + if (body instanceof BlockTree) { + List returns = new ArrayList<>(); + new ReturnCollector().scan(body, returns); + if (returns.isEmpty()) { + return false; + } + for (ReturnTree r : returns) { + if (r.getExpression() == null || !alwaysNonZero(r.getExpression())) { + return false; + } + } + return true; + } + return false; + } + + /** True if {@code e} is provably a non-zero {@code int} constant on every branch. */ + private static boolean alwaysNonZero(ExpressionTree e) { + e = stripParens(e); + Integer c = ASTHelpers.constValue(e, Integer.class); + if (c != null) { + return c != 0; + } + // Fall back for `-1` / `+1` in case constant folding did not run. + if (e instanceof UnaryTree + && (e.getKind() == Tree.Kind.UNARY_MINUS || e.getKind() == Tree.Kind.UNARY_PLUS)) { + Integer operand = + ASTHelpers.constValue(stripParens(((UnaryTree) e).getExpression()), Integer.class); + return operand != null && operand != 0; + } + if (e instanceof ConditionalExpressionTree) { + ConditionalExpressionTree cond = (ConditionalExpressionTree) e; + return alwaysNonZero(cond.getTrueExpression()) && alwaysNonZero(cond.getFalseExpression()); + } + return false; + } + + private static ExpressionTree stripParens(ExpressionTree e) { + while (e instanceof ParenthesizedTree) { + e = ((ParenthesizedTree) e).getExpression(); + } + return e; + } + + /** + * True only for methods that genuinely override {@code Comparator.compare} or + * {@code Comparable.compareTo}. Matching by name + arity alone would wrongly flag same-named + * overloads (e.g. {@code compareTo(long)}) and static/private helpers; those override nothing, so + * {@link ASTHelpers#findSuperMethods} returns no comparator super-method for them. + */ + private static boolean isComparatorMethod(MethodTree tree, VisitorState state) { + Symbol.MethodSymbol sym = ASTHelpers.getSymbol(tree); + if (sym == null) { + return false; + } + for (Symbol.MethodSymbol superMethod : ASTHelpers.findSuperMethods(sym, state.getTypes())) { + String ownerName = superMethod.owner.getQualifiedName().toString(); + if (ownerName.equals("java.util.Comparator") || ownerName.equals("java.lang.Comparable")) { + return true; + } + } + return false; + } + + /** Collects return statements owned by this method/lambda, not by nested functions. */ + private static final class ReturnCollector extends TreeScanner> { + @Override + public Void visitReturn(ReturnTree node, List returns) { + returns.add(node); + return super.visitReturn(node, returns); + } + + @Override + public Void visitLambdaExpression(LambdaExpressionTree node, List returns) { + return null; // a nested lambda's returns are its own + } + + @Override + public Void visitClass(ClassTree node, List returns) { + return null; // a nested / anonymous class's returns are its own + } + } +} diff --git a/errorprone/src/test/java/errorprone/ComparatorNeverReturnsZeroTest.java b/errorprone/src/test/java/errorprone/ComparatorNeverReturnsZeroTest.java new file mode 100644 index 00000000000..95e461b0618 --- /dev/null +++ b/errorprone/src/test/java/errorprone/ComparatorNeverReturnsZeroTest.java @@ -0,0 +1,244 @@ +package errorprone; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; + +/** Tests for {@link ComparatorNeverReturnsZero}. */ +public class ComparatorNeverReturnsZeroTest { + + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(ComparatorNeverReturnsZero.class, getClass()); + + // ---------- positive: must be flagged ---------- + + @Test + public void lambdaExpression_leEver1Else1_flagged() { + helper + .addSourceLines( + "Test.java", + "import java.util.List;", + "class Test {", + " void f(List xs) {", + " // BUG: Diagnostic contains: ComparatorNeverReturnsZero", + " xs.sort((a, b) -> a <= b ? 1 : -1);", + " }", + "}") + .doTest(); + } + + @Test + public void lambdaExpression_gtThen1Else1_flagged() { + helper + .addSourceLines( + "Test.java", + "import java.util.List;", + "class Test {", + " void f(List xs) {", + " // BUG: Diagnostic contains: ComparatorNeverReturnsZero", + " xs.sort((a, b) -> a > b ? 1 : -1);", + " }", + "}") + .doTest(); + } + + @Test + public void lambdaBlock_allReturnsNonZero_flagged() { + helper + .addSourceLines( + "Test.java", + "import java.util.Comparator;", + "class Test {", + " Comparator c() {", + " // BUG: Diagnostic contains: ComparatorNeverReturnsZero", + " return (a, b) -> {", + " if (a <= b) {", + " return 1;", + " }", + " return -1;", + " };", + " }", + "}") + .doTest(); + } + + @Test + public void anonymousComparator_compare_flagged() { + helper + .addSourceLines( + "Test.java", + "import java.util.Comparator;", + "class Test {", + " Comparator c() {", + " return new Comparator() {", + " // BUG: Diagnostic contains: ComparatorNeverReturnsZero", + " public int compare(Long a, Long b) {", + " return a <= b ? 1 : -1;", + " }", + " };", + " }", + "}") + .doTest(); + } + + @Test + public void namedComparable_compareTo_flagged() { + helper + .addSourceLines( + "Test.java", + "class Test implements Comparable {", + " long t;", + " // BUG: Diagnostic contains: ComparatorNeverReturnsZero", + " public int compareTo(Test o) {", + " return this.t < o.t ? -1 : 1;", + " }", + "}") + .doTest(); + } + + // ---------- negative: must NOT be flagged ---------- + + @Test + public void lambdaLongCompare_ok() { + helper + .addSourceLines( + "Test.java", + "import java.util.List;", + "class Test {", + " void f(List xs) {", + " xs.sort((a, b) -> Long.compare(a, b));", + " }", + "}") + .doTest(); + } + + @Test + public void lambdaComparingLong_ok() { + helper + .addSourceLines( + "Test.java", + "import java.util.Comparator;", + "import java.util.List;", + "class Test {", + " void f(List xs) {", + " xs.sort(Comparator.comparingLong((Test t) -> t.t).reversed());", + " }", + " long t;", + "}") + .doTest(); + } + + @Test + public void ternaryCanReturnZero_ok() { + helper + .addSourceLines( + "Test.java", + "import java.util.List;", + "class Test {", + " void f(List xs) {", + " xs.sort((a, b) -> a <= b ? 1 : 0);", // can yield 0 -> not a violation shape + " }", + "}") + .doTest(); + } + + @Test + public void ceilingDivisionNotAComparator_ok() { + helper + .addSourceLines( + "Test.java", + "class Test {", + " long ceil(long numerator, long denominator) {", + " return (numerator / denominator) + ((numerator % denominator) > 0 ? 1 : 0);", + " }", + "}") + .doTest(); + } + + @Test + public void compareToUsingLongCompare_ok() { + helper + .addSourceLines( + "Test.java", + "class Test implements Comparable {", + " long t;", + " public int compareTo(Test o) {", + " return Long.compare(this.t, o.t);", + " }", + "}") + .doTest(); + } + + @Test + public void compareToWithNullGuardAndRealCompare_ok() { + // Mirrors DataWord.compareTo: one constant return (-1 for null) plus a non-constant return. + helper + .addSourceLines( + "Test.java", + "class Test implements Comparable {", + " long t;", + " public int compareTo(Test o) {", + " if (o == null) {", + " return -1;", + " }", + " return Long.compare(this.t, o.t);", + " }", + "}") + .doTest(); + } + + @Test + public void overloadedCompareToNotAnOverride_ok() { + // compareTo(long) is an overload, not an override of Comparable.compareTo(Test); must not flag. + helper + .addSourceLines( + "Test.java", + "class Test implements Comparable {", + " long t;", + " public int compareTo(Test o) {", + " return Long.compare(this.t, o.t);", + " }", + " int compareTo(long value) {", + " return value < 0 ? -1 : 1;", + " }", + "}") + .doTest(); + } + + @Test + public void twoArgStaticCompareOverloadInComparator_ok() { + // static compare(long,long) is a 2-arg overload inside a Comparator class but overrides + // nothing. The old name+arity+owner rule would flag it, the override-based rule must not. + helper + .addSourceLines( + "Test.java", + "import java.util.Comparator;", + "class Test implements Comparator {", + " public int compare(Long a, Long b) {", + " return Long.compare(a, b);", + " }", + " static int compare(long a, long b) {", + " return a < b ? -1 : 1;", + " }", + "}") + .doTest(); + } + + @Test + public void twoArgPrivateCompareOverloadInComparator_ok() { + // private compare(long,long) is a 2-arg overload inside a Comparator class but overrides + // nothing. The old name+arity+owner rule would flag it, the override-based rule must not. + helper + .addSourceLines( + "Test.java", + "import java.util.Comparator;", + "class Test implements Comparator {", + " public int compare(Long a, Long b) {", + " return Long.compare(a, b);", + " }", + " private int compare(long a, long b) {", + " return a < b ? -1 : 1;", + " }", + "}") + .doTest(); + } +} diff --git a/gradle/verification-metadata.xml b/gradle/verification-metadata.xml index 832d2728f0b..4658cea2dce 100644 --- a/gradle/verification-metadata.xml +++ b/gradle/verification-metadata.xml @@ -387,6 +387,22 @@ + + + + + + + + + + + + + + + + @@ -395,6 +411,16 @@ + + + + + + + + + + @@ -526,6 +552,14 @@ + + + + + + + + @@ -720,6 +754,19 @@ + + + + + + + + + + + + + @@ -789,6 +836,27 @@ + + + + + + + + + + + + + + + + + + + + + @@ -2145,6 +2213,14 @@ + + + + + + + + @@ -2153,6 +2229,22 @@ + + + + + + + + + + + + + + + + @@ -2403,6 +2495,14 @@ + + + + + + + +