Size collections from the instance passed to NewTheorem - #93
Draft
HowardvanRooijen wants to merge 1 commit into
Draft
Size collections from the instance passed to NewTheorem#93HowardvanRooijen wants to merge 1 commit into
HowardvanRooijen wants to merge 1 commit into
Conversation
A collection symbol takes its length from the collection already on the solution instance, and read it with no null check: an environment that declared a collection without initialising it threw a bare NullReferenceException naming nothing. A value tuple could never hold a collection at all - the instance is built with Activator.CreateInstance, and a tuple has nowhere to put an initialiser. The instance passed to NewTheorem(T) is now the template for the solution. It was a parameter named dummy that the method discarded; its collections now give the solution's collections their length, ahead of any initialiser on the type, and it is carried through Where and into the optimiser. Nothing else about it is read. A collection with no length from either source is rejected by name, with the reason. That is what lets a tuple carry a collection, and an anonymous type too - the guard #75 added for anonymous collections is gone, since the template is exactly the initialised instance it was missing. 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 #78.
The defect
A collection symbol takes its length from the collection already on the solution instance, and
read it with no null check:
Two halves, as the issue puts it. The diagnostic half: a forgettable mistake produced a bare
NRE with nothing naming the member - in an environment with several collections, nothing said
which. The structural half: a value tuple could never hold a collection at all, because the
instance is built with
Activator.CreateInstanceand a tuple has nowhere to put an initialiser.Nothing the caller could write fixed that.
Measured before the change: a null field, a null property, a null
List<T>, a null collection ona nested object, and a tuple - with or without element constraints - all threw the same bare NRE.
The change
The instance passed to
NewTheorem(T)is now the template for the solution. That overloadalready existed; its parameter was named
dummyand the method discarded it. Its collectionsnow give the solution's collections their length, so:
works, and so does an anonymous type with a collection - which #75 had to refuse by name two PRs
ago, for exactly the reason this fixes: the anonymous instance is created uninitialised, and the
initialised one the caller passed was being thrown away.
Rules, each pinned by a test and a mutation:
they want more directly than the type has. With no template, the initialiser applies as before.
of the template alongside the environment.
Where(each of which builds a new theorem) and reaches the optimiser, soOrderBysees it.template is never written to.
Collection symbol B has no length. A collection must be pre-sized: initialise it on the environment type, or pass an instance with it initialised to NewTheorem.The
//todo: deal with length in a more robust waythat sat above the count read for years isgone with it.
Public surface
Z3Context.NewTheorem<T>(T dummy)becomesNewTheorem<T>(T template)and reads the argument.Binary-compatible; source-breaking only for a caller using the named argument
dummy:. The XMLdocumentation on the overload is rewritten to say what the instance is for, with a tuple example
beside the anonymous-type one.
This was put to the maintainer against the diagnostic-only fix the issue suggested as the safe
minimum, with the measurements above, and chosen.
Tests
239 → 247. Three pins rewritten in
CollectionSymbolTests, one inEnvironmentTypeTests, andnew coverage for each rule.
Solve_NullCollectionInAPublicField_ThrowsNotSupportedExceptionNamingIt/..Property..Solve_TwoCollectionsWithOneUnsized_NamesTheUnsizedOneAis sized,Bis not, and the message saysBSolve_CollectionInAValueTupleEnvironment_IsSizedByTheTemplateSolve_CollectionInAValueTupleEnvironmentWithoutATemplate_ThrowsNotSupportedExceptionItem1, which is what the field is called at runtimeSolve_NullCollectionProperty_IsSizedByTheTemplate/..NullGenericCollection../..InANestedObject..List<T>, and a collection two levels downSolve_TemplateCollection_TakesPrecedenceOverTheInitialiserSolve_TemplateElementValues_DoNotReachTheSolution[99, 99], solution constrained to1; the template is untouched afterwardsOrderByDescending_OnATemplatedTheorem_KeepsTheTemplateSolve_AnonymousTypeWithACollectionProperty_SizesItFromTheTemplate(inEnvironmentTypeTests)Mutation results
Wheredrops the templateWhereOrderBytest aloneVerification
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.