feat(openapi-gen-build): Dynamic module imports #3

The PR replaces the naive str::replace-based crate:: rewriting in namespace.rs with a state-machine scanner that skips comments and string/char literals, and makes write_contract_mod in generate.rs auto-discover emitted modules instead of hardcoding apis/models. The README is updated accordingly.

Overall the change is a clear improvement: module discovery is deterministic (sorted), and the scanner is a single O(n) pass that correctly handles line comments, nested block comments, and escaped quotes, with good unit tests. The Config API used in the README example exists and matches.

Two correctness bugs in the scanner should be fixed before merging:

Lifetimes are treated as char literals. ' always enters ScanState::Character, but a lifetime such as 'a, 'static, or impl<'a> has no closing quote. The scanner then stays in Character state until the next unrelated ', so every crate:: path in that span is silently left unrewritten (e.g. fn f<'a>(x: &'a crate::models::Foo) keeps crate::models). This produces broken generated code at the consumer side without any error.
Raw strings are not recognized. r"...", r#"..."#, br"...", etc. are handled as ordinary strings. A raw string containing a double quote — which is legal — prematurely closes the string state and causes the crate:: text inside the literal to be rewritten, corrupting the literal.

Minor robustness concern: write_contract_mod will declare any .rs file found (including dotfiles such as .models.rs, producing invalid pub mod .models;) and will emit a duplicate pub mod foo; if both foo.rs and foo/mod.rs exist.

No security issues found (module names come from directory entries and cannot contain path separators). Performance is acceptable for a build-time script.

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes

# Review Summary The PR replaces the naive `str::replace`-based `crate::` rewriting in `namespace.rs` with a state-machine scanner that skips comments and string/char literals, and makes `write_contract_mod` in `generate.rs` auto-discover emitted modules instead of hardcoding `apis`/`models`. The README is updated accordingly. Overall the change is a clear improvement: module discovery is deterministic (sorted), and the scanner is a single O(n) pass that correctly handles line comments, nested block comments, and escaped quotes, with good unit tests. The `Config` API used in the README example exists and matches. Two correctness bugs in the scanner should be fixed before merging: 1. **Lifetimes are treated as char literals.** `'` always enters `ScanState::Character`, but a lifetime such as `'a`, `'static`, or `impl<'a>` has no closing quote. The scanner then stays in `Character` state until the next unrelated `'`, so every `crate::` path in that span is silently left unrewritten (e.g. `fn f<'a>(x: &'a crate::models::Foo)` keeps `crate::models`). This produces broken generated code at the consumer side without any error. 2. **Raw strings are not recognized.** `r"..."`, `r#"..."#`, `br"..."`, etc. are handled as ordinary strings. A raw string containing a double quote — which is legal — prematurely closes the string state and causes the `crate::` text *inside* the literal to be rewritten, corrupting the literal. Minor robustness concern: `write_contract_mod` will declare any `.rs` file found (including dotfiles such as `.models.rs`, producing invalid `pub mod .models;`) and will emit a duplicate `pub mod foo;` if both `foo.rs` and `foo/mod.rs` exist. No security issues found (module names come from directory entries and cannot contain path separators). Performance is acceptable for a build-time script. --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **changes**

clank-bot reviewed

2026-08-01 17:18:58 +00:00

clank-bot left a comment

Code review findings from opencode.

crates/openapi-gen-build/src/generate.rs

					
				@ -83,0 +89,4 @@

				        };

				        if path.is_file() {

				            if path.extension().and_then(|ext| ext.to_str()) == Some("rs") && name != "mod.rs" {

[bug] Module discovery declares every .rs file except mod.rs, without filtering dotfiles or editor artifacts. A .models.rs or models.rs~-style stray file yields invalid declarations such as pub mod .models;, and if both foo.rs and foo/mod.rs exist the generated mod.rs contains pub mod foo; twice, which fails to compile. Consider skipping names that are not valid Rust identifiers and de-duplicating the module list.

[bug] Module discovery declares every `.rs` file except `mod.rs`, without filtering dotfiles or editor artifacts. A `.models.rs` or `models.rs~`-style stray file yields invalid declarations such as `pub mod .models;`, and if both `foo.rs` and `foo/mod.rs` exist the generated `mod.rs` contains `pub mod foo;` twice, which fails to compile. Consider skipping names that are not valid Rust identifiers and de-duplicating the module list.

crates/openapi-gen-build/src/namespace.rs

					
				@ -31,0 +75,4 @@

				    }

				    let byte = bytes[index];

				    if byte == b'"' {

[bug] Raw string literals (r"...", r#"..."#, br"...", c"...") are not distinguished from plain strings. Because a raw string may legally contain ", the string state closes early at that quote and any crate:: text inside the literal is treated as code and rewritten, corrupting the literal (e.g. r#"let s = "crate::apis";"#). Generated Rust output can contain such literals (doc examples, etc.); the scanner should recognize the r/br/cr prefix and the matching #-delimited terminator.

[bug] Raw string literals (`r"..."`, `r#"..."#`, `br"..."`, `c"..."`) are not distinguished from plain strings. Because a raw string may legally contain `"`, the string state closes early at that quote and any `crate::` text inside the literal is treated as code and rewritten, corrupting the literal (e.g. `r#"let s = "crate::apis";"#`). Generated Rust output can contain such literals (doc examples, etc.); the scanner should recognize the `r`/`br`/`cr` prefix and the matching `#`-delimited terminator.

crates/openapi-gen-build/src/namespace.rs Outdated

					
				@ -31,0 +79,4 @@

				        output.push(byte);

				        return (ScanState::String(false), index + 1);

				    }

				    if byte == b'\'' {

[bug] A ' is always treated as the start of a char literal, but in Rust ' also introduces a lifetime ('a, 'static, impl<'a>, &'a ...) which has no closing quote. The scanner then remains in ScanState::Character until an unrelated ' later in the file, so every crate:: path inside that span is silently NOT rewritten (e.g. fn f<'a>(x: &'a crate::models::Foo) keeps crate::models, breaking the consumer build). Consider detecting ' followed by an identifier char (and not \) as a lifetime instead of a char literal, and add a test with a lifetime adjacent to a crate:: path.

[bug] A `'` is always treated as the start of a char literal, but in Rust `'` also introduces a lifetime (`'a`, `'static`, `impl<'a>`, `&'a ...`) which has no closing quote. The scanner then remains in `ScanState::Character` until an unrelated `'` later in the file, so every `crate::` path inside that span is silently NOT rewritten (e.g. `fn f<'a>(x: &'a crate::models::Foo)` keeps `crate::models`, breaking the consumer build). Consider detecting `'` followed by an identifier char (and not `\`) as a lifetime instead of a char literal, and add a test with a lifetime adjacent to a `crate::` path.

crates/openapi-gen-build/src/namespace.rs

					
				@ -31,0 +161,4 @@

				        && (index + 5 == bytes.len() || !is_ident(bytes[index + 5]))

				}

				fn skip_path_whitespace(bytes: &[u8], mut index: usize) -> usize {

[style] skip_path_whitespace only skips spaces and tabs, but Rust allows newline whitespace around path separators (crate\n::foo is valid). Such a path would not be rewritten. This is unlikely in generated output, but skipping all ASCII whitespace (including \r, \n) would make the scanner fully consistent.

[style] `skip_path_whitespace` only skips spaces and tabs, but Rust allows newline whitespace around path separators (`crate\n::foo` is valid). Such a path would not be rewritten. This is unlikely in generated output, but skipping all ASCII whitespace (including `\r`, `\n`) would make the scanner fully consistent.

martin added 1 commit

2026-08-01 17:24:54 +00:00

fix: Issues with ' and " in generated output

ci/woodpecker/push/verify Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

7329eb8d9a

clank-bot commented

2026-08-01 17:29:40 +00:00

Let me verify the raw-string closing semantics against rustc and run the tests.
The bash tool can't spawn processes in this environment, so I'll verify the remaining questions statically. Let me check how the namespace is wired to the module tree.
{"summary": "# Review Summary\n\nA focused improvement to openapi-gen-build that (1) makes write_contract_mod discover generated modules dynamically instead of hardcoding apis/models, and (2) replaces the naive substring-based crate:: rewriting with a small hand-rolled scanner that respects comments, string/char/raw-string literals, and lifetimes. The PR is well tested and the README updates match the implementation.\n\nCorrectness: The scanner is careful — line/block/nested comments, escaped quotes, raw strings with varying hash counts, character literals and lifetimes are all handled, and grouped imports like use crate::{apis, models}; are rewritten correctly. I verified the namespace wiring: Config::generate computes ns = format!(\"{}::{}\", namespace, spec.name), so rewritten paths (crate::generated::contract::apis) line up with the real module tree (generated/mod.rs → pub mod contract; → discovered modules). Backward compatibility holds: for the default rust generator, discovery yields apis/models, matching the previous hardcoded output. The raw-string termination point (\" followed by ≥ N hashes) matches rustc's lexer rule, and re-scanning surplus hashes as code is byte-for-byte benign for valid input. I could not run the test suite in this environment, but the added tests cover the important edge cases.\n\nMinor observations (non-blocking):\n- character_literal_end does not recognize \\xNN hex-escaped char literals, so such literals are scanned as code. This cannot produce wrong output today (a single-char literal can never contain crate::), but it is an inconsistency in the lexer worth handling or at least covering with a test.\n- write_contract_mod could emit duplicate pub mod lines if a generator ever emitted both foo.rs and foo/mod.rs; a cheap dedup would make it robust.\n\nPerformance: Linear scan, per-byte prefix checks fail fast; no concerns. Security: None — all writes target the generator output directory. Maintainability: The scanner is well documented and the state machine is clear; the README accurately describes the new behavior.","verdict": "approve","comments": [{"file": "crates/openapi-gen-build/src/namespace.rs","line": 157,"severity": "style","message": "character_literal_end handles \\u{...} and single-char escapes but not \\xNN hex escapes: for a literal like '\\x63', the else branch consumes only the x (via UTF-8 char length) and then fails to find the closing quote, returning None. The ' is then treated as code. This is currently harmless — a single-char literal can never contain the text crate::, so no incorrect rewrite can result — but it is an inconsistency in the hand-rolled lexer. Consider handling \\xNN (and ideally \\u{...} already done) explicitly, or add a regression test asserting '\\xNN' is preserved, so future maintainers don't assume all char-literal syntax is covered."},{"file": "crates/openapi-gen-build/src/generate.rs","line": 91,"severity": "style","message": "write_contract_mod pushes both a foo.rs file and a foo/ directory (with mod.rs) as candidates, then emits pub mod foo; for each. If a generator output contained both a foo.rs and foo/mod.rs, the sorted modules list would contain foo twice, producing duplicate module declarations and a compile error. The previous hardcoded apis/models output could not hit this. A cheap fix is to dedup after sorting (or collect into a BTreeSet). Real openapi-generator outputs don't collide today, so this is low priority."}]}

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: comment

Let me verify the raw-string closing semantics against rustc and run the tests. The bash tool can't spawn processes in this environment, so I'll verify the remaining questions statically. Let me check how the namespace is wired to the module tree. {"summary": "# Review Summary\n\nA focused improvement to `openapi-gen-build` that (1) makes `write_contract_mod` discover generated modules dynamically instead of hardcoding `apis`/`models`, and (2) replaces the naive substring-based `crate::` rewriting with a small hand-rolled scanner that respects comments, string/char/raw-string literals, and lifetimes. The PR is well tested and the README updates match the implementation.\n\n**Correctness:** The scanner is careful — line/block/nested comments, escaped quotes, raw strings with varying hash counts, character literals and lifetimes are all handled, and grouped imports like `use crate::{apis, models};` are rewritten correctly. I verified the namespace wiring: `Config::generate` computes `ns = format!(\"{}::{}\", namespace, spec.name)`, so rewritten paths (`crate::generated::contract::apis`) line up with the real module tree (`generated/mod.rs` → `pub mod contract;` → discovered modules). Backward compatibility holds: for the default `rust` generator, discovery yields `apis`/`models`, matching the previous hardcoded output. The raw-string termination point (`\"` followed by ≥ N hashes) matches rustc's lexer rule, and re-scanning surplus hashes as code is byte-for-byte benign for valid input. I could not run the test suite in this environment, but the added tests cover the important edge cases.\n\n**Minor observations (non-blocking):**\n- `character_literal_end` does not recognize `\\xNN` hex-escaped char literals, so such literals are scanned as code. This cannot produce wrong output today (a single-char literal can never contain `crate::`), but it is an inconsistency in the lexer worth handling or at least covering with a test.\n- `write_contract_mod` could emit duplicate `pub mod` lines if a generator ever emitted both `foo.rs` and `foo/mod.rs`; a cheap dedup would make it robust.\n\n**Performance:** Linear scan, per-byte prefix checks fail fast; no concerns. **Security:** None — all writes target the generator output directory. **Maintainability:** The scanner is well documented and the state machine is clear; the README accurately describes the new behavior.","verdict": "approve","comments": [{"file": "crates/openapi-gen-build/src/namespace.rs","line": 157,"severity": "style","message": "`character_literal_end` handles `\\u{...}` and single-char escapes but not `\\xNN` hex escapes: for a literal like `'\\x63'`, the `else` branch consumes only the `x` (via UTF-8 char length) and then fails to find the closing quote, returning `None`. The `'` is then treated as code. This is currently harmless — a single-char literal can never contain the text `crate::`, so no incorrect rewrite can result — but it is an inconsistency in the hand-rolled lexer. Consider handling `\\xNN` (and ideally `\\u{...}` already done) explicitly, or add a regression test asserting `'\\xNN'` is preserved, so future maintainers don't assume all char-literal syntax is covered."},{"file": "crates/openapi-gen-build/src/generate.rs","line": 91,"severity": "style","message": "`write_contract_mod` pushes both a `foo.rs` file and a `foo/` directory (with `mod.rs`) as candidates, then emits `pub mod foo;` for each. If a generator output contained both a `foo.rs` and `foo/mod.rs`, the sorted `modules` list would contain `foo` twice, producing duplicate module declarations and a compile error. The previous hardcoded `apis`/`models` output could not hit this. A cheap fix is to dedup after sorting (or collect into a `BTreeSet`). Real openapi-generator outputs don't collide today, so this is low priority."}]} --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **comment**

martin merged commit 618ed261b1 into main

2026-08-01 17:31:18 +00:00

martin deleted branch dynamic-modules

2026-08-01 17:31:18 +00:00

martin referenced this pull request from a commit

2026-08-01 17:31:20 +00:00

feat(openapi-gen-build): Dynamic module imports (#3)

Sign in to join this conversation.

No reviewers