Skip to content

fix(module): define GNU dynamic-module error symbols - #478

Merged
eval-exec merged 1 commit into
eval-exec:mainfrom
thanosapollo:fix/module-error-symbols
Oct 7, 2026
Merged

eval-exec merged 1 commit into
eval-exec:mainfrom
thanosapollo:fix/module-error-symbols

Conversation

@thanosapollo

@thanosapollo thanosapollo commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Why

module-load signals module-open-failed and the other module-load-failed errors (and the dynamic-module runtime signals memory-buffer-too-small and invalid-arity), but none of GNU's dynamic-module error symbols had error-conditions or error-message. A failed dlopen therefore surfaced as (error "Invalid error symbol" module-open-failed) instead of a catchable module-open-failed, and handlers for module-load-failed never matched. The MELPA parity notes already record this symptom (Invalid error symbol: module-open-failed in the closql / sqlite3-api case).

Change

Register the complete syms_of_module set from GNU src/emacs-module.c, with GNU's condition lists and messages, in both the obarray error table and ErrorRegistry:

Symbol Conditions Message
module-out-of-memory error Module out of memory
module-load-failed error Module load failed
module-open-failed module-load-failed error Module could not be opened
module-not-gpl-compatible module-load-failed error Module is not GPL compatible
missing-module-init-function module-load-failed error Module does not export an initialization function
module-init-failed module-load-failed error Module initialization failed
invalid-arity error Invalid function arity
memory-buffer-too-small error Memory buffer too small

No module-loading behaviour changes; only the error symbols' properties are defined.

Verification

New tests in neovm-core (runtime/errors/tests/mod.rs, Nextest names emacs_core::errors::tests::*):

  • registry_module_load_failed_family checks the ErrorRegistry hierarchy.
  • module_error_symbols_match_gnu_emacs_module_c evaluates in the bootstrap evaluator: module-load of a missing file is caught by a module-load-failed handler as module-open-failed; each symbol's error-conditions / error-message equals GNU's; and error-message-string formats a module-open-failed error.

Both fail on the base commit and pass with this change. Focused Linux run with Nextest:

cargo nextest run -p neovm-core --lib -E 'test(~emacs_core::errors::) | test(~dynamic_module) | test(~module_load) | test(~module_open)'

102 tests run, 102 passed (the existing error-table and dynamic-module tests plus the two new ones). cargo fmt --check -p neovm-core passes. This is a focused selection, not a full-workspace or hosted-CI result.

Assistance

This PR is agent-assisted.

module-load signals module-open-failed, module-not-gpl-compatible,
missing-module-init-function and module-init-failed, but none of these
had error-conditions or error-message, so a failed dlopen surfaced as
(error "Invalid error symbol" module-open-failed).  Register the full
syms_of_module set from GNU emacs-module.c, including the
module-load-failed parent and the invalid-arity,
memory-buffer-too-small and module-out-of-memory conditions.
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a247d0a9-7c88-4f96-951a-856eaa042baf
📥 Commits

Reviewing files that changed from the base of the PR and between 7764ef7 and 034f17a.

📒 Files selected for processing (2)
  • crates/neovm-core/src/emacs_core/runtime/errors/mod.rs
  • crates/neovm-core/src/emacs_core/runtime/errors/tests/mod.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The standard error registry now includes eight dynamic-module error conditions with defined parent relationships. New tests check their inheritance, condition lists, messages, and formatted error output.

Changes

Dynamic-module errors

Layer / File(s) Summary
Register and verify module errors
crates/neovm-core/src/emacs_core/runtime/errors/mod.rs, crates/neovm-core/src/emacs_core/runtime/errors/tests/mod.rs
The standard error declarations and hierarchy add eight dynamic-module error conditions. Tests check condition inheritance and lists, Lisp-visible messages, and formatted module-open-failed output.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: eval-exec

Merge Risk: ⚪ Minimal · up to 034f1

No actionable issue is identified in the module-error definitions or tests. The change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 034f1

The change makes existing module failures catchable under the intended error names without changing module-loading permissions or execution authority. No material new security risk was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The introduced behavior affects failure classification and handler selection in the invoking evaluator. Native modules retain their existing host-process execution authority; the registrations do not create a new module-loading entrypoint or confer additional privileges.

Trust Boundaries and Controls

  • observed — The loader still returns failures when library opening, the GPL marker check, or initialization-symbol lookup fails. Registering those failure symbols changes their catchability, not whether the loader advances past those checks.

Resilience and Maintainability Implications

  • inferred — Initialization failure, pending non-local exit, or interruption returns before successful-module insertion. Native initialization is not transactional with evaluator state, so prior side effects need not be undone. This limitation predates the PR: generic error handlers could already catch the base’s invalid-symbol error and continue after failure.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the change: defining GNU dynamic-module error symbols.
Description check ✅ Passed The description explains the missing error properties, the symbols and condition hierarchies added, and the reported tests. It is directly related to the changeset.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

@coderabbitai
coderabbitai Bot requested a review from eval-exec October 5, 2026 09:45
@eval-exec
eval-exec merged commit 4860aba into eval-exec:main Oct 7, 2026
35 of 42 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.

2 participants