[patch] Fix storage conversion regressions from exact factors - #231
Merged
Merged
Conversation
Restore integer storage behavior broken in 5.2.0. 5.2.0 converted every conversion factor and metric magnitude for a storage type in one static initializer, so a single value too large for the type (Tera or CurieToBecquerels for int, Yotta for long) made every factory and In(unit) for that type throw TypeInitializationException. Length<int>.FromKilometer(1) threw instead of returning 1000. Each holder value is now a property over a nullable parsed field, and a type that does not parse the literal converts the double at each read, so each value succeeds or throws OverflowException on its own, exactly as the 5.1 factories did. Make StorageMath.Sqrt return the converged root or throw. A value a double cannot hold is scaled by powers of four into [1, 4) for the seed and the root scaled back, and a root that does not settle throws ArithmeticException. A BigInteger of 2^2048 now gives 2^1024 instead of about 2^1792. Recompute every literal built on pi in conversions.json and domains.json from pi itself, correctly rounded to 150 significant digits. The old literals were wrong from the 97th to 104th significant digit. Add PiLiteralTests, which checks each literal to its last digit against pi computed by Machin's formula. Replace AMetricMagnitudeIsExact, which passed through the old double route too, with a test that combines a magnitude with a 17-digit factor and fails through that route. Emit a d suffix on every double constant, so a literal such as 100000000000000000000 compiles, and make SEM009 reject a literal, operand, or quotient beyond the range of double, or a non-zero value that rounds to zero. Treat a TryParse that throws NotSupportedException or ArgumentException for NumberStyles.Float as a type that cannot parse the literal, so it falls back to the double instead of failing. Correct the claim that no double constant changed. PsiToPascals and RevolutionPerMinuteToRadianPerSecond moved to the adjacent double in 5.2.0, and with them IUnit.ToBaseFactor of Psi and RevolutionPerMinute. Document it in CLAUDE.md and docs/physics-generator.md. Multiply both magnitude and conversion factor in QuantitiesGenerator when a unit declares both, as UnitsGenerator already does. Claude-Session: https://claude.ai/code/session_01K5Bk9UjGdGUtC5C6qK5ZxD
Both CodeQL findings are exact-zero comparisons, and both are kept exact rather than turned into a tolerance: - ConversionValue.IsHeldByDouble tested `value != 0d` to catch a non-zero literal that underflowed. It now asks whether the magnitude is above zero, which is the same test once NaN is already excluded. The framework predicate CodeQL suggests, `double.IsZero`, is a static abstract on INumberBase<double> and does not exist on netstandard2.0, which this generator targets. - ParseThrowingNumber.IsZero now routes through INumberBase<T>.IsZero via a constrained type parameter, which is the only way to reach a static abstract interface member. Also clears the Sonar findings in the code this branch adds, none of which change any generated output: - PiLiteralTests.ArctanOfReciprocal advanced `n` in the incrementer while testing `power` in the condition (S1994). It is a while loop now. - ConversionValue.IsDecimalLiteral was over the cognitive complexity limit (S3776), split into SkipSign, TryPassFraction and TryPassExponent. - The fourth `"internal"` literal in ConversionsGenerator tripped S1192. Added Emit.Internal and used it across the three generators that spell the modifier. Solution builds with no warnings, all 1237 tests in Semantics.Test pass, and regenerating the committed generator output leaves no diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MLjFwwiUGZ6i5yZ7rj1WAU
|
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.



