Converge AssertJ isSameAs/isNotSameAs(0|1) within a single run#1036
Merged
Conversation
…in a single run The Assertj aggregate normalizes primitive isSameAs/isNotSameAs to isEqualTo/isNotEqualTo via Picnic's AssertJPrimitiveRulesRecipes, after which our type-specific rules collapse isEqualTo(0) to isZero(), etc. Because between-cycle re-attribution does not give the rewritten node a typed receiver, the second simplification only fired on a subsequent rewriteRun. Let our type rules also match the isSameAs/isNotSameAs forms directly so they collapse straight to isZero/isNotZero/isOne, and order them before AssertJPrimitiveRulesRecipes so they win within a single cycle. Fixes #1032
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's changed?
rewriteRunof theAssertjaggregate leftassertThat(intValue).isNotSameAs(0)asisNotEqualTo(0), and only a second run collapsed it toisNotZero().Root cause
Within the aggregate, Picnic's
AssertJPrimitiveRulesRecipesnormalizesisSameAs/isNotSameAson primitives toisEqualTo/isNotEqualTo. Our type-specific rules then collapseisEqualTo(0)→isZero(), etc. But the node produced by the primitive rule isn't given a typed (AbstractIntegerAssert) receiver by between-cycle re-attribution, so our Refaster rules don't match it until the source is fully re-parsed on a later run.Fix
Integer,Long,Short,Byte,Double,Float) now also match theisSameAs/isNotSameAsforms directly, collapsing straight toisZero()/isNotZero()/isOne()on the freshly-parsed, fully-typed source.AssertJShortRulesRecipesahead ofAssertJPrimitiveRulesRecipesso all our number rules run before the primitive normalization (the other six already did), letting them win within a single cycle.BigIntegeris unaffected — Picnic's primitive rules don't touch it, so there is no intermediateisSameAs→isEqualTostep to converge.Testing
Added a single-run convergence test across all affected primitive types to
AssertJBestPracticesTest.Extended the per-type rule tests (
Integer/Long/Short/Byte/Double/Float) with theisSameAs/isNotSameAsforms.Full test suite passes.
Fixes AssertJ aggregate is not idempotent: isNotSameAs(0) → isNotEqualTo(0) → isNotZero() requires two rewriteRuns #1032