fix(split): visited reporting non existing refs - #272
Open
ShaMan123 wants to merge 1 commit into
Open
Conversation
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.
IfcSplitter.split()reports ids that no line in the file definesCaught when reviewing code with claude.
My take
I believe this is why the parser should use web-ifc instead of re-inventing the wheel - and address perf using the stream parser #252 and other dedicated fixes
Summary
split()returnsMap<groupId, { path, ids }>. Theidsset includes express ids that are not written to the output file and do not exist in the input file at all — ids that only ever appeared as a reference, never as an entity definition.extract()is affected the same way; it sharescollectDeps.Reproduction
Self-contained, public API only:
Two ways to trigger it, one in each wall:
#12holds#999in an attribute slot — a reference to a line the file never defines (ordinary in exports that drop entities).#15has#99999inside a quoted string,'Wall B (see #99999)'.Actual
The highest id defined anywhere in the input is 17. Neither
999nor99999is written toout_0.ifc/out_1.ifc.Expected
Root cause
collectDeps(andcollectDepsAll) mark an id visited before discovering whether it resolves:getRefsreturnsnullonly for an id that was never passed toLineIndex.set, i.e. one with no line of its own —setrecords a zero-length ref list for defined lines with no references, so an empty result is distinguishable from a missing one. By then the id is already invisited, which is the group'sfileIdsset and is whatsplit()returns asids.The ids get there because
extractRefsscans a line's raw text for#+ digits with no validation that the target exists.Impact
idsis the natural input for building an id → group map. Sized bymax(ids)it allocates for ids nothing can ever query; that bound is attacker/exporter-controlled through string content — a 9-digit number in a description field asks for a multi-GB array.GroupData.totalIdsover-reports.Fix
const id = stack.pop()!; if (visited.has(id)) continue; - visited.add(id); const refs = index.getRefs(id); if (!refs) continue; + visited.add(id);in both
collectDepsandcollectDepsAll. Nothing downstream regresses: no line is ever emitted for these ids, so removing them changes only what is reported. It also shrinks the sets slightly.Related, not fixed by the above
extractRefs(packages/fragments/src/Utils/ifc-parsing-utils.ts) is not string-aware, so a#Nin any quoted text is read as a reference. WhenNhappens to name a real entity, that entity and its dependency subtree are pulled into the group — a text mention changes the output file's contents. Changing'Wall B (see #99999)'to'Wall B (see #10)'in the reproduction above adds10to group 1, which otherwise has no claim on it.Whether the parser should skip single-quoted strings is a separate call — it touches every caller of
extractRefs, not just the splitter.