fix: resolve five latent bugs in util and actor modules#2056
Draft
farmdawgnation wants to merge 2 commits into
Draft
fix: resolve five latent bugs in util and actor modules#2056farmdawgnation wants to merge 2 commits into
farmdawgnation wants to merge 2 commits into
Conversation
Baseline tests passed for all real code on Scala 2.13.18 and 3.3.7; the only failures were latent-bug regression specs. Fix the underlying bugs and repair the specs so the full suite is green on both Scala versions. - SecurityHelpers.randomLong/randomInt: math.abs(MinValue) overflows and stays negative, so the modulo could be negative. Reduce via math.floorMod through a new nonNegativeMod helper. - CurrencyZone.get: replaceAll treated the grouping separator as a regex (German '.' erased every digit). Strip it literally, normalise the decimal separator to '.', and drop residual non-numeric characters so round()'s BigDecimal parse no longer throws for EU locales. - TimeHelpers TimeSpan.format: week scale divisor of 10000 wrapped weeks; use Long.MaxValue so the largest unit never caps. - StringHelpers.splitNameValuePairs: reading pair(1) threw IndexOutOfBounds for entries with no '='; pattern-match and handle the missing-value case. - LAFuture.collect: over-allocated the accumulator (0 to len) and used insert (which shifts) instead of update, returning results in completion order. Use 0 until len + update to preserve input order. Specs exercise the real APIs and use cross-version beEqualTo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes latent utility and actor bugs in Lift and adds regression specs intended to keep behavior green across Scala 2.13 and Scala 3.
Changes:
- Fixes modulo handling, currency normalization, time-span formatting, string pair parsing, and
LAFuturecollection ordering. - Adds shared regression specs for the utility bugs and actor collection ordering.
- Updates
LAFuture.collect/collectAllto write results by input index.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
core/util/src/main/scala/net/liftweb/util/SecurityHelpers.scala |
Replaces math.abs(...) % mod with floorMod-based reduction. |
core/util/src/main/scala/net/liftweb/util/CurrencyZone.scala |
Normalizes locale-specific formatted currency strings into BigDecimal-compatible strings. |
core/util/src/main/scala/net/liftweb/util/TimeHelpers.scala |
Removes the artificial 10000-week cap in TimeSpan.format. |
core/util/src/main/scala/net/liftweb/util/StringHelpers.scala |
Handles name/value entries missing an explicit value. |
core/actor/src/main/scala/net/liftweb/actor/LAFuture.scala |
Preserves input ordering when collecting future results. |
core/util/src/test/scala/net/liftweb/util/LatentBugsSpec.scala |
Adds regression coverage for utility latent bugs. |
core/actor/src/test/scala/net/liftweb/actor/LatentCollectBugSpec.scala |
Adds regression coverage for out-of-order future completion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+60
| val list = props.split(",").toList.flatMap(in => { | ||
| in.roboSplit("=") match { | ||
| case name :: value :: _ => Some((name, unquote(value))) | ||
| case name :: Nil if name.nonEmpty => Some((name, "")) | ||
| case _ => None | ||
| } |
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
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.
Summary
Gets the full test suite green on both Scala 2.13.18 and Scala 3.3.7 for all Lift modules. The baseline showed all real code already passing; the only failures were latent-bug regression specs (failing by design on 2.13, and not even compiling on Scala 3). This fixes the five underlying bugs and repairs the specs.
Bugs fixed
SecurityHelpers.randomLong/randomInt—math.abs(value) % modoverflows forMinValue(stays negative), so results could be negative. Now reduces viamath.floorModthrough a newnonNegativeModhelper.CurrencyZone.get—replaceAlltreated the grouping separator as a regex (German.erased every digit, thenroundthrewNumberFormatException). Now strips the grouping separator literally, normalizes the decimal separator to., and drops residual non-numeric characters (German currency NBSP).TimeHelpersTimeSpan.format— week scale divisor of10000wrapped weeks at the 10000 boundary. Changed toLong.MaxValueso the largest unit never caps.StringHelpers.splitNameValuePairs— readingpair(1)threwIndexOutOfBoundsfor entries with no=. Now pattern-matches and handles the missing-value case.LAFuture.collect— over-allocated the accumulator (0 to len) and usedinsert(which shifts) instead ofupdate, returning results in completion order rather than input order. Fixed to0 until len+update.Regression specs (
LatentBugsSpec,LatentCollectBugSpec) now exercise the real APIs and use cross-versionbeEqualTo.Test results
sbt testexits 0 on both versions:🤖 Generated with Claude Code