Move rescript.json reading out of bsc into rewatch#8365
Conversation
The compiler's Ext_path.package_dir lazily walked the filesystem from cwd until it found rescript.json, solely to determine the JS output root in lam_compile_main. That walk is unnecessary when rewatch is driving the compile — rewatch already knows the package root. - compiler: add custom_package_dir ref in Ext_path; convert package_dir to a unit -> string that prefers the ref and falls back to the existing walk. Add -bs-project-root CLI flag on bsc. - rewatch: emit -bs-project-root <package-root> from compiler_args for every bsc invocation. No behavioral change for users; this is the first step toward removing direct rescript.json reads from bsc entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The gentype backend was the last site in bsc that parsed rescript.json directly (via GenTypeConfig.read_config walking up from cwd). Rewatch already parses that file, so it can hand the needed fields to bsc as CLI flags, eliminating the filesystem walk and the duplicated JSON reading. Compiler side: - GenTypeConfig.ml: drop the JSON reader. Replace with module-level refs populated by bsc CLI flags, plus `build_config ~namespace` that assembles a Config.t from those refs. - New bsc flags: -bs-gentype-module, -bs-gentype-module-resolution, -bs-gentype-export-interfaces, -bs-gentype-generated-extension, -bs-gentype-suffix, -bs-gentype-shim, -bs-gentype-debug, -bs-gentype-dep, -bs-gentype-source-dir, -bs-gentype-bsb-project-root. -bs-project-root now also populates GenTypeConfig.project_root. - Config.t.sources becomes a `string list` (pre-expanded directories) instead of raw Ext_json_types, simplifying ModuleResolver. - Debug.set_item takes a bool-implied item name instead of an Ext_json_types.t value. - Paths.ml: drop get_config_file; read_config is now a thin wrapper around build_config. Rewatch side: - Replace `GenTypeConfig = serde_json::Value` with a typed struct; Deserialize supports both the object and (deprecated) array forms of `shims`. - New Config::get_gentype_args assembles the full -bs-gentype-* flag set from the typed gentypeconfig, suffix, dependencies, pre-expanded source dirs, and workspace root. - compile.rs: when gentype is enabled, walk the package's declared source folders (honoring `subdirs: true`) to produce the directory list gentype needs for cross-file shim resolution — `package.dirs` alone only tracks dirs with .res files. Generated .gen.tsx / .bs.js output is byte-identical; all existing gentype and full test suites pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
With rewatch always passing -bs-project-root, the fallback walk in Ext_path.find_root_filename was only reachable from direct bsc invocations. Move that walk to the Node.js wrapper (cli/bsc.js), which auto-injects -bs-project-root when absent, and make Ext_path.package_dir fail loudly if the flag was never set. - Ext_path: drop find_root_filename, find_config_dir, fallback_package_dir. package_dir () now errors out with a clear message if custom_package_dir is None. - Literals.rescript_json: removed (last reference was the deleted walk). - cli/bsc.js: walk up from cwd for rescript.json, append -bs-project-root <dir> to the args when it isn't already there. The compiler now has zero code paths that read or locate rescript.json. Locating the project root is a concern entirely for the build system (rewatch) or the JS wrapper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three sites still used compiler/ext/ext_json* after the rescript.json removal: gentype's .sourcedirs.json reader, reanalyze's cmt-scan reader, and the parser's own ounit test suite. Each is handled separately so the custom lexer-based parser can be deleted outright. Gentype: - Add -bs-gentype-dep-path <name>=<path> flag. rewatch emits one per dependency, resolved through the packages map it already holds. - GenTypeConfig.Config.t gains a dep_paths Hashtbl populated from the flags. ModuleResolver.sourcedirs_to_map now reads from config.sources + config.dep_paths instead of opening <bsb_project_root>/lib/bs/ .sourcedirs.json. The missing-.sourcedirs.json warning and the read_dirs_from_config fallback are gone — rewatch supplies both pieces directly. Reanalyze: - Delete dead readSourceDirs and readDirsFromConfig (no callers). - Port readCmtScan from Ext_json_parse to jsonlib (Json.parse), which reanalyze already depends on for its rescript.json reading. - Drop `ext` from reanalyze's dune libraries. Parser: - Delete ext_json.ml/mli, ext_json_parse.mll/mli, ext_json_noloc.ml/ mli, ext_json_types.ml. - Delete the ocamllex rule in compiler/ext/dune. - Delete ounit_ext_json_tests.ml and its registration in ounit_tests_main.ml. Net: -1006 lines. No code anywhere in the repo parses JSON with the custom lexer now; jsonlib is the only JSON parser left. Generated .gen.tsx / .bs.js output is byte-identical; make test, test-gentype, and test-rewatch all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa5434765
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three small modules in compiler/ext had no callers anywhere in the repo (neither in production code, nor in tests): - ext_color: unused color/ANSI helpers - ext_position: unused Lexing.position utilities - ext_string_array: unused string-array helpers Removed alongside the Ext_json cleanup. Build and tests unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When gentypeconfig.module is omitted, the old JSON-reading path fell
back to package-specs.module (object form) before defaulting to
ESModule. My flag-based rewrite dropped that fallback, so a project
with "package-specs": { "module": "commonjs" } but no explicit
gentypeconfig.module would suddenly emit ESModule-style imports —
breaking downstream TypeScript consumption.
Replay the old precedence in rewatch when assembling
-bs-gentype-module:
1. gentypeconfig.module if set
2. else package-specs.module when package-specs is a single object
3. else leave the flag off (bsc applies its ESModule default, same as
before)
Array-form package-specs still doesn't feed the fallback — that
matches the old code's Obj-only pattern exactly. Added two tests
covering both paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d423388997
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two small alignments found during a detailed behavioral review. Each
only matters in a pathological collision case (two dirs / deps with
the same module name), but worth getting right since this PR promises
byte-identical behavior.
1. bs_dependencies / sources: old code accumulated these with a
prepend into a ref and returned the ref unreversed, so iteration
proceeded in reverse-input order. My new code did List.rev and
flipped the order, which would swap the last-write-wins winner in
ModuleResolver's file_map on collision. Drop the List.rev.
2. dep_paths: I was sorting alphabetically in rewatch. The old code
iterated the .sourcedirs.json "pkgs" array in order. Remove the
sort and pass the caller's order through unchanged.
Not preserved (agreed with reviewer that this was a bug): the old
code's namespace field was populated from the cmt filename only when
rescript.json had {"namespace": true} literally. For string-valued
namespaces like {"namespace": "Custom"}, the old pattern match fell
through and returned None — gentype then emitted imports like
./Module pointing at files actually named Module-Custom.bs.js. The
new code (always use the cmt-derived namespace) is correct.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for GenTypeModule { |
There was a problem hiding this comment.
Why not derive Deserialize here?
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for GenTypeModuleResolution { |
collect_gentype_source_dirs was being called from compiler_args — i.e. once per source file per build. The result is per-package and doesn't change during a build, so the redundant walks were pure waste on gentype-enabled projects with many files. Move the walk into package discovery (packages::make), cache it on a new Package.gentype_dirs field, and only compute it for packages that have a gentype_config. compiler_args now just borrows the cached slice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The manual Deserialize impls were reinventing what serde can do with
#[serde(rename = "...")] on each variant — the same pattern already in
use for PackageModule in this file. Drop the hand-written impls in
favor of derive; serde's default error message for unknown variants
("unknown variant `foo`, expected one of ...") is good enough.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
\"Custom\" was a holdover from when there were two sources for the package dir (CLI flag vs filesystem walk fallback) — \"custom\" meant \"CLI-set\" in contrast to the walked-up default. Now that the walk is gone, there's only one source and \"custom\" adds no information. Rename the ref to [project_root] so it matches the CLI flag name [-bs-project-root] directly. The getter stays [package_dir ()] since that's the established term used by [lam_compile_main] and [js_packages_info]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The comment was positioned above the record literal but only applied to two fields inside it, making it read as if it documented the whole construction. The invariant it described (kept via commit history) is better tracked in git blame than inlined at the call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Eliminates the two sites in the compiler (
bsc) that still readrescript.jsondirectly, replacing them with CLI flags the build system (rewatch) passes in. Rewatch already parses the config, so the roundtrip was unnecessary.Two commits, shipped together:
-bs-project-rootflag — replacesExt_path.package_dir's filesystem walk for locating the package root (used to determine the JS output directory). Rewatch always passes the package root now.-bs-gentype-*flags — the gentype backend previously walked up fromcwdto findrescript.jsonand re-parsed it. All consumed fields (module,moduleResolution,shims,debug,exportInterfaces,generatedFileExtension,suffix,dependencies,sources) are now passed as CLI flags. Rewatch gains a typedGenTypeConfigstruct (replacing theserde_json::Valuealias) and assembles the full flag set, including pre-expanded source directories that gentype needs for cross-file shim resolution.The
Ext_pathfallback walk is kept for bsc-standalone / script-mode use, so nothing regresses for directbsc foo.resinvocations.Notable behavior
GenTypeConfig.Config.t.sourceschanges fromExt_json_types.t optiontostring list(pre-expanded dirs from rewatch).ModuleResolver.read_dirs_from_configcollapses from a recursive JSON walker to a one-liner filter.Debug.set_itemnow takes just the item name (bool is implied true);-bs-gentype-debugis repeatable.[\"From=To\", ...]array form.Still-parsed JSON
ModuleResolver.read_source_dirsstill opens.sourcedirs.jsonat<bsb_project_root>/lib/bs/.sourcedirs.json. That's a rewatch-produced file, notrescript.json, and is shared with reanalyze — a separate concern, left for a future PR.Test plan
dune build— cleancargo build --manifest-path rewatch/Cargo.toml— cleancargo test --manifest-path rewatch/Cargo.toml— 68 pass (added tests for typedGenTypeConfig,-bs-gentype-*arg emission, and both shim formats)make test— 922 + 743 pass, 0 failuresmake test-gentype— all suites greenmake test-rewatch— all integration tests pass.gen.tsx/.bs.jsoutput byte-identical to main (no diff intests/gentype_testsafter rebuild)rewatch compiler-argsconfirms-bs-project-rootand the full-bs-gentype-*flag set are emitted with correct values🤖 Generated with Claude Code