Choose numeric conversions by sort, not by target type - #92
Draft
HowardvanRooijen wants to merge 1 commit into
Draft
Choose numeric conversions by sort, not by target type#92HowardvanRooijen wants to merge 1 commit into
HowardvanRooijen wants to merge 1 commit into
Conversation
A double symbol could not be compared against a float variable. C# widens the float, and the visitor read the Convert node's target type as telling it what the operand was: every conversion to double was integer-to-real, so a real operand was cast to IntExpr and failed. The same assumption meant two symbols of different real types could not be related at all, and long, float and decimal had no arm whatever - a long symbol could not be compared to an int one. A numeric conversion is one of three things in Z3, decided by the sorts: nothing when both types map to the same sort, integer-to-real, or real-to-integer. The visitor now asks TryGetSymbolSort - the one mapping the symbols are declared with, internal from here - for the target sort and decides from that and the operand. The arm #63 added for short and enums was this rule for one row and is subsumed; its tests still fail when the rule is wrong. The pin that recorded (long)t.X1 as unsupported is repointed at (byte), which the mapping genuinely has no row for, so the catch-all stays covered. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
Fixes #76.
The defect
A
doublesymbol could not be compared against afloatvariable:C# widens the
floattodouble, so the tree carries aConvert(Single -> Double)node, and thevisitor chose the conversion from the node's target type alone: every conversion to
doublewasMkInt2Real, on the assumption that the operand must have been an integer. Here it was a real.It is wider than the issue says. Measured before the change, sixteen shapes:
doublesymbol==floatvariable - the issueInvalidCastExceptiondoublesymbol==(double)of afloatordecimalvariableInvalidCastExceptiondoublesymbol==floatsymbolInvalidCastExceptiondoublesymbol==floatsymbol* 2InvalidCastExceptionlongsymbol==intsymbolNotImplementedException- no arm forlongat alldecimalsymbol==intsymbolNotImplementedException- no arm fordecimalfloatsymbol==intsymbolNotImplementedException- no arm forfloatfloatsymbol==(float)of adoublevariableNotImplementedExceptiondoublesymbol==int,longorshortvariable, orintsymboldoublesymbol==floatliteraldoubleconstant before the visitor sees it, which is why the issue needed a variableThe bold rows are two symbols of the library's own supported types that could not appear in the
same constraint. Nothing about
Symbols<double, float>orSymbols<long, int>suggests itsmembers cannot be compared.
The change
A numeric conversion means one of three things in Z3, and which one depends on the sorts, not
the CLR types: nothing at all when both types map to the same sort, integer-to-real, or
real-to-integer. The visitor now asks for the target type's sort from the same mapping the
symbols are declared with -
TryGetSymbolSort, which #64 made the single source for thatquestion and which becomes
internalhere - and decides from that and the operand's sort:That replaces the switch on the target type, including the
when inner.IsIntarm #63 added forshortand enums - which was this rule for one row. The mutation matrix shows the #63 coveragecarrying over: dropping the same-sort branch fails every
shortand enum test #63 wrote.A conversion to a type the mapping has no row for still falls through to the existing catch-all,
with its message unchanged.
One behaviour changed on purpose beyond the issue. The
chararm passed an integer operandthrough and threw for a real one; a real operand converted to
charnow goes throughMkReal2Int, as it does forint. No test exercised thechararm in either direction; it iskept as an integer-sort target so the integer case is unchanged.
Measured after the change
All sixteen shapes round-trip: the issue's repro gives
1.5,double == floatsymbol gives1.5on both sides,double == float * 2gives3,long == intsymbol gives5,decimal == intsymbol gives7,float == intsymbol gives3, and every shape that workedbefore still gives the same value.
Tests
230 → 239. A new
NumericConversionTests.csfor the shapes above, since they are a subject oftheir own rather than a marshalling detail, and one pin repointed.
Solve_DoubleSymbolComparedToAFloatVariable_RoundTripsTheValueSolve_DoubleSymbolComparedToAFloatSymbol_RelatesTheTwo/..InArithmeticWithAFloatSymbol..Solve_DoubleSymbolComparedToAnIntSymbol_RelatesTheTwoSolve_DecimalSymbolComparedToAnIntSymbol_RelatesTheTwo/..Float..Solve_LongSymbolComparedToAnIntSymbol_RelatesTheTwoSolve_DoubleSymbolComparedToAWidenedDecimalVariable../Solve_FloatSymbolComparedToANarrowedDoubleVariable..Solve_CastToAnUnmappedType_ThrowsNotImplementedException(inUnsupportedExpressionTests)(long)- which now works - to(byte), which the mapping has no row forThe real-to-integer direction already has its test from #63,
Solve_DoubleSymbolCastToInt_ConvertsRatherThanPassingThrough, which stays where it is.Mutation results
shortand enum test from #63, theshortcollection tests from #64, and the anonymous every-scalar test from #75double,decimalandfloatsymbols against anintsymbol - and nothing else in the suite, which is why the regression guard is thereThe first row is the useful one: four earlier fixes in this stack all ran through the arm this
change replaces, and all of them still fail when it is wrong.
Verification
dotnet build solutions/Z3.Linq.slnx -c Release- clean,TreatWarningsAsErrorson./build.ps1 -Configuration Release- 46 tasks, 0 errors, 0 warningsRelease note
Releases remain on hold under #60 until Microsoft.Z3 5.x reaches nuget.org, so this reaches
mainbut not consumers. Nothing about the hold changes.