Fix incorrect analysis report for optional function args by cknitt · Pull Request #8321 · rescript-lang/rescript

cknitt · 2026-03-31T11:33:05Z

Fixes #8302

@cristianoc This was done by Codex, can you check if it makes sense?

cknitt · 2026-03-31T11:33:23Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57f9dd1a65

ℹ️ 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".

chatgpt-codex-connector · 2026-03-31T11:38:42Z

+      if posTo.pos_fname <> posFrom.pos_fname then
+        match Declarations.find_opt_builder decls posTo with
+        | Some {declKind = Value {ownsOptionalArgs; optionalArgs}} ->
+          ownsOptionalArgs && not (OptionalArgs.isEmpty optionalArgs)


Check source ownership for cross-file function refs

The cross-file branch only validates posTo, so it still records function references whose posFrom is a non-owning alias/value with optionalArgs = empty. In this commit, that empty source state is then merged in compute_optional_args_state via OptionalArgs.combine_pair (which intersects states), so a live cross-file alias like let g = A.f can erase f’s optional-arg state and suppress legitimate optional-argument diagnostics on the real declaration.

Useful? React with 👍 / 👎.

cknitt · 2026-03-31T11:52:28Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6969ec07c

ℹ️ 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".

chatgpt-codex-connector · 2026-03-31T11:58:48Z

+      if not (List.mem locFrom !callee_locs) then
+        suppressOptionalArgs locTo.loc_start)


Stop erasing optional args on non-call identifier refs

The new collectExpr path clears a declaration’s optionalArgs whenever it sees a Texp_ident that is not in callee_locs, which includes ordinary aliasing like let alias = f. This mutates f before optional-arg state is computed, so later direct calls to f start from an empty optional-arg set and DeadOptionalArgs.check can no longer report real unused/always-supplied optional arguments; even dead aliases can suppress diagnostics for the original function.

Useful? React with 👍 / 👎.

cknitt · 2026-03-31T12:11:47Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c7342857a

ℹ️ 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".

chatgpt-codex-connector · 2026-03-31T12:17:23Z

+        match Declarations.find_opt_builder decls posTo with
+        | Some {declKind = Value {optionalArgs}} ->
+          not (OptionalArgs.isEmpty optionalArgs)
+        | _ -> false


Record optional calls even when target decl isn't built yet

The same-file guard in addReferences drops a call unless Declarations.find_opt_builder decls posTo already has a non-empty optional-arg declaration. In a let rec ... and ... group, expressions are traversed per binding before later bindings are added, so a call from an earlier binding to a later optional-arg function can hit None here and never be recorded. This undercounts real calls and can produce wrong optional-argument diagnostics (e.g., reporting an arg as unused/redundant when it is used through mutual recursion).

Useful? React with 👍 / 👎.

pkg-pr-new · 2026-03-31T12:19:52Z

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@8321

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@8321

@rescript/win32-x64

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

commit: 71e38c2

cristianoc

Are the tests OK? Seems like no new warnings reported, and I guess that's the intention?

I had this looked at, some changes here seem similar.
Though I remember some fundamental issue that aliasing cannot really be tracked.
It would be useful to generate a detailed explanation of the code changes.

cknitt · 2026-04-01T08:14:00Z

@cristianoc Thanks!

Are the tests OK? Seems like no new warnings reported, and I guess that's the intention?

Yes, exactly.

It would be useful to generate a detailed explanation of the code changes.

Will add that.

However, another (local) Codex review found an issue that still needs to be fixed:

This branch clears ownsOptionalArgs for any top-level identifier use that is not the direct callee of an application, which also catches ordinary Tstr_eval expressions. Rebuilding the deadcode fixture with let takesFn = _ => (); takesFn(formatDate) makes formatDate stay live but drops the expected optional argument fmt of function formatDate is never used warning, so passing a function as a first-class value at top level now produces a false negative.

Will set this to draft in the meantime.

konradtho · 2026-05-13T06:22:17Z

Any update on this?

konradtho · 2026-06-05T19:39:39Z

@cknitt Not to bother you, but would be so nice to be able to have reanalyze work as expected in ci/cd pipelines to not loose track of any dead code

stian-strand · 2026-06-16T06:34:57Z

Also waiting for this one

stian-strand · 2026-07-03T07:34:16Z

Its my birthday soon, would be a great present getting this fixed :))

Fridyyy · 2026-07-03T08:08:44Z

Also waiting for this one @cknitt!

cknitt · 2026-07-07T14:05:41Z

