compiler: update typecore; tests: add VariantCoercionConstructUsed; d… #7839

cristianoc · 2025-09-04T10:21:28Z

…ocs: update AGENTS; docstring_tests: update DocTest outputs

…ocs: update AGENTS; docstring_tests: update DocTest outputs Signed-off-by: Cristiano Calcagno <[email protected]>

pkg-pr-new · 2025-09-04T10:30:08Z

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7839

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7839

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7839

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7839

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7839

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7839

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7839

commit: 6012c86

zth

Great stuff! Some cleanup suggested.

zth · 2025-09-04T10:59:39Z

compiler/ml/typecore.ml

+     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


These matches could be done in one match, right?

@codex can this be done in one match?

To use Codex here, create an environment for this repo.

zth · 2025-09-04T11:00:18Z

compiler/ml/typecore.ml

+        let has_src name = List.exists (fun n -> n = name) src_names in
+        let tgt_ty_name = Path.last p_tgt_conc in


Use StringSet instead for perf for large amounts of constructors?

@codex Use StringSet instead for perf for large amounts of constructors?

To use Codex here, create an environment for this repo.

zth · 2025-09-04T11:12:22Z

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.

type a = A | B
module DoNotWarn: {
  let log: a => unit
} = {
  type b =
    | ...a
    | C

  let log = (x: a) => Js.log((x :> b))
}

let _ = DoNotWarn.log(A)

cristianoc · 2025-09-04T11:28:41Z

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.

