Skip to content

AIGEN: Fix iterator compilation bug with value class field access (#769)#1077

Open
mbouaziz wants to merge 2 commits into
mainfrom
fix-769-iterator-compilation-bug
Open

AIGEN: Fix iterator compilation bug with value class field access (#769)#1077
mbouaziz wants to merge 2 commits into
mainfrom
fix-769-iterator-compilation-bug

Conversation

@mbouaziz

@mbouaziz mbouaziz commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixed issue Iterator compilation bug: reached an Unreachable point #769 where the compiler incorrectly marked code as unreachable when accessing fields of value classes (like Some<T>.value) if the field type appeared as unresolved type _ during compilation
  • The bug occurred with lazy iterator chains using flatMap and filter with yield
  • Moved the canInstantiate check inside the NamedLeaf branch only (for reference classes which require scalarization), allowing value class field access to proceed without this check
  • Removed the .collect(Array) workaround in SqlSchemaMigration.sk

Verification

Verified end-to-end by building Docker images with STAGE=1 (stage1 compiler compiled from source, which includes this fix) and running the full skdb test suite:

# Build with the fixed compiler
STAGE=1 bin/docker_build.sh --arch amd64 skiplang skip skdb-base

# Clean stale build artifacts (static_state.db format mismatch between compiler versions)
docker run --rm -v $PWD:/work skiplabs/skdb-base:latest \
  bash -c "find /work -name static_state.db -delete"

# Run tests
docker run --rm --network=host -v $PWD:/work -w /work \
  skiplabs/skdb-base:latest bash -c "make test-native"
docker run --rm --network=host -v $PWD:/work -w /work \
  skiplabs/skdb-base:latest bash -c "make test-wasm"

Test plan

  • All 1698 compiler tests pass
  • skdb native tests: 1265 tests pass (including ALTER TABLE ADD COLUMN which exercises schema migration with the lazy iterator code path)
  • skdb wasm tests: 283/283 pass
  • The generic_refinement_method test continues to pass (regression test for the fix)
  • Verified locally by reviewer

Fixes #769

🤖 Generated with Claude Code

@mbouaziz

mbouaziz commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

This PR was created using no other guidance than "Fix #769"

@mbouaziz mbouaziz requested review from jberdine and pikatchu February 6, 2026 23:41
@mbouaziz mbouaziz force-pushed the fix-769-iterator-compilation-bug branch from 56c71a9 to 77317fa Compare February 9, 2026 13:32
@jberdine

jberdine commented Feb 9, 2026

Copy link
Copy Markdown
Contributor

This PR was created using no other guidance than "Fix #769"

Did you review it?

@mbouaziz

mbouaziz commented Feb 9, 2026

Copy link
Copy Markdown
Contributor Author

I did review but I didn't take time to collect enough context to understand it deeply.
Anyways, CI is still red so Claude is still clauding...
turning the PR to draft

@mbouaziz mbouaziz marked this pull request as draft February 9, 2026 16:08
@mbouaziz mbouaziz force-pushed the fix-769-iterator-compilation-bug branch from 77317fa to 3acdd84 Compare February 20, 2026 11:51
@mbouaziz

Copy link
Copy Markdown
Contributor Author

CI setup for this PR

This PR includes a temporary second commit that:

  1. Updates the bootstrap submodule — regenerated from stage1 with the specialize.sk fix (make -C skiplang/compiler promote using the fixed compiler)
  2. Points CI images to fix-769 tag — Docker images (skiplabs/skiplang:fix-769, skiplabs/skip:fix-769, skiplabs/skdb-base:fix-769) were built locally with the new bootstrap and pushed to Docker Hub (amd64 only)

This allows CI to run the full test suite using a compiler that includes the fix, verifying end-to-end correctness.

Before merging, the second commit (ci: use fix-769 Docker images and updated bootstrap) should be squashed/dropped and the bootstrap submodule updated separately through the normal process.

Local verification results

  • Native tests: 1265 pass (including ALTER TABLE ADD COLUMN which exercises the fixed code path)
  • Wasm tests: 283/283 pass
  • Compiler tests: 1698 pass

@mbouaziz mbouaziz marked this pull request as ready for review February 20, 2026 13:37

@jberdine jberdine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change LGTM.

It would be a good idea to fully redo the test after rebasing.

@mbouaziz mbouaziz force-pushed the fix-769-iterator-compilation-bug branch from bc8a01d to 56bb92c Compare July 2, 2026 15:22
@mbouaziz

mbouaziz commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Redo after rebase (2026-07-02)

Rebased onto current main (was 653 commits behind; clean rebase). Re-reviewed the diff and tightened the comment in specialize.sk explaining why the NamedInner branch doesn't need the canInstantiate check.

CI setup for this PR (updated)

The temporary CI commit is simpler than before: it only points the CI jobs at fresh fix-769 Docker images. No bootstrap submodule update is needed anymore, because:

  • the compiler job now builds stage0 → stage1 from source and tests stage1, which includes the fix regardless of the bootstrap;
  • the images (skiplabs/skiplang:fix-769, skiplabs/skip:fix-769, skiplabs/skdb-base:fix-769) were rebuilt today from this branch with STAGE=1, so their toolchain is compiled from source and includes the fix — that's what the skdb/skdb-wasm jobs use to compile sql/src without the .collect(Array) workaround.
STAGE=1 bin/docker_build.sh --push --arch amd64 skiplang skip skdb-base -- \
  --set skiplang.tags=skiplabs/skiplang:fix-769 \
  --set skip.tags=skiplabs/skip:fix-769 \
  --set skdb-base.tags=skiplabs/skdb-base:fix-769

Before merging: drop the ci: use fix-769 Docker images commit, and update the bootstrap through the normal process — the post-merge :latest images are built STAGE=0 from main's bootstrap, so without a promoted bootstrap their toolchain would lack the fix and skdb tests on main would hit the #769 trap.

mbouaziz and others added 2 commits July 3, 2026 10:42
The compiler was incorrectly marking code as unreachable when accessing
fields of value classes (like Some<T>.value) if the field type appeared
as unresolved type `_` during compilation. This happened with lazy
iterator chains using flatMap and filter with yield.

The fix moves the canInstantiate check inside the NamedLeaf branch only
(for reference classes which require scalarization), allowing value class
field access to proceed without this check since it's just bookkeeping.

This removes the .collect(Array) workaround in SqlSchemaMigration.sk.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Temporary commit, to be dropped before merge: point CI jobs at the
fix-769 image tags, built from this branch with STAGE=1 so that the
toolchain in the images includes the specialize.sk fix (needed to
compile sql/src without the .collect(Array) workaround).

Co-Authored-By: Claude Fable 5 <[email protected]>
@mbouaziz mbouaziz force-pushed the fix-769-iterator-compilation-bug branch from 56bb92c to bee8b0d Compare July 3, 2026 08:42
@mbouaziz

mbouaziz commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Test redo complete — all green ✅

As requested, the full test suite was redone after the rebase:

  • Rebase: clean rebase onto current main (was 653 commits behind), no conflicts.
  • Re-review: change re-verified after rebase; the NamedInner comment in specialize.sk was tightened while at it. InstrTree still has exactly two variants, so the match remains exhaustive; canInstantiate is a pure query, so restricting it to the NamedLeaf branch changes no other behavior. Also verified that the now-lazy iteration in resetReactiveViews is safe: nothing in the loop body writes to the views directory being iterated.
  • CI (on fresh fix-769 images built from this branch with STAGE=1): compiler (stage0→stage1 built from source + full test suite), skdb (native tests, including ALTER TABLE ADD COLUMN which exercises the un-workaround-ed code path — the live regression test for Iterator compilation bug: reached an Unreachable point #769), skdb-wasm, and fast-checks all pass.

Merge plan

  1. Bootstrap promoted from this branch's stage1 (make -C skiplang/compiler promote) and submitted as a separate PR — the normal process. This must land before this PR merges: the post-merge :latest images are built STAGE=0 from main's bootstrap, so an unpromoted bootstrap would ship a toolchain without the fix and break skdb tests on main.
  2. After the bootstrap PR merges: rebuild and push the :latest images from main.
  3. Drop the temporary ci: use fix-769 Docker images commit from this branch (CI then runs on :latest images, which by then contain the fix).
  4. Merge.

mbouaziz added a commit that referenced this pull request Jul 3, 2026
## Summary

Bumps the `skiplang/compiler/bootstrap` submodule to a bootstrap
regenerated from a stage1 compiler that includes the #769 value-class
field-access fix (moving the `canInstantiate` check into the `NamedLeaf`
branch only).

Bootstrap commit:
[SkipLabs/skiplang-bootstrap@763ee89](SkipLabs/skiplang-bootstrap@763ee89)
(built on top of the current `c352962`).

## Why

This is the prerequisite that unblocks the merge of #1077. Once this
lands on `main`, the `skiplabs/*:latest` Docker images can be rebuilt
(they are built `STAGE=0` from `main`'s bootstrap, so they will then
carry the fixed compiler). #1077 can then drop its temporary `ci: use
fix-769 Docker images` commit and go green on `:latest`.

This PR is intentionally minimal: it changes only the submodule pointer,
so `sql/src` here still contains the `.collect(Array)` workaround and CI
passes on the current `:latest` images. The `compiler` job builds
`STAGE=1` from the new bootstrap and exercises the fixed compiler
directly.

Related: #1077, #769

🤖 Generated with [Claude Code](https://claude.com/claude-code)
pull Bot pushed a commit to edisplay/skip that referenced this pull request Jul 3, 2026
Bumps the bootstrap to include the value-class field-access fix
(specialize.sk: canInstantiate check moved into the NamedLeaf branch).
Prerequisite for SkipLabs#1077: once this is on main, the :latest images rebuild
with a fixed compiler so SkipLabs#1077 can drop its temporary fix-769 image pins.

Bootstrap source: SkipLabs/skiplang-bootstrap@763ee89fa2b3

Co-Authored-By: Claude Fable 5 <[email protected]>
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.

Iterator compilation bug: reached an Unreachable point

2 participants