super_errors_multi: multi-file fixture harness for cross-module errors#8433
Open
JonoPrest wants to merge 16 commits into
Open
super_errors_multi: multi-file fixture harness for cross-module errors#8433JonoPrest wants to merge 16 commits into
JonoPrest wants to merge 16 commits into
Conversation
…/.res signature mismatch)
…sambiguation errors
…el-constructor disambiguation
… polyvariant constraint, alias dot access
…async/promise, optional label omitted
…rity, dict pattern, .resi-only ref
…-module GADT disambiguation
…ptional, Unboxed_representation, Tag_name mismatches
…ssing `find_in_path_uncap` resolves `.cmi` lookups case-insensitively, so the filename printed in error messages picks up whichever case the host filesystem returns: lowercase on macOS APFS/HFS+, capitalised on Linux ext4. Snapshots captured locally on macOS therefore mismatch on Linux CI (`foo.cmi` vs `Foo.cmi`). Normalise every `.cmi` basename to lowercase in postProcessErrorOutput so snapshots are platform-independent. The same function runs on the expected file too, so existing snapshots keep matching.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8433 +/- ##
==========================================
+ Coverage 59.92% 60.11% +0.19%
==========================================
Files 373 373
Lines 54210 54210
==========================================
+ Hits 32487 32591 +104
+ Misses 21723 21619 -104 🚀 New features to boost your workflow:
|
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #8432 (jono/expand-coverage-2). Adds a new test directory
tests/build_tests/super_errors_multi/that mirrorssuper_errors/but forerrors/warnings that only surface across compilation-unit boundaries.
Why a new harness
The existing
super_errors/runner only feeds one.resfile tobscperfixture. That covers everything reachable from a single source file but
leaves a class of compiler errors untested:
.resiand.res(Includemod throughcompunit).Interface_not_compiled(missing.cmiwhen-bs-read-cmiis set).This PR adds a parallel directory with its own runner that compiles each
fixture's files via
bscdirectly, ordering.resiahead of.respermodule and tagging output by source file.
Harness design
fixtures/.Files inside are compiled alphabetically (with
.resiahead of thematching
.res). Fixtures needing a specific cross-module order canprefix filenames with a numeric label (e.g.
01_Foo.res).-w +A -bs-jsx 4 -color always -bs-cmi-only, plus-bs-read-cmiper file when a corresponding.resiexists.-bs-cmi-onlystops the pipeline after typechecking, avoiding the "Js_not_found" failure
bsc otherwise raises when trying to resolve sibling
.jsfor cross-moduleimports.
expected/<FixtureName>.expectedper fixture with allfiles' stderr concatenated, tagged
===== Foo.res =====.single-file runner (super_errors: run fixtures in parallel #8430).
.cmi/.cmj/.cmt/.cmti/.mjs/.jsare cleaned perfixture before each run and globally gitignored.
Format / format-check scripts are extended to skip
fixtures/Iface_not_compiled/since that fixture intentionally contains a.resiwith a parse error.What's covered (multi-file fixtures by category)
Includemod via
.resi/.resmismatch (throughcompunit):Interface_not_compiled)Privacy)Field_mutable)Field_optional)Unboxed_representation)Tag_name)Variant_representation)Cross-module type / record / constructor disambiguation:
constructor)
module B = A.Inner,Ctypes againstB.Reexport.t)module S = Lib.Settings; S.host)type b = ...A.t | Coverlap).resi).resialone)Async / FFI / patterns / GADTs:
Unqualified_gadt_pattern— was reachable only here,not from a single file)
let module(M) = …pattern)in toplevel
let)Warnings across modules:
@deprecated @valexternal)
@deprecatedtype used cross-module)Limitations
bscrather thanrescript build, so fixtures can'trely on
rescript.json, namespacing, or third-party packages. JSX fixturesthat import
Reactwere ruled out for this reason.Cannot_scrape_alias(typetexp / typemod) requires a broken alias chainin
.cmidata that a hand-written fixture can't produce.Scoping_pack,With_makes_applicative_functor_ill_typed,With_changes_module_alias,Cannot_eliminate_dependencyall needapplicative-functor or destructive-substitution constructions that didn't
fire from any fixture I tried. Likely dead in current ReScript syntax;
flagged for a separate audit.
Includemod.Unbound_module_path/Unbound_modtype_path/Invalid_module_aliasrequire signature comparisons whose triggers couldn'tbe reproduced from the fixture format. Also flagged.
Coverage impact
Measured locally with
make coverage(bisect_ppx point coverage). Baselineis
upstream/master(commit at the time of this PR).Per-file moves on the modules this branch targets:
compiler/ml/includecore.mlcompiler/ml/includemod.mlcompiler/ml/typemod.mlcompiler/ml/typedecl.mlcompiler/ml/translmod.mlcompiler/ml/typecore.mlcompiler/ml/typetexp.mlcompiler/ml/env.mlcompiler/ml/translcore.mlThe two ~+5 pp jumps on
includecore.mlandincludemod.mlare theheadline — those are precisely the modules that the
.resi/.resmismatchfixtures (Iface_*) exercise via the
Includemod.compunitentry point,which has no equivalent in single-file work. Privacy, mutable-field,
optional-field, unboxed-rep, tag-name, and variant-representation paths in
includecoreare now hit. The +37 absolute points ontypecore.mlcomefrom cross-module GADT disambiguation (
Unqualified_gadt_pattern),Modules_not_allowed,Unexpected_existential,Cannot_infer_signature,and
Undefined_method.Modules where the single-file suite already topped out the reachable paths
(
rec_check.ml,transl_recmodule.ml,bs_syntaxerr.ml,ast_attributes.ml,ast_external_process.ml,cmi_format.ml) show 0 movement — multi-filedoesn't unlock anything new in those.