Integer storage converts again, and exact factors stay exact
Versions 5.2.0 through 5.2.3 break every quantity stored as an integer. Since #226,
Length<int>.FromKilometer(1)throwsTypeInitializationExceptioninstead of returning 1000, and sodoes every other factory and every
In(unit)forintandlong. This PR restores the 5.1 behaviorexactly: each factor succeeds or throws
OverflowExceptionon its own, at the call that uses it.It also fixes the other review findings on #226:
StorageMath.Sqrtreturned a wrong root without complaint for values adoublecannot hold. ABigIntegerof 2^2048 gave about 2^1792. It now gives 2^1024, or throws if Newton's method does notsettle.
conversions.jsonand
domains.json. They are recomputed from π and correctly rounded to 150 significant digits.100000000000000000000,1e400).TryParsethrows forNumberStyles.Floatbroke every factor for that type.doubleconstant changed was wrong.PsiToPascalsandRevolutionPerMinuteToRadianPerSecondeach moved to the adjacentdouble(closer to the true value)in 5.2.0, and
IUnit.ToBaseFactorofPsiandRevolutionPerMinutemoved with them. That is nowdocumented rather than denied.
Mechanism
Integer storage. 5.2.0 emitted each factor as
internal static readonly T X = StorageLiteral.Parse<T>(literal, double),where the fallback
T.CreateChecked(double)ran inside theValues<T>static initializer. One valuethat does not fit (Tera or
CurieToBecquerelsforint, Yotta forlong) failed the initializer, andevery value for that type failed with it. Each value is now:
StorageLiteralno longer converts thedoubleat all. It answersnullfor an integer type or onethat cannot parse the literal, so the initializer cannot throw for those types, and the conversion
happens at the read exactly as the 5.1 factories did it.
doubleanddecimalstill read a cachedparsed value, and
QuantityValueTypeTestsstill measures 0 bytes allocated.Square roots. When the value does not convert to a normal
double, it is scaled by powers of fourinto [1, 4), the root is taken there and scaled back by the matching power of two, so the seed is
always within a factor of two. Reaching the step cap now throws
ArithmeticExceptioninstead ofreturning the estimate. The
double,float,Half, and integer primitive paths are unchanged.π literals. π was computed to 220 places with Machin's formula over integers, checked against a
second Machin-like formula and against the first 100 published digits, and each literal written
correctly rounded to 150 significant digits. Changed:
DegreeToRadians,GradianToRadians,RevolutionToRadians,RevolutionPerMinuteToRadianPerSecond, andFootLambertToCandelaPerSquareMeterinconversions.json, andTwoPi,DegreesPerRadian, andRadiansPerDegreeindomains.json. Nodoubleconstant changes, because every error was past the17th digit.
SEM009. Every generated
doubleconstant now carries adsuffix, and SEM009 rejects a literal,operand, or quotient beyond the range of
double, and a non-zero value that rounds to zero.Parse exceptions.
StorageLiteraltreatsNotSupportedExceptionandArgumentExceptionfromTryParseas a failed parse. Nothing else is caught.Magnitude and factor.
QuantitiesGeneratornow multiplies both when a unit declares both, asUnitsGeneratoralready did. No unit declares both today, so no generated output changes.Risk
Values<T>members change from fields to properties over private fields. They areinternal, so no public surface changes. The committed diff is limited to
ConversionConstants.g.cs(the
dsuffixes and the holder shape),MetricMagnitudes.g.cs(the holder shape), andPhysicalConstants.g.cs(the three π literals and their descriptions).committed metadata is affected.
T.CreateCheckedper conversion again, as in 5.1.Testing
Each fix has a test that failed before the change:
IntegerStorageConversionTestscomparesintandlongfactories (FromKilometer,FromCentimeter,FromFoot,FromMile,FromHour,FromCelsius,FromFahrenheit,FromCurie) andIn(unit)against the 5.1 generated expressions with the 5.1 constants, including the
OverflowExceptionfromFromCurieonintand theDivideByZeroExceptionfromIn(Units.Fahrenheit).StorageMathTestscoversBigInteger2^2048 and 10^400 (exact roots), the smallest and largestdecimal, and zero.PiLiteralTestschecks all eight π literals to their last digit against π computed in the test.AMetricMagnitudeCombinedWithALongFactorIsExactreplacesAMetricMagnitudeIsExact, and failswhen
decimalis temporarily sent through thedoubleroute.GeneratorDiagnosticTestscovers SEM009 range rejection, compiles each accepted constant withRoslyn, and checks a unit with both a magnitude and a factor.
StorageLiteralTestsruns factories over a customINumber<T>whose parse throwsNotSupportedExceptionorArgumentException.The solution builds with no warnings.
Semantics.Testpasses all 1237 tests.Semantics.Cpp.Testpasses 29 of 33, and the 4 skipped tests are the C++ compiler tests that need
g++orclang++.Regenerating the committed generator output and the alias props leaves no diff beyond what this PR
commits.
🤖 Generated with Claude Code
https://claude.ai/code/session_01K5Bk9UjGdGUtC5C6qK5ZxD