type a = A | B
module DoNotWarn: {
  let log: a => unit
} = {
  type b =
    | ...a
    | C

  let log = (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.expected Build: - Compiler builds cleanly; super-errors snapshot matches.

zth · 2025-09-05T05:05:39Z

@jfrolich can you verify whether the test added to this branch gives the expected warning (warns about 1 constructor)?

jfrolich · 2025-09-05T06:26:43Z

Yes it should warn about C, but not about the other constructors!

zth · 2025-09-05T06:41:42Z

Great! So @cristianoc it seems the implementation is correct. I think we can move ahead with cleanup.

cristianoc · 2025-09-05T06:45:51Z

Great! So @cristianoc it seems the implementation is correct. I think we can move ahead with cleanup.

Any more cases to test? With interfaces?
If not, then cleanup.

zth · 2025-09-05T06:47:38Z

Great! So @cristianoc it seems the implementation is correct. I think we can move ahead with cleanup.

Any more cases to test? With interfaces? If not, then cleanup.

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/with Mark 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 <[email protected]>

Build a set of source constructor names and use O(log n) membership checks instead of O(n) List.exists for each target constructor. Signed-off-by: Cristiano Calcagno <[email protected]>

cristianoc · 2025-09-05T10:03:43Z

@zth take another look

zth · 2025-09-05T10:27:10Z

compiler/ml/typecore.ml

+         (* 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. *)
+         if not (Path.same p_tgt p_tgt_conc) then
+           let exposed_ty_name = Path.last p_tgt in
+           let exposed_decl = Env.find_type p_tgt env in
+           List.iter
+             (fun (c : Types.constructor_declaration) ->
+               let cname = Ident.name c.cd_id in
+               if has_src cname then
+                 Env.mark_constructor_used Env.Positive env exposed_ty_name
+                   exposed_decl cname)
+             tgt_cons


@cristianoc what does this mean in practice? I don't get it.

@codex what's the answer here?

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 locations compiler/ml/typecore.mlL3051-L3064

View task →

@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.

Summary

Expanded the comments around coercion handling to define p_tgt (path used in source code) versus p_tgt_conc (resolved declaration path) and to explain how Env.mark_constructor_used records constructor usage on both the concrete type and any exposed alias so “constructor … is never used” warnings are cleared compiler/ml/typecore.mlL3051-L3064

Testing

⚠️ make format (fails: dune: command not found)

⚠️ make test (fails: KeyboardInterrupt during ninja build)

Notes

apt-get update failed with 403 errors, so required dependencies (e.g., dune) could not be installed

View task →

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.

zth · 2025-09-05T10:27:28Z

example.res

This can be removed, irrelevant.

@codex remove this

Summary

Removed the redundant example.res file 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)

View task →

zth · 2025-09-05T10:27:40Z

issue.txt

Let's remember to remove this before merging.

@codex remove

Summary

Removed the temporary issue.txt debugging log from the repository

Testing

⚠️ make format (fails: dune: command not found)

⚠️ make test (fails: dune: command not found)

View task →

@codex perhaps AGENTS.md needs to be changed with instruction on how to set up opam and dune?

Summary

Added an “Installing opam and dune” section to AGENTS.md with step-by-step commands for setting up the OCaml toolchain so dune is available for build and test targets AGENTS.mdL111-L131

Removed the temporary issue.txt debugging 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, leaving dune and cargo unavailable for formatting and tests

View task →

tests/docstring_tests/DocTest.res

zth · 2025-09-05T10:29:01Z

@cristianoc added a few comments. The changelog text is quite convoluted as well, but I'm not sure I care too much about that.

cristianoc · 2025-09-05T10:40:54Z

@zth would you take over the PR?
Getting diminishing returns going back and forth for little things
later, we can "create an environment for this repo."

zth · 2025-09-05T11:11:03Z

Let's close it if so, and someone can use the insights here as the base for fixing this later on.

cristianoc · 2025-09-05T14:19:11Z

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

cristianoc · 2025-09-05T14:40:05Z

@codex try to build the compiler and run tests now, report if it worked

chatgpt-codex-connector · 2025-09-05T14:43:42Z

Summary

Build failed: make terminated while compiling the Rust-based rewatch component, preventing the subsequent dune build step from running
Build environment missing dune; direct invocation shows bash: command not found: dune
Test suite did not run: make test similarly terminated during the rewatch build phase

Testing

❌ make (terminated during rewatch build; could not reach compiler build step)
❌ dune build (dune not installed)
❌ make test (terminated during rewatch build; tests not executed)

View task →

cristianoc · 2025-09-05T15:09:10Z

@codex try again to build the compiler now that internet access is enabled in the sandbox

chatgpt-codex-connector · 2025-09-05T15:16:33Z

Summary

Bootstrapped and installed Dune 3.17 to satisfy the repository’s required Dune language version.
make still failed because opam packages such as flow_parser, yojson, ounit2, cmarkit, and the tool cppo were missing from the environment
make test likewise failed for the same missing dependencies, so the test suite could not run

Testing

❌ make
❌ make test

Notes

Access to opam repositories is blocked (HTTP 403), preventing installation of the required OCaml packages and tools.

View task →

Signed-off-by: Cristiano Calcagno <[email protected]>

…rcion Mark shared constructors as used on the target variant after successful coercion; keep warnings for target-only constructors. Part of #7839.

cristianoc · 2025-09-06T03:24:56Z

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!

compiler: update typecore; tests: add VariantCoercionConstructUsed; d…

992088e

…ocs: update AGENTS; docstring_tests: update DocTest outputs Signed-off-by: Cristiano Calcagno <[email protected]>

cristianoc requested a review from jfrolich September 4, 2025 10:21

cristianoc mentioned this pull request Sep 4, 2025

Incorrect compiler warning (constructor is never used to build values) #7838

Open

cristianoc requested a review from zth September 4, 2025 10:24

zth requested changes Sep 4, 2025

View reviewed changes

next iter

7f1f3c4

cristianoc requested a review from zth September 4, 2025 11:37

cristianoc added 2 commits September 4, 2025 13:42

changelog: link variant coercion unused-constructor fix to #7839

6012c86

cristianoc added 2 commits September 5, 2025 12:00

typecore: avoid option tuple match; wrap both extractions in single t…

5d5e111

…ry/with Mark 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 <[email protected]>

typecore: speed up constructor membership with StringSet

bfcef55

Build a set of source constructor names and use O(log n) membership checks instead of O(n) List.exists for each target constructor. Signed-off-by: Cristiano Calcagno <[email protected]>

zth reviewed Sep 5, 2025

View reviewed changes

tests/docstring_tests/DocTest.res Show resolved Hide resolved

cristianoc added 2 commits September 6, 2025 05:11

chore: remove example.res and issue.txt

2c6a0e9

Signed-off-by: Cristiano Calcagno <[email protected]>

typecore: avoid false 'constructor … is never used' after variant coe…

7da7c05

…rcion Mark shared constructors as used on the target variant after successful coercion; keep warnings for target-only constructors. Part of #7839.

		let has_src name = List.exists (fun n -> n = name) src_names in
		let tgt_ty_name = Path.last p_tgt_conc in

compiler: update typecore; tests: add VariantCoercionConstructUsed; d… #7839

Are you sure you want to change the base?

compiler: update typecore; tests: add VariantCoercionConstructUsed; d… #7839

Uh oh!

Conversation

cristianoc commented Sep 4, 2025

Uh oh!

pkg-pr-new bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

zth commented Sep 4, 2025

Uh oh!

cristianoc commented Sep 4, 2025

Uh oh!

zth commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfrolich commented Sep 5, 2025

Uh oh!

zth commented Sep 5, 2025

Uh oh!

cristianoc commented Sep 5, 2025

Uh oh!

zth commented Sep 5, 2025

Uh oh!

cristianoc commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Sep 5, 2025

Choose a reason for hiding this comment

Summary

Testing

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Sep 5, 2025

Choose a reason for hiding this comment

Summary

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Sep 5, 2025

Choose a reason for hiding this comment

Summary

Testing

pkg-pr-new bot commented Sep 4, 2025 •

edited

Loading

zth commented Sep 5, 2025 •

edited

Loading