- Notifications
You must be signed in to change notification settings - Fork471
compiler: update typecore; tests: add VariantCoercionConstructUsed; d…#7839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ocs: update AGENTS; docstring_tests: update DocTest outputsSigned-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
pkg-pr-newbot commentedSep 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great stuff! Some cleanup suggested.
compiler/ml/typecore.ml Outdated
| with | ||
| |Some (_p_src, _p_src_conc, src_decl),Some (_p_tgt, p_tgt_conc, tgt_decl) | ||
| -> ( | ||
| match (src_decl.type_kind, tgt_decl.type_kind)with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These matches could be done in one match, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex can this be done in one match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To use Codex here,create an environment for this repo.
compiler/ml/typecore.ml Outdated
| lethas_srcname=List.exists (funn -> n= name) src_namesin | ||
| let tgt_ty_name=Path.last p_tgt_concin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
UseStringSet instead for perf for large amounts of constructors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex Use StringSet instead for perf for large amounts of constructors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To use Codex here,create an environment for this repo.
The example is wrong, it does not trigger the warning. Update the example to be: // Repro for incorrect "constructor ... is never used" warning with coercions// This should compile cleanly without warnings when coercing from a -> b.typea=A |BmoduleDoNotWarn: {letlog:a=>unit}= {typeb= |...a |Cletlog= (x:a)=>Js.log((x :>b))}let_=DoNotWarn.log(A) |
Took it from the chat. Let me close this and start again. |
…s test for C warning- Fix spurious “constructor … is never used” warnings when values are introduced via coercions between concrete variant types.- After a successful coercion, mark the intersection of constructors as positively used for both: - the concrete target declaration, and - the exposed target alias (e.g. private/abstract signature path) when it differs.- Preserve warnings for constructors only present on the target side (e.g. C) — they remain unused and should warn.Tests:- Add super errors fixture ensuring C still warns: - fixtures: tests/build_tests/super_errors/fixtures/VariantCoercionWarnOnC.res - expected: tests/build_tests/super_errors/expected/VariantCoercionWarnOnC.res.expectedBuild:- Compiler builds cleanly; super-errors snapshot matches.
zth commentedSep 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@jfrolich can you verify whether the test added to this branch gives the expected warning (warns about 1 constructor)? |
Yes it should warn about C, but not about the other constructors! |
Great! So@cristianoc it seems the implementation is correct. I think we can move ahead with cleanup. |
Any more cases to test? With interfaces? |
It only triggers for things with interfaces. I guess for completeness sake we could add an example where the new type is just a spread of the other type, with no new constructors added (but still using coercion), to see that all constructors are marked as being used. |
…ry/withMark constructor usage only when both concrete typedecls are found.Preserves behavior and simplifies the control flow vs tuple-of-options match.Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
Build a set of source constructor names and use O(log n) membershipchecks instead of O(n) List.exists for each target constructor.Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
@zth take another look |
compiler/ml/typecore.ml Outdated
| (* If the target type path differs from its concrete decl (e.g., | ||
| when a signature exposes a private or abstract alias), also mark | ||
| usage on the exposed target declaration so scheduled warnings | ||
| attached to that declaration are cleared.*) | ||
| ifnot (Path.same p_tgt p_tgt_conc)then | ||
| let exposed_ty_name=Path.last p_tgtin | ||
| let exposed_decl=Env.find_type p_tgt envin | ||
| List.iter | ||
| (fun (c: Types.constructor_declaration) -> | ||
| let cname=Ident.name c.cd_idin | ||
| if has_src cnamethen | ||
| Env.mark_constructor_usedEnv.Positive env exposed_ty_name | ||
| exposed_decl cname) | ||
| tgt_cons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@cristianoc what does this mean in practice? I don't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex what's the answer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The code around line 3064 handles cases where the type path used in the code (p_tgt) differs from the path of its concrete declaration (p_tgt_conc). This occurs when a module signature exposes a variant type via a private or abstract alias. When such a coercion happens, the code marks the overlapping constructors as “used” on both the concrete declaration and the exposed alias so that “constructor … is never used” warnings are cleared for both locationscompiler/ml/typecore.mlL3051-L3064
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex explain more in detail the terms used in your response and the mechanism to mark a constructor never used.
Don't assume familiarity with the representation of types and type environments, but introduce enough context to understand this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Summary
- Expanded the comments around coercion handling to define
p_tgt(path used in source code) versusp_tgt_conc(resolved declaration path) and to explain howEnv.mark_constructor_usedrecords constructor usage on both the concrete type and any exposed alias so “constructor … is never used” warnings are clearedcompiler/ml/typecore.mlL3051-L3064
Testing
⚠️ make format(fails: dune: command not found)⚠️ make test(fails: KeyboardInterrupt during ninja build)
Notes
apt-get updatefailed with 403 errors, so required dependencies (e.g.,dune) could not be installed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we remove this special case code, does the fix still work? If so I think we should remove it, and then add it again in the future if we discover real issues it would fix.
example.res Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This can be removed, irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Summary
- Removed the redundant
example.resfile as it was no longer relevant to the pull request
Testing
⚠️ make format(dune: command not found; internal Biome error)⚠️ make test(dune: command not found)
issue.txt Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's remember to remove this before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Summary
- Removed the temporary
issue.txtdebugging log from the repository
Testing
⚠️ make format(fails: dune: command not found)⚠️ make test(fails: dune: command not found)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex perhaps AGENTS.md needs to be changed with instruction on how to set up opam and dune?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Summary
- Added an “Installing opam and dune” section to AGENTS.md with step-by-step commands for setting up the OCaml toolchain so
duneis available for build and test targetsAGENTS.mdL111-L131 - Removed the temporary
issue.txtdebugging log from the repository
Testing
⚠️ apt-get update(repository InRelease files not signed, 403 errors)⚠️ make format(dune: command not found; cargo: command not found)⚠️ make test(cargo: command not found)
Notes
- Package repository errors prevented installing
opam, leavingduneandcargounavailable for formatting and tests
Uh oh!
There was an error while loading.Please reload this page.
@cristianoc added a few comments. The changelog text is quite convoluted as well, but I'm not sure I care too much about that. |
@zth would you take over the PR? |
Let's close it if so, and someone can use the insights here as the base for fixing this later on. |
hold on, our assistant is thinking |
@codex try to build the compiler and run tests now, report if it worked |
Summary
Testing
|
@codex try again to build the compiler now that internet access is enabled in the sandbox |
Summary
Testing
Notes
|
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
…rcionMark shared constructors as used on the target variant after successful coercion; keep warnings for target-only constructors. Part of#7839.
CODEX: I reviewed the latest change to mark shared constructors as used after variant→variant coercions. Verified there’s no redundancy in typecore and no leftover duplication; the extra exposed-decl marking was removed and inclusion handles propagation. Tests cover no spurious warning and still warn for target-only constructors. CHANGELOG updated. @zth Anything else to address here? If not, we’re good to merge. Thanks! |
…ocs: update AGENTS; docstring_tests: update DocTest outputs