I recently tried to rebase and finish it, but ran into more and more issues / edge cases with each change + Codex review.

I am not really the expert on reanalyze, it may take @cristianoc to finish this I am afraid.

I will push may rebased attempt.

Signed-off-by: Christoph Knittel <ck@cca.io>

codecov · 2026-07-07T14:26:50Z

Codecov Report

❌ Patch coverage is 76.62338% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.66%. Comparing base (8cb30c7) to head (71e38c2).

Files with missing lines	Patch %	Lines
analysis/reanalyze/src/dead_value.ml	75.55%	11 Missing ⚠️
analysis/reanalyze/src/dead_optional_args.ml	75.00%	4 Missing ⚠️
analysis/reanalyze/src/cross_file_items_store.ml	71.42%	2 Missing ⚠️
analysis/reanalyze/src/reanalyze.ml	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8321      +/-   ##
==========================================
- Coverage   74.66%   74.66%   -0.01%     
==========================================
  Files         457      457              
  Lines       62110    62174      +64     
==========================================
+ Hits        46373    46420      +47     
- Misses      15737    15754      +17

Files with missing lines	Coverage Δ
analysis/reanalyze/src/cross_file_items.ml	`88.88% <100.00%> (+2.22%)`	⬆️
analysis/reanalyze/src/dead_common.ml	`73.38% <100.00%> (+0.10%)`	⬆️
analysis/reanalyze/src/decl.ml	`77.77% <ø> (ø)`
analysis/reanalyze/src/reactive_merge.ml	`0.00% <ø> (ø)`
analysis/reanalyze/src/reanalyze.ml	`48.56% <50.00%> (+<0.01%)`	⬆️
analysis/reanalyze/src/cross_file_items_store.ml	`81.08% <71.42%> (+1.08%)`	⬆️
analysis/reanalyze/src/dead_optional_args.ml	`87.69% <75.00%> (-4.31%)`	⬇️
analysis/reanalyze/src/dead_value.ml	`83.06% <75.55%> (-2.32%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2026-07-07T15:05:38Z

Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8321

cknitt requested a review from cristianoc March 31, 2026 11:33

chatgpt-codex-connector Bot reviewed Mar 31, 2026

View reviewed changes

cknitt force-pushed the fix-reanalyze branch from b2972b2 to ea581f1 Compare March 31, 2026 11:42

chatgpt-codex-connector Bot reviewed Mar 31, 2026

View reviewed changes

cristianoc reviewed Mar 31, 2026

View reviewed changes

cknitt marked this pull request as draft April 1, 2026 08:14

cknitt force-pushed the fix-reanalyze branch from 8c73428 to 08e1540 Compare July 7, 2026 14:06

cknitt added 8 commits July 7, 2026 16:09

Fix incorrect analysis report for optional function args

5721ce8

Fix

f39d89f

Fix optional arg analysis for function values

bf947f5

Signed-off-by: Christoph Knittel <ck@cca.io>

Suppress optional arg warnings for escaped functions

415ea85

Signed-off-by: Christoph Knittel <ck@cca.io>

Make optional arg value escapes liveness-aware

94df3b4

Signed-off-by: Christoph Knittel <ck@cca.io>

Track optional arg escapes from value references

8de3ef5

Signed-off-by: Christoph Knittel <ck@cca.io>

Track cross-file optional arg value escapes

ac4507f

Signed-off-by: Christoph Knittel <ck@cca.io>

Ignore optional arg wrappers as value escapes

71e38c2

Signed-off-by: Christoph Knittel <ck@cca.io>

cknitt force-pushed the fix-reanalyze branch from 08e1540 to 71e38c2 Compare July 7, 2026 14:15

		if not (List.mem locFrom !callee_locs) then
		suppressOptionalArgs locTo.loc_start)

Uh oh!

Conversation

cknitt commented Mar 31, 2026

Uh oh!

cknitt commented Mar 31, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

cknitt commented Mar 31, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

cknitt commented Mar 31, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

pkg-pr-new Bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

cknitt commented Apr 1, 2026

Uh oh!

konradtho commented May 13, 2026

Uh oh!

konradtho commented Jun 5, 2026

Uh oh!

stian-strand commented Jun 16, 2026

Uh oh!

stian-strand commented Jul 3, 2026

Uh oh!

Fridyyy commented Jul 3, 2026

Uh oh!

cknitt commented Jul 7, 2026

Uh oh!

codecov Bot commented Jul 7, 2026

Codecov Report

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pkg-pr-new Bot commented Mar 31, 2026 •

edited

Loading