Skip to content

Refactor 5 FFI test sites to NonNull / as_ref().expect() for clearer non-null contracts#318

Merged
ciaranra merged 2 commits into
devfrom
codeql-ffi-fp-refactor
May 23, 2026
Merged

Refactor 5 FFI test sites to NonNull / as_ref().expect() for clearer non-null contracts#318
ciaranra merged 2 commits into
devfrom
codeql-ffi-fp-refactor

Conversation

@ciaranra

Copy link
Copy Markdown
Member

Summary

Cleanup follow-up to #317. Refactors 5 test-code FFI sites where assert!(!ptr.is_null()) + raw &*ptr derefs were flagged by CodeQL's rust/access-invalid-pointer. Both reviewers (Codex + Pickle) recommend the refactors on code-quality grounds, with the caveat that CodeQL's null-barrier model is if ptr.is_null() {...}-shaped and may not auto-close the alerts even after this PR.

The 2 discovery.rs vtable derefs are intentionally not touched -- both reviewers explicitly rejected any rewrite as a code smell (the if !X.is_null() && !Y.is_null() { *X } form is canonical Rust raw-pointer null-guard). Those 2 + any remaining test alerts are for UI dismissal as accepted false positives.

What changed

  • crates/pecos-qis-ffi/src/ffi.rs::test_heap_alloc_and_free: assert!(!ptr.is_null()) + raw *mut u8 -> NonNull::new(...).expect(...). The NonNull type encodes the non-null invariant for a malloc-backed allocation where as_ref/as_mut would be the wrong abstraction (uninitialised memory). Codex's specific recommendation.
  • crates/pecos-qis-ffi/src/lib.rs (4 sites: test_pending_operations_storage, test_wait_for_result_ready_with_dynamic_mode, two derefs in test_wait_for_result_ready_exports_only_new_operations): assert!(!ptr.is_null()) + unsafe { &*ptr } -> unsafe { ptr.as_ref() }.expect(...). Non-null encoded in Option<&T>; safe expect() panics on null with a descriptive message. Pickle + Codex aligned.

Net diff: -5 lines (refactor is genuinely more concise). No behaviour change in the tests' assertion semantics.

Why this isn't "code-to-satisfy-the-linter"

Per CLAUDE.md, refactoring solely to placate a static analyzer is in the same family as suppressing warnings to silence them. The reason these refactors pass that test:

  1. They are real idiomatic Rust improvements. NonNull<T> and Option<&T> are the standard types for "definitely non-null" and "maybe non-null with safe ownership" respectively. Using them instead of raw *mut T + assert! + &*ptr is what the Rust FFI ecosystem does.
  2. Both reviewers concur this is worth doing even if CodeQL didn't exist.
  3. The 2 discovery.rs sites are explicitly NOT refactored -- because there, the cleaner pattern would actually be a code smell (combinator chains on Option vs the canonical if !is_null() guard). Cleanup ends where it would start subtracting value.

Side observation (not in scope here)

Codex flagged: should plugin discovery treat a plugin that returns exactly one of handle/vtable (but not both) as a protocol violation (log/reject) rather than silently treating it as "not provided"? A half-populated plugin could leak a foreign handle if the destroy-vtable is absent. Not blocking this PR -- backlog for the plugin-protocol design.

Test plan

  • just lint clean
  • cargo test -p pecos-qis-ffi --lib (89 pass, 0 fail)
  • CI: confirm all checks pass
  • Post-merge: check whether CodeQL auto-closes any of the 5 refactored alerts. Likely outcome: most/all 5 still alert (CodeQL's null-barrier model doesn't follow as_ref()/NonNull through stdlib internals). Whatever survives gets UI-dismissed alongside the 2 discovery.rs sites.

Reviews

  • ~/Repos/pecos-docs/reviews/codex-codeql-ffi-fp-resolution.md
  • ~/Repos/pecos-docs/reviews/pickle-codeql-ffi-fp-resolution.md

@ciaranra
ciaranra merged commit ac2d417 into dev May 23, 2026
92 of 94 checks passed
@ciaranra
ciaranra deleted the codeql-ffi-fp-refactor branch May 23, 2026 18:50
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