Skip to content

fix(lsp): unify the two type short-name indexes that broke the main build - #1993

Merged
DeusData merged 1 commit into
mainfrom
fix/unify-type-short-name-index
Sep 1, 2026
Merged

fix(lsp): unify the two type short-name indexes that broke the main build#1993
DeusData merged 1 commit into
mainfrom
fix/unify-type-short-name-index

Conversation

@DeusData

@DeusData DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

main does not compile. Every open PR is red for this reason, not for anything in the PR.

internal/cbm/lsp/type_registry.h:131:10: error: duplicate member 'type_short_buckets'
internal/cbm/lsp/type_registry.h:132:27: error: duplicate member 'type_short_entries'
internal/cbm/lsp/type_registry.h:133:9:  error: duplicate member 'type_short_bucket_count'
internal/cbm/lsp/type_registry.h:293:6:  error: conflicting types for 'cbm_registry_types_by_short_name'

How it happened

Two changes implemented the same feature — a type short-name index — independently:

commit change iterator
a4c0ffbc perf(cs): eliminate the C# corpus-proportional scans (16 Aug, ours) CBMTypeShortIter
793716dc perf(lsp): index C++ short-name type lookups (#1679, @astandrik) CBMTypeNameIter

They added their members to different hunks of the same struct and their declarations to different parts of the same header section. Git merged them with no conflict, and nothing failed until the merge commit was built.

That is the whole lesson here: a clean merge is not evidence that two changes compose. Neither PR was individually wrong, and neither CI run was individually wrong — #1679 was last built against a main that did not yet contain the other index.

What this does

The two iterators are structurally identical — same five fields, byte-identical _next bodies walking the same reg->type_short_entries chain. They differ in exactly one place, and it is not cosmetic:

  • perf(lsp): index C++ short-name type lookups #1679 — on auxiliary-index allocation failure sets tail_i = 0, so the iterator degrades to a full types[] scan and the candidate set is unchanged.
  • a4c0ffbc — sets tail_i = reg->type_qn_entry_count unconditionally, so on that same failure it skips the first type_qn_entry_count types and silently yields an incomplete candidate set.

So this keeps a4c0ffbc's names and #1679's body:

  • CBMTypeShortIter / cbm_type_short_iter_next survive — they read correctly beside the type_short_* members and beside CBMTypeEmbedIter and CBMFreeFuncIter.
  • The retained implementation is perf(lsp): index C++ short-name type lookups #1679's, including the allocation-failure fallback. C# gains that correctness; C++ keeps it.
  • The surviving declaration carries both rationales, since the C++ short-name lookup and cs_resolve_type_name's step-9 fallback are two consumers of one index.

The merge had also broken two comment/member pairings, repaired here: the duplicated members displaced the free-function index members from under their own comment, and the CBMTypeShortIter block had been interleaved between CBMFreeFuncIter's typedef and its declarations.

Verification

  • c_lsp, cs_lsp, registry881 passed, 0 failed
  • make -f Makefile.cbm lint-format — clean
  • full local suite running; result posted here before merge

#1679's tests carry over unchanged apart from the type name — including the fault-injection case that NULLs the buckets and asserts the full-scan fallback, which is precisely the property being preserved.

…uild

main does not compile. internal/cbm/lsp/type_registry.h declares
type_short_buckets, type_short_entries and type_short_bucket_count twice, and
cbm_registry_types_by_short_name twice with different parameter types.

Two changes implemented the same feature independently:

  a4c0ffb  perf(cs): eliminate the C# corpus-proportional scans  (2026-08-16, ours)
            CBMTypeShortIter, cbm_type_short_iter_next
  793716d  perf(lsp): index C++ short-name type lookups     (PR #1679, astandrik)
            CBMTypeNameIter,  cbm_type_name_iter_next

They landed in different hunks of the same struct and the same header section,
so git merged them with no conflict and nothing failed until the merge commit
was built. This is the failure mode where a clean merge is not evidence that
two changes compose.

The two iterators are structurally identical -- same five fields, byte-identical
_next bodies reading the same reg->type_short_entries chain. They differ in one
place, and it matters:

  #1679          on aux-index allocation failure, sets tail_i = 0, so the
                 iterator degrades to a full types[] scan and the candidate
                 set is unchanged.
  a4c0ffb       sets tail_i = reg->type_qn_entry_count unconditionally, so on
                 that same failure it SKIPS the first type_qn_entry_count types
                 and silently yields an incomplete candidate set.

So this keeps a4c0ffb's names -- CBMTypeShortIter reads correctly beside its
type_short_* members and beside CBMTypeEmbedIter and CBMFreeFuncIter -- and
#1679's body, which is the correct one. The surviving declaration carries both
rationales: the C++ short-name lookup and cs_resolve_type_name's step-9
fallback are now stated as two consumers of one index, which is what they are.

The merge also broke two comment/member pairings, repaired here: the duplicated
members had displaced the free-function index members from under their own
comment, and the CBMTypeShortIter block had been interleaved between
CBMFreeFuncIter's typedef and its declarations.

No behaviour change for either consumer beyond the allocation-failure path,
which strictly gains coverage for C#. c_lsp.c and cs_lsp.c now call one
function. #1679's tests, including the fault-injection case that NULLs the
buckets and asserts the full-scan fallback, carry over unchanged apart from the
type name.

Local: c_lsp, cs_lsp and registry -- 881 passed, 0 failed.

Co-Authored-By: astandrik <astandrik@yandex-team.ru>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner Author

Full local suite, as promised in the description:

7781 passed, 7 skipped

Zero failures, exit 0 — macOS host, ASan/UBSan build. That is every suite, not just the three affected ones (c_lsp, cs_lsp, registry were 881 passed / 0 failed on their own).

Worth stating what this does and does not establish. It establishes that unifying the two iterators onto one type and one implementation breaks nothing anywhere in the tree, which is the risk of a fix like this — the surviving body is now serving a consumer it was not written for. It does not establish the allocation-failure path on its own; that is carried by #1679's fault-injection test, which NULLs the buckets and asserts the iterator still yields the full sequence, and which now covers the C# consumer as well as the C++ one.

Remaining CI: analyze, codeql-gate, lint, and the three pr-smoke legs.

@DeusData
DeusData merged commit 1778637 into main Sep 1, 2026
36 checks passed
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.

1 participant