fix: correct property introspection in meta.Type - #103
Open
ehennestad wants to merge 1 commit into
Open
Conversation
isPropertyValueScalar only looked for a mustBeScalarOrEmpty validator on linked and embedded properties. For any other property it consulted the size declaration instead, which is (1,:) for the generated type classes, so a property restricted to a scalar by a validator was reported as non-scalar. openminds.core.miscellaneous.Membership/startDate is one example. Both constraints are now checked for every property. The size branch also raised 'Not implemented.' for any dimension that was neither fixed nor unrestricted. A dimension that is not fixed leaves the property unconstrained in size, so this now reports non-scalar rather than erroring. isPropertyMixedType read Validation.Class.Name unguarded and errored on a property declared without a class, rather than answering that it is not a mixed type. getMixedTypeForProperty had the same problem and now raises an identified error instead of failing on the unguarded read. The round-trip synthesizer inspected validators itself to work around the scalar bug. It now uses isPropertyValueScalar directly. Co-Authored-By: Claude Opus 5 <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.
Two defects in
openminds.internal.meta.Type, both found while building the round-trip test synthesizer in the previous PR in this stack. Neither is currently reachable from the serializer, which is why they have gone unnoticed, but both are reachable by any other consumer of the meta layer — including the traversal core proposed in #69.isPropertyValueScalar reported scalar properties as non-scalar
A property can be restricted to a scalar in two ways: a
mustBeScalarOrEmptyvalidator, or a(1,1)size declaration. The method only looked for the validator on linked and embedded properties. For every other property it consulted the size declaration instead.The generated type classes declare almost everything as
(1,:)and express the scalar constraint through the validator, so any property holding a primitive value that is restricted to a scalar was reported as non-scalar.openminds.core.miscellaneous.Membership/startDateis one example: it is declared(1,:) datetime {mustBeScalarOrEmpty, mustBeValidDate}, assigning two values correctly fails, andisPropertyValueScalarreturned false.The serializer is unaffected because it only asks about linked and embedded properties, which take the branch that worked.
Both constraints are now checked for every property.
The size branch also raised
'Not implemented.'for any dimension that was neither fixed nor unrestricted, with a comment questioning whether that could happen. A dimension that is not fixed leaves the property unconstrained in size, so that case now reports non-scalar rather than erroring.isPropertyMixedType errored on a property with no declared class
The method read
Validation.Class.Namewithout checking that a class was declared. A property declared without one produces an emptyValidation.Class, and the read fails withInsufficient number of outputs from right hand side of equal sign to satisfy assignmentrather than answering the question.openminds.neuroimaging.device.MRIScannerUsage/fieldOfViewis declaredfieldOfView (1,:)with no class and is registered inLINKED_PROPERTIESwith an empty allowed-type list, so it reproduces this today.A property without a declared class cannot be a mixed type, so the check now returns false.
getMixedTypeForPropertyhad the same unguarded read and now raisesOPENMINDS_MATLAB:MetaType:NotAMixedTypeinstead.Note that the
fieldOfViewdeclaration itself looks like a gap in the generated classes rather than something to fix here. It is worth raising separately against the pipeline.Tests
MetaTypeTestgains three cases. The scalar constraint is pinned twice: once againstommtest.helper.PropertyDeclarationFixture, a small class added for this purpose that carries one property of each declaration shape, and once againstMembership/startDateso the behaviour is also pinned for a real generated type.The no-declared-class case is tested only against the fixture. Testing it against
MRIScannerUsage/fieldOfViewwould tie the test to a schema anomaly that may well be corrected upstream, at which point the test would start failing for the wrong reason.The round-trip synthesizer added in the previous PR inspected validators itself to work around the scalar defect. That workaround is removed here, which also serves as a check that the fix covers the case it was written for.
🤖 Generated with Claude Code