Repository navigation
Duplicate-name ids, a model's MAT workspace for Usage, a lone config set, and struct-array element names - #127
Merged
Conversation
…rn at open A node id is its path, so two entries with one name in one section share one id, and so does everything inside them. The session index keeps the last one indexed. deleteNodeById, and every other operation addressed by id, then acted on whichever twin that was, so deleting the row the user picked could delete the other entry (F1-50). Only a malformed file holds such a pair, and both entries are kept as read. Per the 2026-10-16 group E ruling, option (a), no id changes. Instead: - findNodeById, deleteNodeById, addChildTo and editProperty refuse an id that names more than one node: the node itself, or any node above it, has a sibling with the same id. deleteNodesById refuses the whole batch when any id in it is shared, as it already does for a batch spanning two files. A host resolves a row through findNodeById before it edits or deletes it, so the refusal sits in that lookup too, not only in the session's own mutations. - At open, the source gets one warning per group of twins, under a new ParseWarningCode, 'duplicate-id'. part is the entry name. A clean file still carries no warnings field. - A host that renames or removes one twin through mutateSubtree or unindexSubtree gets the twin left behind back in the index, so the id resolves to it again. - findUsages is unchanged. It reads by name, and twins share the name, the source and the section, so its answer is the same for each twin. Refusing it would also make a row's UsedBy cell differ inside and outside a rowsOf batch. The check costs nothing on a well-formed file. It is gated on a set of ids that indexing found held twice, and that set is empty unless a file has twins. None of the 108 readable fixtures or the 335 readable corpus files has any, so no existing file warns. Reindex time on a 150,004-node dictionary is unchanged within noise. One existing test changes. test/publicTypeSurface.test.ts pins ParseWarningCode as a closed union, using an exhaustive switch over its codes. The ruling adds a code, so the switch gains the 'duplicate-id' arm.
A model can keep its workspace in a MAT file (WSDataSource 'MAT-File'). That file is the model workspace, so MATLAB looks in it right after the model's own workspace and before any data dictionary. Both usage engines looked in it last, with the model's other .mat files, because both parsers list it in externalDataSources and nothing else told it apart. So when the linked dictionary defined the same name, the usage was credited to the dictionary entry, which is the one definition the block does not read, and the .mat variable showed no usage. The parsers now also name the file on its own, as ParsedSlx.workspaceDataSource (null when the workspace is not kept in a MAT file). This is set by the package reader (blockDiagram.json and the legacy blockdiagram.xml) and by the classic .mdl reader. The file stays in externalDataSources, so a host that loads the externals still loads it, and nothing a host reads today changes. ModelNode carries it as workspaceDataSource, and ModelSummary as the optional workspaceMatRef. Both engines now look in that file right after the model's own workspace: UsageIndex.resolveName and the session's resolutionOrder. A summary or a host parse built before the field existed has none, and resolves as before. Tests (each new one failed before the change for the reason it names): - usageEngines: the same model as .slx and as a classic .mdl, linked to a dictionary that defines kp too. Both engines now credit the block to the .mat's kp and nothing to the dictionary's. Also the case where the settings part lists the same file as an external. Two controls pass before and after: a name only the dictionary defines, and an ordinary external .mat that still comes after the dictionary. - Parser tests for workspaceDataSource: .slx, legacy XML, modern and classic .mdl, deduplicated with the externals, and null when the workspace is not kept in a MAT file. - ModelNode and ModelSummary carry the field, by basename, and leave it out when there is no such file. resolveName puts it after the model's own workspace and before the dictionary. No existing test changed.
…ops a model opening C-255, per the maintainer's group E ruling of 2026-10-16 (recommendation (c), applied to both index parts). MATLAB always writes ConfigSetInfo (configSetInfo.json) and ModelReferences (graphicalInterface.json) as arrays: 68 of 68 and 46 of 46 across the 151 .slx in the repo and the corpus. A hand-edited or third-party package with another shape threw out of parseModelParts (a lone object is not iterable and has no .map; a non-string PartName has no .replace), so the whole .slx or package .mdl failed to open. A reference with no ModelName also made the open throw, later, in the references section. Now, in SlxParser.ts: - A lone object is read as a list of one, with no warning. - Any other non-array ConfigSetInfo (a string, number, boolean, null, or the key left out) is one part-unreadable warning on simulink/configSetInfo.json, and no sets are read. Before, the falsy ones were read silently as no sets. - A non-array ModelReferences that is a string, number or boolean is one part-unreadable warning on simulink/graphicalInterface.json. Absent or null stays silent: it is the normal case for a model that references nothing (78 of 124 parts). - An entry that is not an object, or that does not name both its part and its set (PartName, ConfigSetName) or both its block and its model (BlockPath, ModelName), is skipped with a part-unreadable warning. The entries around it are still read. The XML indexes and the classic .mdl reader already skip such entries and are unchanged. Tests: 37 new cases in test/parseWarnings.test.ts, driving parseSlx, parseMdl and a session open. Before the change, 34 failed (a throw, or no warning) and 3 controls (an absent, null or empty ModelReferences stays silent) passed. No existing test changed. dist rebuilt.
…avour C-058, per the maintainer's group E ruling of 2026-10-16: the naming half, recommendation (a). The nameEditable half is held for the maintainer and is not changed here. An array element has no name of its own. Its label is its MATLAB subscript, structNd(2,1,1), which BaseNode derives from _subscript, and that was already the same in every flavour. Its name, and so its node id, was not. MatlabVariableNode (a text dictionary's cdata, every .mat and model-workspace struct array) named elements 1, 2, .... Three other builders named them 0, 1, ...: - StructNode, for a binary dictionary's struct arrays and a text dictionary's 2-D ones; - ObjectNode's object-array loop, for object arrays in both dictionary flavours, MCOS object arrays in a .mat file, and a binary dictionary's enumeration arrays; - ObjectNode._addTextEnumElements, for a text dictionary's enumeration arrays. So MATLAB's one value structNd had the ids structNd/1 ... structNd/12 from the text dictionary and structNd/0 ... structNd/11 from the binary one, and BaseNode._slotAmongSiblings, which reads a name as its 1-based slot, missed on every 0-based element and fell back to a scan. All three now name element ei String(ei + 1). The subscript index stays ei, so no label changes. Nothing in src reads an element's name except _slotAmongSiblings, and no writer uses it. Public effect: the node ids of those elements change by one (.../0 becomes .../1). A host that persisted expansion or selection by id loses it once for those rows. Tests: new test/elementNameBase.test.ts, 29 cases, opening real fixtures through ingest: structNd in all four cases.* flavours, ten object and enumeration arrays, a text enumeration array against the binary one, and every subscripted element in 13 fixtures checked against its slot. Before the change 24 failed, each on a 0-based name; the 5 controls (text, .mat and model-workspace structNd, and the .slx and sparse slot checks) passed. Reverting each of the three sites alone fails 5, 19 and 3 of them. Changed tests, each pinning only the old 0-based name: - test/objectValueRefused.test.ts: other/widget/Parts/0 becomes Parts/1 (the same first element), in both custom_object fixtures. - test/matStructArray.test.ts: children[2] of a .mat Parameter array is its third element, now named '3' (five assertions in two tests). Docs: BaseNode's comment on the 1-based convention, StructNode.md, ObjectNode.md, and the DESIGN.md note that recorded the split, now retired. dist rebuilt.
This was referenced Oct 17, 2026
Merged
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
1 participant
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.
Four backlog items, one commit each. Every commit has its
dist/rebuilt (rm -rf dist && npm run build), andnpm run verifypassed at each one.The ruling, for all four: "Agree your suggestions, use subagents, teammates, or workflow agents to handle all of them automatically. Add unit tests to lock down the fix. I pre-approve PR and merge as long as it passes all tests." Themes 3, 4, 6 and 7 (C-004, C-058, F1-50, C-255 among them): "take each item's recommendation".
F1-50: two entries with the same name (98f8f7c)
Recommendation (a): "Refuse deleteNodeById and edits by id when more than one indexed node has that id, and warn at open. No id changes and no wrong-entry delete."
What was wrong: if one section held two entries with the same name, a delete by id could remove the other entry.
Root cause: a node's id is its path, so twins share one id, and so does everything inside them.
DataModel.indexSourcekept whichever twin was indexed last.Fix:
findNodeById,deleteNodeById,addChildToandeditPropertynow refuse an id that names more than one node.deleteNodesByIdrefuses the whole batch if any id in it is shared. At open, the file gets one warning per group of twins, with a newParseWarningCode,'duplicate-id'. If a host renames or removes one twin, the other is indexed again. No id changes. The check costs nothing on a well-formed file. No fixture and none of the 335 readable corpus files has twins, and reindex time on a 150,004-node dictionary is unchanged within noise.findUsagesis left as it is: it reads by name, so it gives the same answer for either twin.Tests: new
test/ambiguousId.test.ts, 14 tests, run through a real session. Before the fix, 12 failed for the right reason: no warning, a node returned where null was expected, a field added, or the twin left behind could not be found. The 2 controls passed.Changed existing test:
test/publicTypeSurface.test.ts. Its exhaustive switch pinsParseWarningCodeas a closed union, so it gains a'duplicate-id'arm.Left out (vscode, when it next bumps its core pin):
SlddModel.findNode's cache fallback must not hand back a twin that core refused.duplicate-idas a part that could not be read.binaryEntryScopedEdit.test.ts:651fails against this core. That test makes twins on purpose and asserts that a remove picks the wrong one. That is exactly what the ruling removes: the remove is now refused.C-004: a model workspace kept in a .mat (811f794)
Recommendation: "Take the additive option, with a synthetic .mdl test (WSDataSource 'MAT-File' plus a linked dictionary defining the same name) asked of both engines in usageEngines.test."
What was wrong: a model can keep its workspace in a MAT file. MATLAB looks in that file right after the model's own workspace and before the dictionary. Both usage engines looked in it last. So when the linked dictionary defined the same name, the usage went to the dictionary entry.
Root cause: both parsers list the file only in
externalDataSources, so nothing marked it as the workspace.Fix: a new field,
ParsedSlx.workspaceDataSource. It is null when the workspace is not in a MAT file. The.slx, legacy XML, modern.mdland classic.mdlreaders all set it.ModelNodecarries it asworkspaceDataSource, andModelSummaryas the optionalworkspaceMatRef.UsageIndex.resolveNameand the session'sresolutionOrdernow look in that file right after the model's own workspace. The file stays inexternalDataSources, so nothing a host reads today changes.workspaceMatRefis optional, so a summary that a host builds by hand is still valid.Tests: 21 new:
usageEngines(as.slxand as a classic.mdl, asked of both engines),blockParamUsages,mdlParser,usageIndexandmodelNode. Before the fix, 17 failed for the right reason. For example,signals.mat'skpgave[]where['K']was expected, and the dictionary'skpgave['K']where[]was expected. The 4 controls passed.Changed existing tests: none. The
slxModelhelper inusageEnginesgained anexternalsoption, and no assertion changed.Left out:
ModelBlockNode.paramSourceIdstill points a block's parameter link at the dictionary. That is navigation, not usage, and it lives in an area outside this item. dex-core's usage engine needs the same rule, but that is a separate repo.C-255: a config set or model reference index that is not a list (f8b6e34)
Recommendation (c): "Both: wrap a lone object, warn part-unreadable for any other non-array, and skip with a warning any entry whose PartName or ConfigSetName is not a string", "applied in the same change to extractModelReferences' ModelReferences".
What was wrong: a hand-edited
.slxwith a loneConfigSetInfoobject, or a non-stringPartName, could not be opened. The same was true ofModelReferences, or a reference with noModelName.Root cause: both values were cast to arrays with no check, then iterated and
.replaced.Fix (
SlxParser.tsonly):part-unreadablewarning on that part.ModelReferencesstays silent, as it is for a model that references nothing.Tests: 37 new cases in
test/parseWarnings.test.ts, run throughparseSlx,parseMdland a session open. Before the fix, 34 failed: they threw (for example "configSetInfo is not iterable", "refs.map is not a function"), or they gave no warning. The 3 controls passed: an absent, null or[]ModelReferences stays silent.Changed existing tests: none.
C-058: element names in struct, object and enumeration arrays (2fc29bb)
Recommendation: "(a) together with the nameEditable half of (c)." Option (a): "StructNode/ObjectNode stamp String(ei+1), matching MatlabVariableNode and BaseNode.ts:486's stated convention."
What was wrong: the same MATLAB value got element ids
structNd/1…12in a text dictionary butstructNd/0…11in a binary one.Root cause: three builders numbered elements from 0:
StructNode,ObjectNode's object-array loop, andObjectNode._addTextEnumElements.MatlabVariableNodenumbered them from 1.Fix: all three now use
String(ei + 1)._subscriptis unchanged, so no label changes. Comments inBaseNode,StructNode.mdandObjectNode.mdwere updated, and the note intest/parity/matlab/DESIGN.mdthat recorded the split is retired. Public effect: element ids under binary-dictionary struct arrays, 2-D struct arrays in a text dictionary, object arrays (both dictionary formats and.mat) and enumeration arrays change from…/0to…/1. A host that saved expansion or selection by id loses it once for those rows.Tests: new
test/elementNameBase.test.ts, 29 cases on real fixtures, run throughingest. Before the fix, 24 failed, each on a 0-based name. The 5 controls passed. Reverting each site alone (StructNode, the ObjectNode loop, the text enumeration builder) fails 5, 19 and 3 cases respectively.Changed existing tests (each pinned only the old 0-based name):
test/objectValueRefused.test.ts:60,64:other/widget/Parts/0becomesParts/1. It is the same first element.test/matStructArray.test.ts:479,487,516,525,529:children[2]is the third element, so its name becomes'3'.Left out: the nameEditable half is held for the maintainer. It rests on the premise that "a per-element field rename" happens, and this code says otherwise.
StructNode._renameFieldrenames a field on every element of the array. That is documented (StructNode.md, "the one structural change a struct array does allow"), it was a data-loss fix (cd72928), andstructNode.test.ts:212and:233pin it. Refusing the rename would remove that feature and leave no way to rename a struct array's field. So it needs the maintainer to confirm it.Gate
rm -rf dist && npm run buildleaves the tree clean.npm run verifypasses: 248 test files passed and 1 skipped, 8294 tests passed and 26 skipped. Main 641fca9 had 8193 passed and 26 skipped, so this adds 101 tests.dist/rebuilds unchanged, andnpm run verifypasses: 8347 tests passed and 26 skipped.binaryEntryScopedEdit.test.ts:651, from F1-50 (see above). The other isdimensionsMode.test.ts:43, which also fails against core main 641fca9 and comes from LookupTable, Breakpoint, VariantBank and Signal properties follow MATLAB #124.Implements the maintainer's 2026-10-16 group E ruling.