Make short and enum symbols work - #88
Draft
HowardvanRooijen wants to merge 1 commit into
Draft
Conversation
A short symbol could not be solved at all, and neither could an enum. Two defects in series, either of which hid the other. Translation: C# widens short to int for a comparison, and an enum's TypeCode is that of its underlying type, so both arrive at the Int32 arm of the conversion switch - which assumed the target type told it the operand's Z3 sort and cast an IntExpr to RealExpr. The arm now checks the operand: a value already at integer sort passes through, and only a real goes to MkReal2Int. The Char arm three lines below already did exactly this. Marshalling: TypeCode.Int16 shared the Int32 arm and handed reflection an int, which a short member rejects. It gets its own arm with a checked cast. Checked, not plain: the symbol is an unbounded MkIntConst, so Z3 can pick a value no short can hold, and an unchecked cast wraps 40000 to -25536 - a wrong answer that looks like a right one. Raised as #87, which would bound the symbol so the situation cannot arise. Enums need nothing on the marshalling side. #63 predicted a second defect there; reflection converts an int to an enum member on its own, measured rather than assumed. Also removes a switch on the operand type whose only cases were empty breaks, and drops the two characterisation pins these replace. Co-Authored-By: Claude Opus 5 (1M context) <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 #63.
The defect
shortis listed among the supported symbol types -TypeCode.Int16is in the sort mapping andin both marshalling switches - but no theorem over a
shortcould be solved. Two defects inseries, and neither was reachable while the other stood.
1. Translation. A
shortsymbol is created withMkIntConst, so it is anIntExpr. C#widens
shorttointfor the comparison, and the visitor read thatConvertnode's targettype as telling it the operand's Z3 sort:
2. Marshalling. Behind it,
TypeCode.Int16shared theInt32arm and returned anint,which reflection refuses to write to a
shortmember -ArgumentException: Object of type 'System.Int32' cannot be converted to type 'System.Int16'.Enums take the same route: an enum's
TypeCodeis that of its underlying type, soDayOfWeekisInt32, gets anMkIntConst, and then fails identically. That was reported as a comment on #63and is fixed here by the same change.
The change
The conversion arm now reads the operand, not the target. A widening onto a value Z3 already
holds at integer sort is a no-op; only a real needs converting:
The
Chararm three lines below already did exactly this. Whoever wrote it had the right idea anddid not carry it across.
TypeCode.Int16gets its own marshalling arm, with a checked cast toshort. That choiceis load-bearing and is pinned by a test - see below.
Also removed: the switch on the operand type immediately above, whose only two cases were empty
breaks. #63 guessed it was "possibly the vestige of an intended fix", which reads right - itswitches on exactly the thing the fix needed to consult, and then does nothing with it.
Enums need nothing on the marshalling side. #63's comment predicted a second defect there, by
analogy with
short. There is not one: the model value is anint, and reflection converts anintto an enum member on its own. Measured directly(
PropertyInfo.SetValue(new EnumEnvironment(), 1)succeeds) rather than assumed, and the mutationmatrix below shows it independently - reverting the marshalling arm fails nine tests, none of them
an enum one.
Why the cast is checked
The symbol is an unbounded
MkIntConst. Nothing tells Z3 the value has to fit in 16 bits, so aconstraint written against the widened
intcan be satisfied by a number the member cannot hold:An unchecked cast wraps 40000 to -25536 - a wrong answer that looks like a right one. Measured
both ways. Throwing is the lesser of the two available evils; the better answer is to bound the
symbol so Z3 cannot pick the value at all, which is raised as #87. The behaviour is pinned so
that a later simplification to a plain cast fails rather than silently starting to wrap.
C# blocks the direct spelling -
t.X1 == 40000against ashortiserror CS0652- so the reproneeds a variable. That is also why this went unnoticed.
What is deliberately not changed
The array-element copy of the
Int16arm (Theorem.cs:502) still shares theInt32case. Itis unreachable behind #64 - a
short[]dies earlier, in the visitor, because the array sortmapping gives
Int16aMkBitVecSort(16)range while the scalar mapping usesIntSort:So no test can cover a change there, and shipping an unverifiable edit seemed worse than recording
it. Noted on #64 for whoever fixes it.
#76 is untouched. It is the
Doublearm of the same switch and fails for the same reason onearm along, and #76 suggests doing the two together. Confirmed still failing identically after this
change (
RatNumtoIntExpr), so this PR neither fixes nor disturbs it. Kept separate becausethey are separate lines with separate repros, and #76 has no pin yet.
Enums with a non-
intunderlying type stay unsupported. Abyte-backed enum isTypeCode.Byte, which the sort mapping has never handled, so it stops at the guard that rejects abare
byteoruint:NotSupportedExceptionnaming the member. That is the outcome #63 askedfor where a type genuinely is not supported, and it is now pinned.
Tests
202 -> 214.
Solve_ShortSymbol_RoundTripsTheValueInt16boundaries - replaces theThrowsInvalidCastExceptionpinSolve_ShortSymbolConstrainedOutsideShortRange_ThrowsOverflowExceptionSolve_ShortSymbolsInArithmetic_RoundTripTheValuesConvertwherever ashortis used in arithmetic, not only in a comparisonSolve_ShortSymbolComparedToAnIntSymbol_RoundTripsBothSolve_UnconstrainedShortSymbol_ReturnsAResultshortcould not be in beforeSolve_EnumSymbol_RoundTripsTheValueUnsupportedExpressionTestsSolve_EnumPropertyWithAnUnsupportedUnderlyingType_ThrowsNotSupportedExceptionSolve_DoubleSymbolCastToInt_ConvertsRatherThanPassingThroughThe one that nearly did not work
Adding the guard dropped branch coverage by one. The branch it pointed at was the real-to-int
conversion: before this change that line was reachable only from a widening the visitor misread,
so every execution of it threw. It was covered without ever having worked, and no test
exercised a genuine
(int)cast of a real symbol.The first version of the test asserted
(int)t.X1 == 3on adoublesymbol. It passed - andpassed just as happily under a mutation that removed the guard entirely, because Z3 satisfies that
constraint with
X1 = 3.0, where truncating and not truncating agree. The test now addst.X1 > 3.5, which the two readings disagree about: with the conversion the answer is a real in(3.5, 4), and without it the theorem is unsatisfiable. Recorded because the weak version looked
exactly as convincing as the strong one.
Mutation results
when inner.IsIntguardshorttest and every enum testInt16shares theInt32marshalling arm againshorttest - and no enum test, confirming enums need no marshalling changecheckedtouncheckedThe first two barely overlap in what they catch, which is the point: the two defects are
independent and each is covered on its own.
Verification
dotnet build solutions/Z3.Linq.slnx -c Release- clean,TreatWarningsAsErrorson./build.ps1 -Configuration Release- 46 tasks, 0 errors, 0 warningsnew branch is covered in both directions
New issue
#87 - a
shortsymbol is not bounded toshort's range, so Z3 can pick a value it cannothold. Includes what I could and could not reach for
int: the same hole exists in principle,IntNum.Intthrows aboveintrange, but I could not construct a theorem that gets there.Release 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.