Three cross-path fixes, an asserted folder DAG, and the tests that hold them - #21
Merged
Conversation
A model names its references without an extension, so the bare name has to be
completed before it can be a link target or matched against a file on disk, and
it is completed with the PARENT model's own extension: a legacy .mdl hierarchy is
legacy throughout.
That guess was spelled /\.mdl$/i.test(name) ? '.mdl' : '.slx' in two places —
ModelNode.fromParsed here, for the node tree, and the vscode host's
common/fileTypes.refModelExt, for the usage graph, which resolves edges by
filename and so cannot use the bare name either. Two copies of one rule that has
to hold on both paths, or the tree row and the graph edge name two different
files for the same reference. Neither copy could see the other, and the only test
covering the pair asserted one fixture's outcome ('plant.mdl') rather than the
rule.
So the rule moves to fileKinds, beside the kind tests it belongs with, and is
published: the host can delete its copy on the next pin bump.
addReferenceEntry now asks isModelFile whether a name is ALREADY complete,
instead of its own /\.(slx|mdl)$/i — a third copy of a question this package
already publishes an answer to. Behaviour is unchanged (extOf takes the last
extension and lower-cases it, which is what the anchored case-insensitive regex
did), but a second opinion about whether 'plant.MDL' needs completing would
append one, and 'plant.MDL.slx' is a link to nothing.
Tests pin the two halves TOGETHER — which extension, and whether to add one —
across both containers and both cases, including the mixed-hierarchy case that
modelNameOf reconciles.
Four properties came from how `src/` is arranged, and were held in place by nothing but the habit of the people editing it: the main barrel is browser-safe, `./node` is the one entry allowed a `node:` builtin, `datamodel/` does not run `core/`, and the runtime module graph is acyclic. None of them fails where it would be broken. A `node:fs` in a parser breaks the CONSUMER's web bundle. A value import where an `import type` was breaks at bundle load, before any test body runs, because the node layer is a class hierarchy and one of the two modules in a cycle necessarily evaluates first and sees `undefined`. An undeclared dependency resolves here from a hoisted node_modules and is absent for anyone installing the package fresh. The walker is built around the one distinction all four turn on: whether tsc erases an edge. This package leans on that heavily and deliberately — 169 type-only edges, including `schemaBridge` and fifteen `Prop*` atoms naming `BaseNode` for typing alone — so a graph reader blind to the `type` keyword reports a dozen cycles here that do not exist, and would flag the one legitimate upward reach (`UsageIndex` naming `NodeUsage`) as a layering violation. It also blanks comments before matching, because the comments in this repo explain rules by quoting imports. Each assertion was checked against a mutation that should break it: a value import creating a cycle, a builtin in a parser, the barrel importing `./node`, `UsageIndex` value-importing `DataModel`, and an import of a package that is not a dependency. All five were caught, each by the intended test and by no other, except the `UsageIndex` one which correctly fails both the cycle test and the layering test at once. Tests only — no `src/` or `dist/` change. 4149 pass (+8), 26 skipped.
`find` over the two candidate spellings a relative specifier can resolve to, rather than a loop whose only body is an assignment and a break. Same two candidates, same first-match-wins order; the `?? null` keeps the "leaves the tree" answer explicit rather than leaving it to the initialiser.
`fileKinds` was written because eight readers each spelled their own
`endsWith('.sldd')`, every copy case-SENSITIVE while the globs that ADMITTED the
file were not. Four readers in this package never switched over, and two of them
were wrong in exactly the way the module's own header describes.
`DataSourceNode` is the visible one. A model records a linked file the way its
author typed it, and `refBasename` lower-cases precisely so `Params.SLDD` inside
a model resolves to `params.sldd` on disk. Its icon and its class name did not,
so a capitalised dictionary got a working link and a MAT-file presentation: a row
that is quietly wrong about what it points at, with nothing to notice. Those two
getters were also two independent `endsWith` chains answering one question, so a
kind added to one and not the other yields a dictionary icon labelled 'MAT File'.
They now derive from a single classification, and the test asserts that as a
partition rather than as a table, so it pins the relationship and not the
vocabulary.
`ModelSectionNode.addDataSourceEntry` split the source path on `/` alone, but the
string is whatever MATLAB wrote into the model, so a model saved on Windows
records `..\shared\signals.mat` — and the whole path became the node's name.
The Name column showed a path and `linkTarget` was a path, while the link still
resolved, because that matching goes through `refBasename`, which splits on both.
`basenameOf` is the one place that rule lives.
`ProjectItemNode` and `ModelNode.sourceFormat` were already correct; they now say
so through the same module instead of by hand. Filling the branch under
`sourceFormat` turned up a real gap: nothing stopped the `.mdl` test from
matching anywhere in the name, and `serializeSource` picks a WRITER by that
field, so `mdl_library.slx` would be offered to Simulink as the wrong container.
`parseProject` is public and takes a project NAME, not a filename — the name a
project calls itself, which is also its fallback when the `.prj` metadata is
missing. So every caller has to strip the extension first, and there are two:
this package, before it builds a project's node tree, and the vscode host, before
it builds its own index over the same parse. Both spelled
`basename.replace(/\.prj$/i, '')` by hand.
That is the third rule of this shape, after the eight `endsWith('.sldd')` copies
and the model-reference extension guess, and it fails the same way: the name is
user-visible on both sides — a tree row and a graph group heading — so a drift
between the copies shows one project under two names, with neither side looking
wrong alone. `projectNameOf` is now published beside `refModelExt` and
`isProjectFile`.
It is total where `modelNameOf` is nullable, and the doc comment says why: null
there means "not a model, do not compare stems", whereas the answer wanted here
is always a label, and a null would only be re-defaulted back to the filename at
each call site. It also does not take a path, matching its sibling, so callers
compose it with `basenameOf` the way `UsageIndex` already does for models — the
composition is pinned, because a half-answer that strips the extension off a
whole path reads fine at a call site.
`addProjectSource` also had the last hand-rolled basename in `src/`; it now uses
`basenameOf` too, keeping the `|| srcId` fallback for an empty path.
A `.sldd` holds every entry in one part, spelled two ways: the zip member `data/chunk0.xml` in a compressed-binary dictionary, and the JSON key path `__MW_TEXT_PARTS__` -> `__MW_TEXT_PART__/data/chunk0` -> `__MW_TEXT_content` in a textual one. Both were literals at seven sites here — and six more in the vscode host, which owns the open document this package cannot touch — with nothing checking that the sites agreed. None of the disagreements would have thrown. `parseBinarySldd` excludes the data part from the pass-through bag and `serializeBinarySldd` puts it back; two spellings there produce a valid zip carrying the old entries under the name a reader looks up and the new ones under a name nothing reads. Two writers build the JSON path — the binary reader and `SlddNode.serializeJson` — and `slddChunkContent` alone reads it; a drifted writer yields null, which SlddNode reports as `source-empty`, indistinguishable from a dictionary the user had just created and not filled in. SlddContent's own header records that failure already happening once. So SlddParts.ts owns the names, derived from one stem because the same `chunk0` is what makes the two spellings the same part, and the barrel publishes them for the reason it publishes SC_PART: the host looks the member up, excludes it, and re-inserts it, and splices byte offsets into the raw JSON rather than re-serializing it. test/slddParts.test.ts checks what the constants cannot check for themselves. It spells the strings out as literals, since a test that asked a constant what it is would pass for any value, and it walks the JSON path by hand over the raw bytes of a MATLAB-written textual fixture, so the shape is pinned to a real dictionary and not just to this package's own writers. The reader-excludes-what-the-writer- inserts assertion closes a genuine gap: the existing 4157 tests all pass with that exclusion drifted. Tests in this package go on spelling the literals. Rewriting the fixtures to use the constants would remove exactly the check that says the constants are right.
A dictionary entry's property bag is what the file held, and MATLAB writes only the properties it has something to say about. Every node in the data cluster therefore gates its write-back on one question — was this key on disk, or has the user since set it — and five of them spell that gate twice, once for the compressed-binary path and once for the JSON text one. Two hand-written copies of one decision can drift, and a per-path test that asserts a literal about one of them cannot see the drift. absentPropertyWriteBack.test.ts asserts the agreement instead of either side's literals: the same keys, in all three states the gate distinguishes, for every class. classWithoutElement.test.ts covers the same duplication in its second shape, the envelope unwrap copy-pasted into five parse methods. slddNode.test.ts covers the dictionary root — what it says about its file's format, and what it does with a content part that arrived short of a list it needs. Also drops a statement in summarizeFiles that could not run: a .sldd key can never collide with a .mat one, because refBasename keeps the extension. Its comment claimed an eviction policy that is not in force and should not be — params.mat and params.sldd are two files, and both belong in the answers. Coverage: node/container 87.25 to 95.58 branches, node/data 90.50 to 92.49, BinarySlddSerializer 69.56 to 95.65, MatWriter 80.95 to 95.23, UsageIndex 84.48 to 89.66, ParseWarning 50 to 100. 4221 tests pass, up from 4166.
Emptying the Minimum or Maximum box on a Parameter or a bus element blanked the row and then saved the number that was already in the file. The bound came back on reopen, and nothing reported that the edit had been dropped. Both classes kept the value the file arrived with in a `_rawMin`/`_rawMax` field and fell back to it whenever the live property was undefined -- which is exactly the state clearing the box produces. SignalNode already wrote this correctly, so the rule had three hand-written copies and one of them disagreed. A cleared bound is now written as `[]`, which is what MATLAB itself stores: it spells an empty bound `<P Name="Min" Class="double" Dimension="0*0"/>` in a compressed-binary dictionary, and `elem.Min = []` is the assignment it accepts to clear one. `undefined` is not the alternative -- one property bag feeds both save paths and serializePropertyXml spells `undefined` as `Class="char"`, an empty char where a double belongs. The stored fields are gone rather than left unread, so there is no longer a copy of the old value for a later change to reach for. One residual difference from MATLAB, pinned in the tests rather than claimed as parity: an uncompressed-text dictionary gets `"Min": []` where MATLAB omits the key. Both read back as no bound. test/absentPropertyWriteBack.test.ts covers the rule for both classes across both save paths, asserting what a reopened file shows as well as the binary XML spelling that rules the `undefined` shortcut out.
SlddNode.parse spelled `__MW_TEXT_PARTS__` itself and the catalog reader spelled `__MW_TEXT_content` itself, while serializeJson in the same file writes both through SlddParts -- two copies of one name, which is the drift SlddParts exists to close. The reader is the half that fails quietly. A mismatch on the catalog key does not empty the dictionary, because the entries come through slddChunkContent; it loses only the System Composer catalog, and a missing catalog is reported as "no catalog part" and then classified from the raw Simulink class -- a StructType read as a Data Interface, which looks exactly like a right answer. No behaviour change: the constants hold the strings that were there. What changes is that slddNode.test.ts, which builds its fixtures through the same constants, is now the only place the names are written down.
The premise at the top of this file -- MATLAB writes only the properties it has something to say about -- was stated without evidence, and it is the thing a reader has to trust before any assertion below means anything. It is now measured in both formats, with the fixture that shows it. The claim that needed correcting is that binary always emits every key, present-but-empty. It does not: a Simulink.Parameter given no bound has no Min tag at all in object_array_binary.sldd, which MATLAB wrote. A bound explicitly assigned [] is written. So presence-in-the-file separates "never set" from "set to empty" in both flavours, which is exactly what makes keying on presence rather than on truthiness the right gate. Also measured: R2027a reads a text dictionary's "Min": [] as class double, size [0 0], isempty 1, and isequal to an omitted-key entry returns 1 -- so the note's "reads it back as the same absent bound" is now a measurement rather than an argument. And the part worth knowing that nobody would guess: MATLAB does not normalize the spelling away. It re-serializes only the entries it modified, so editing a neighbouring entry leaves "Min": [] byte-identical and an entry the user never touches again keeps it indefinitely. The divergence is durable, not self-healing. Comments only.
…s that cannot fire Three reductions found by the simplifier pass, none of them behaviour: - ParameterNode._getSerializedProperties spelled `this.serial._properties as Record<string, unknown>` four times, once per guard. It is now the local `sp` — the name SignalNode and BusElementNode already give the same bag — with a note on why every guard asks it rather than the outgoing copy. - SlddNode.parse tested `!content` and then `content`, as two statements. `else`, since nothing between them can change the answer. - addProjectSource's `basenameOf(...) || srcId` fallback cannot fire: basenameOf returns its whole input when it finds no separator, so it is falsy only for an input that was empty, and that input is srcId itself. Both halves of that are pinned in test/fileKinds.test.ts. And one that is: ModelSectionNode.addReferenceEntry no longer defaults defaultExt to '.slx'. A default is a second answer to "which extension does a bare reference name take", which fileKinds.refModelExt owns; taken silently by a caller that forgot the parent's, it labels a .mdl hierarchy .slx and links to nothing. No caller took the default, so the argument is now required and omitting it is a compile error.
The cross-path assertion in absentPropertyWriteBack.test.ts covered four classes. Five more have the same two-hand-written-copies shape and nothing compared them: - Breakpoint, LookupTable and CustomObject gate only Description, and today their two methods are character-for-character identical. That identity is not a property of the code — it is still two copies — so they join CASES, which asserts both paths name the same keys in all three states the gate distinguishes. - ConfigSet's Name and ConfigSetRef's SourceName are deliberately UNGATED, like an AliasType's BaseType: they identify the entry rather than describe it, so MATLAB gets them even as the empty string a half-built entry carries. What the two paths must agree ON there is that the key is unconditional, so they get the sibling of the existing BaseType test rather than a CASES row. The ConfigSet half also pins the rename, which is the drift its own class comment records: ConfigName used to be stored independently, both bags read it, and a renamed entry saved under its old name and reverted on reopen. Header count corrected to eleven with the classes enumerated, so it can be checked rather than trusted. Every new assertion mutation-tested: dropping the gate from ONE path in BreakpointNode, CustomObjectNode or LookupTableNode fails the cross-path test naming that class, and gating ConfigSet's Name in serializeValue alone fails the new unconditional test on the text bag. 4230 -> 4231.
…s still right This file's header justified the whole write-back gate with "MATLAB writes only the properties it has something to say about", called that measured in both formats, and cited test/fixtures/object_array_binary.sldd as the evidence. The binary half is false and the citation does not support it. Measured, R2027a Prerelease 27.1.0.3353139, on dictionaries MATLAB wrote itself: a Simulink.Parameter whose Min was never touched still gets <P Name="Min" Class="double" Dimension="0*0"/>, and its entry block is byte-identical to one explicitly assigned Min = [] once Name/UUID/LastMod are normalized out. A property set to 5 and then cleared lands in that same state, in both formats. isequal on the two in memory is 1, so there is no "was it ever assigned" bit to serialize even in principle. Description, DocUnits, DataType, Complexity and Dimensions behave the same way, as do Simulink.Signal and Simulink.ValueType: the behaviour is writer-wide, not per-property. The cited fixture is not MATLAB output. It has no UUID, no LastMod, no CoderInfo on any of its three parameters -- a real MATLAB-written Simulink.Parameter always carries one -- no DD.Dictionary trailer, and its entire chunk sits on one unindented line where MATLAB pretty-prints with four-space indents. It could never have been evidence about what MATLAB emits. The text half stands: an uncompressed-text .sldd really does omit the key for an empty Description or bound. So the two formats encode emptiness by different mechanisms -- binary as Dimension="0*0" versus a value body, text as an absent key -- and presence distinguishes "never set" from "set to empty" in neither. None of which makes the gate wrong, which the header now says explicitly so the correction does not read as a licence to remove it. Its real rule was never the premise it was written under: preserve exactly the key set the file had, and add a key only where the user set a value. That holds whatever the upstream writer emits, and it is what keeps a no-edit save from producing a source-control diff. What the measurement changes is the gate's reach -- on MATLAB-written binary the `in stored` half always fires, so the live audience is bags some other writer produced: our own fixtures, older releases, and this extension's own re-saves.
…ave path A dictionary entry that saves as a Simulink object has two save paths: _getSerializedProperties, iterated into <P> tags for a compressed-binary .sldd, and serializeValue, whose override bag becomes the JSON of an uncompressed-text one. Every class in this cluster stated its written properties twice, once per path, in two hand-written copies of the same gate. Nothing objected if only one copy moved. Teach a class a second property, or tighten one gate, and the result is a dictionary that saves differently in its two flavours -- a key written into the binary file and missing from the text one, from the same model in the same session. Either file on its own looks right, which is what makes that class of difference hard to notice. New SimulinkObjectNode holds the shape: _serializedOverrides() is the one place a subclass names what it saves, and both inherited paths read it. Thirteen classes now extend it and delete both methods. The truthiness gate is stated once as _gatedProps. Deliberately unchanged: the two paths still MERGE that one list differently. _getSerializedProperties assigns straight over the stored bag; serializeValue reaches _mergeProps, which knows about MATLAB's saveobj envelope. Making them merge alike would change what one path writes for every class here, so it is a separate question from where the list lives. VariantVariableNode is the one class whose binary path is envelope-aware, and it now says so by overriding just that path to put the same list through _mergeProps. Three classes stay off the new base, for reasons worth recording. ValueTypeNode keeps its own DataType predicate rather than using _gatedProps: 'double' is what an absent DataType means and is also truthy, so the shared test would write that default into every dictionary saved without edits. SignalNode's two paths write different values for a cleared bound ([] in binary, undefined in text so JSON.stringify drops the key), which one shared list cannot express. ParameterNode already funnels one path through the other. Output-identical, including key order, which matters because both writers iterate the bag: Object.assign keeps a key the stored bag already carried in its original position and appends a new one in override order, exactly as the sequential assignments did. absentPropertyWriteBack.test.ts pins the two paths against each other for eleven classes and passes untouched. Mutation-checked: dropping the `in stored` half of the shared gate fails naming the emptied-Description case, dropping the value half fails seven tests across four files, and routing ValueType's DataType through the shared truthiness test fails three including "serializes back out without inventing properties the file did not have".
The file-level import graph has been asserted acyclic for a while. The FOLDER graph
was not, and it had exactly one cycle: `datamodel/node -> datamodel/node/data ->
datamodel/node`. Every file in it was fine, which is why nothing caught it — a
directory cycle can exist while no single file participates in one.
The whole downward half was one file. `NodeClassMap` enumerates the twenty-four entry
classes in `node/data/` to register them, and sat in `node/` beside the base classes
those subclasses extend, so it imported DOWNWARD against the layering twenty-four
times while the subclasses imported upward. Moved beside the classes it enumerates,
which is where a registry of them belongs; the cycle is gone for one `git mv` with no
interface inverted and no behaviour changed. Its two side-effect importers
(`src/index.ts`, `src/node/index.ts`) name the new path; nothing else imported it,
and the barrel's exported surface is identical.
Then two assertions so it cannot come back, and three that pin what is already true:
- The directory graph is acyclic, reported as the folder pair AND the import lines
that make each leg, because "a cycle exists" is not something a reader can act on.
- `datamodel/schema/` and `datamodel/display/` import nothing outside themselves.
`schema/index.ts` already CLAIMED this in its header ("the seed of a future
standalone dex-schema package; dependencies point INTO it only"); now it is checked.
- `datamodel/parser/` — 7600 lines, the bulk of the package — reaches for exactly one
module outside itself, `datamodel/blockIdentity.ts`, and that exemption is asserted
to still be USED, so the test cannot start passing because the seam quietly stopped
existing.
These are the seams a package split would cut first, and they cost an `export` rather
than an invented interface. `datamodel/node/` is bigger and deliberately not bounded:
it is entangled with the session, the props and the schema.
Six mutations, all run by hand rather than taken on report. Restoring the pre-move
`NodeClassMap` fails the directory test and NOTHING else — eleven passing, including
the file-level rule — which is the whole argument for the new test existing. A stray
outbound import in schema, in display, and in parser each fails exactly its own
assertion and prints the offending line; deleting the two `blockIdentity` imports
fails the exemption-still-used check; and misspelling a folder name fails on "holds
modules" instead of passing vacuously on an empty list.
4231 -> 4235 tests.
`MatlabVariableNode.ts` was 2725 lines, and the first 200 of them were not about being a node at all: what class a value keeps across an edit, when a number cannot be spelled as a bare JSON literal, how a matrix prints and how that printing reads back. None of it reads node state — that is exactly why it was written as module-local functions rather than methods — so it can be a module, and now it is: `matlabValueRules.ts`, 221 lines, imported by the one file that used it. Pure motion. Every comment came across unshortened, and the header collects the three MATLAB quirks these functions encode so none of them has to be rediscovered as a bug: JSON's single number type (which is why `TYPED_NUMERIC_CLASS`, `classAfterEdit`, `elementClass` and `needsTypedLiteral` are four views of one set), the column-major storage of a char matrix, and int64/uint64 exceeding a double's exact range by construction. It also names a trap: `formatMatrix` here is the DISPLAY formatter, while `BinarySlddParser` has its own module-local `formatMatrix` that builds the serial `Matrix(...)` form — same name, opposite ends of the pipeline. Two module-level declarations deliberately stayed behind, because they are about this node's own jobs rather than about the value it holds: `MCOS_ICON_MAP`, the icon a tree row shows, and `MI_MATRIX`, the one MAT-file tag the cdata reader looks for. What moved was module-private and is now exported, which is a real widening of visibility — stopped at the package boundary on purpose: none of these names is added to `src/index.ts` or `src/node/index.ts`, so a consumer cannot start depending on one. Three mutations, run rather than assumed. `classAfterEdit` returning `parsedType` unconditionally fails 11 tests, including "keeps the class when a Value edit re-states the number" and the whole defect-42 exact-integer group. Indexing `formatCharMatrix` row-major fails 7, printing `['ac'; 'bd']` where MATLAB prints `['ab'; 'cd']`. Dropping `needsTypedLiteral`'s non-finite arm fails "round-trips a non-finite scalar back out as a typed literal, not JSON null". So the moved code is still under test at its new address. 4235 tests, unchanged, and the same 26 skipped: no behaviour moved, only lines.
`core/DataModel.ts` was 2257 lines: eight type declarations, a query compiler, and a 1900-line `createSession` that is the session. The first two need nothing from the third and are now `core/sessionTypes.ts` (204 lines) and `core/findQuery.ts` (144). DataModel.ts is 2004. `sessionTypes.ts` is the CONTRACT — what a caller hands `createSession` and what its methods hand back: `LinkResolution`'s four arms, `NodeUsage`'s seven fields, `SerializedSource`, `FindNodesQuery`. A host's actual question is "what does resolveLink give me back, and what are the failure arms for?", and answering it meant opening the session that implements it. It is now one file, and it holds types and nothing else deliberately: every import of it is erased by tsc, so it can never create a load-order edge or pull bytes into a consumer's bundle. `Session` stayed behind because it is `ReturnType<typeof createSession>` and cannot leave the implementation. `findQuery.ts` is the "decide what matching MEANS" half of `findNodes` — the whole of it, and it reads no session state. Its own header records what deliberately stayed in `findNodes`: criteria combine with AND, and a query with no criteria matches NOTHING rather than everything, because both of those are facts about the walk, not about compiling a test. The three corners a query API's behaviour really lives in are now readable without 1900 lines of session around them: an empty search box is not a match-all, a caller's `/g` pattern is cloned stateless (or `test` would advance `lastIndex` and match every second node, in the caller's own object), and a getter that throws makes its criterion false rather than empty. No importer changed anywhere: DataModel.ts re-exports every name it exported before, from the same path. That was the condition for the move being free, and it is load- bearing in two places — `src/index.ts` publishes those names onto the package barrel, and `datamodel/usage/UsageIndex.ts` reaches up for `NodeUsage`, which is the one upward edge from `datamodel/` into `core/` that `test/moduleBoundaries.test.ts` allows by name and on condition it stays type-only. Both headers say so, so nobody "cleans it up" later. The comment on that test was updated to stay true; the assertion is untouched. Also dropped one import that the move made dead: `EventBusInstance`, used only by `CreateSessionOptions`. `tsconfig.json` sets `strict` but not `noUnusedLocals`, so typecheck could not have caught it — after moving declarations out of a file here, its remaining imports have to be re-read by hand. Three mutations, run rather than assumed. `isAsked` accepting `''` fails "treats an empty string as no criterion rather than as a match-all". Neutering the `global || sticky` clone fails "matches every node with a global RegExp, not every other one". Making `textCriterion`'s catch return true fails "survives a node whose value cannot be read, and does not match it". One failure each, each naming the rule. 4235 tests and 14 barrel exports, both unchanged; the export-name set of DataModel.ts is identical before and after.
Typing `{[1;2]}` into a cell gave back `{[1 2]}`, and saveChanges wrote the
1x2. tokenizeCellElements read each element with its own code per bracket
kind rather than with the parser's own parse, and the `[` arm pushed the flat
element list from parseArray, dropping dims. A bare JSON list has nowhere to
carry [2,1] and reads back as a row — which is what _serializeArray already
says in the comment above its own typed-literal fallback.
Five element kinds were wrong, not one: a numeric column and matrix reshaped,
a logical array retyped to double (a bare list says double), a char matrix
flattened to its column-major codes so the text scrambled, a string array
retyped to char and flattened, and a 1x1 string retyped to char. `{"a"}`
showing as `{'a'}` was the same defect as `{[1;2]}` showing as `{[1 2]}`.
Parse every element recursively and convert the result once, in cellElementRaw,
so an element of a cell is read exactly as a value of its own and the serial
grammar keeps its single owner in formatMatrixSerial/formatMxCharSerial. Each
arm of the converter is a line MATLAB wrote itself: probe_cell_shapes.m builds
a text dictionary of seventeen cell values and prints the bytes. Two of those
spellings are not free choices — a logical row keeps a typed envelope the
double row does not need, and a 1x1 string is the one-element list ["a"],
both because the bare form already states a class.
Two existing assertions move: a double-quoted element inside a cell is now a
string rather than a char, which is the parity the recursion buys.
cellElementShape.test.ts holds it — 24 tests over the raw spelling, the
display, and a round trip through the value's own displayed text in both .sldd
flavours; 21 fail with the fix reverted. It also pins defect 50, found here
and deliberately not fixed: the parser assembles a cell's own element list
row-major while every reader stores one column-major, so editing a multi-row
cell transposes it. MATLAB writes {1 2; 3 4} as _elements [1, 3, 2, 4].
Committing a 2x3 cell wrote {1, 4, 2; 5, 3, 6} — every off-diagonal element
moved — and retyping the displayed text transposed it again, so the value
never settled. A 2x3 string array did the same. MatlabValueParser assembled
both element lists row-major while every reader delivers one column-major,
and MATLAB's own file agrees with the readers: probe_cell_shapes.m measures
{1 2; 3 4} as _elements [1, 3, 2, 4].
This is defect 14 again on the other path. That fix made the readers agree
with MATLAB and pinned the READ path's answer, so the write path could stay
wrong with a green suite — which is the argument for pinning the invariant
between two paths rather than each path's answer.
Give the flatten a name and an order: flatten(matrix, cols, 'row' | 'column'),
called 'column' from the cell and string-array returns and 'row' from the
numeric one. The numeric exception is not an inconsistency — formatMatrixSerial
re-transposes that list on the way out — and "make them all the same" is a
third mutation the tests catch. charFromRows had the column-major rule right
from the start, one value type away.
The new block in cellElementOrder.test.ts states no order of its own: it
retypes each fixture's own displayed text and demands MATLAB's truth.json
subscript-to-value pairs back, in both .sldd flavours, with mat2x3 as the
row-major control. Three mutations, all caught: cell to row-major fails 6
tests, string to row-major 4, numeric to column-major 14 across seven files.
Only a 1xN or an Nx1 was unaffected, which is why this survived the cell-shape
fix that found it: a vector flattens to the same list either way, and every
editable cell fixture in the corpus is a vector.
Three behavioural fixes since 1.12.0, all of them the same shape — a rule
applied on one path and not on its twin:
- clearing a Min or Max on a Simulink.Parameter or a bus element was silently
discarded on save, and the old bound came back on reopen;
- a shaped value typed into a cell lost its shape on screen and in the file
({[1;2]} became {[1 2]}, and four other element kinds went the same way);
- a multi-row cell or string array was transposed on commit.
The rest is test coverage and structure: the folder graph is now an asserted
DAG, the MATLAB value rules and the session's vocabulary and query compiler
have their own files, each entry class names its written properties once
rather than once per save path, and five constants that were spelled in
several places have one owner each.
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.
Nineteen commits. Three behavioural fixes, all the same shape — a rule applied on one path and not on its twin — plus the structure and test coverage that make that shape visible.
Fixes
A cleared Min or Max was silently discarded on save.
ParameterNodecaptured_rawMinat parse time and never updated it, soprops.Min = this.Min !== undefined ? this.Min : this._rawMinsent a cleared bound straight back to the value on disk. Same shape inBusElementNode;SignalNode— the same rule, a different hand-written copy — was already right. The fallback is now[], which is MATLAB's own spelling of an empty bound in a binary dictionary (Dimension="0*0"), measured rather than assumed:undefinedwritesClass="char"where a double belongs.A shaped value typed into a cell lost its shape, on screen and in the file.
{[1;2]}came back{[1 2]}andsaveChangeswrote the 1x2.tokenizeCellElementsread each element with its own code per bracket kind rather than with the parser's ownparse, and the[arm pushed the flat element list withdimsdropped. Five element kinds were affected, not one — a numeric column and matrix reshaped, a logical array retyped to double, a char matrix flattened to its column-major codes so the text scrambled, a string array retyped to char, and a 1x1 string retyped to char. Every element now goes throughparseand one sharedcellElementRawconverter, so an element of a cell is read exactly as a value of its own. Each arm of that converter is a line MATLAB wrote itself;test/parity/matlab/probe_cell_shapes.mis the probe.A multi-row cell or string array was transposed on commit. The parser assembled both element lists row-major while every reader delivers one column-major, and MATLAB agrees with the readers (
{1 2; 3 4}is_elements: [1, 3, 2, 4]). A 2x3 cell came back with every off-diagonal element moved, and retyping the displayed text transposed it again.flatten(matrix, cols, 'row' | 'column')now says which order each type wants; the numeric exception is deliberate, becauseformatMatrixSerialre-transposes that list on the way out.Structure
moduleBoundaries.test.ts), which pins theparser/→schema/→display/seams instead of packaging them prematurely.Verification
npm run verifygreen at this commit: typecheck clean, 4268 tests passed / 26 skipped across 141 files (4235 before this branch), smoke OK, tarball dist-only, leak check clean.Every test added was mutation-verified — deliberately broken to confirm it fails and that the failure names the right thing. 93 mutations through the coverage phase (92 caught first try; the one miss became a test that then caught it), plus four for the cell fixes: reverting the shape fix fails 21 tests, and the three order mutations fail 6, 4 and 14.