Skip to content

reject extending an inline table after it is defined#487

Open
sahvx655-wq wants to merge 1 commit into
BurntSushi:masterfrom
sahvx655-wq:inline-table-immutable
Open

reject extending an inline table after it is defined#487
sahvx655-wq wants to merge 1 commit into
BurntSushi:masterfrom
sahvx655-wq:inline-table-immutable

Conversation

@sahvx655-wq

Copy link
Copy Markdown
Contributor

The inline-table cases under internal/toml-test were already on the skip list with a "we allow appending to tables, but shouldn't" note, which is what put me onto this. valueInlineTable builds the table and hands back its map, but nothing records that an inline table is a closed, self-contained definition, so a later table header or dotted key walks straight back into it. a = {b = 1} then a.c = 2, a = {} then [a.b], and tab = { inner = { dog = "x" }, inner.cat = "y" } all parse without error even though the spec says an inline table cannot be extended once its braces close. Unfixed, this silently accepts malformed documents and mutates a value the author meant to be immutable.

I record each inline table's full key path in a closed set the moment it finishes building, and reject extension where the traversal actually happens: the parent descent in addContext (table headers) and addImplicitContext (dotted keys). Arrays of tables reuse the same key path for every element, so a new element clears the closed entries beneath it, otherwise the second [[arr]] element would wrongly inherit the first element's inline tables. Putting the check next to the traversal covers both the header and dotted-key routes in one spot rather than at each call site. Six toml-test invalid fixtures that were skipped for this reason now pass.

@sahvx655-wq

Copy link
Copy Markdown
Contributor Author

gentle ping

@arp242 arp242 force-pushed the inline-table-immutable branch from d9ebdd9 to 9594c02 Compare June 27, 2026 13:52
@arp242

arp242 commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

The entire way it keeps context confuses me every time I look at it (I didn't write it), and to be honest I would much rather fix/rewrite it properly instead of heaping on more stuff making it even harder to understand.

@sahvx655-wq

Copy link
Copy Markdown
Contributor Author

Fair enough, the context tracking is genuinely hard to follow. I didn't write it either and spent a fair while just working out where the traversal actually happens before touching anything. I tried to keep this to the smallest footprint I could: one set recording the closed key path, checked at the two points where a header or dotted key re-enters an existing table, rather than scattering guards across the call sites. But I take the point that any addition to that code makes the next read harder, not easier.

Happy to leave this parked if you'd sooner rewrite the context handling properly. If it's any use, the six un-skipped toml-test fixtures pin down exactly the behaviour that's currently wrong, so they'd serve as a ready regression net for whenever that rewrite lands. No rush from my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants