Remove the unreachable Longident.Lapply constructor#8469
Conversation
Longident.Lapply is the parsetree form of OCaml's applicative-functor path syntax (F(X).t), where a functor is applied inline inside an identifier path. ReScript's grammar has no such production: functor application is only a module expression (module M = F(X)), which the type checker resolves to Path.Papply, never a Longident.Lapply. The parser never builds Lapply, so every match on it was dead (assert false / fatal_error / raise Not_found / defensive printer fallbacks). Remove the constructor and all its handling across the type checker, printers, depend, and analysis. The sole construction site was Ctype.lid_of_path (Path.Papply -> Longident.Lapply); since no Papply flows into that type-path conversion, replace it with assert false, matching the existing Papply -> assert false invariants. Removing the Env.lookup_module Lapply arms also left functor_components.fcomp_arg write-only, so drop that field too. Add a super_errors fixture exercising functor application plus a type-path error, confirming the functor machinery and error printing are unchanged.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a11e0c9f4
ℹ️ 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".
| (**************************************************************************) | ||
|
|
||
| type t = Lident of string | Ldot of t * string | Lapply of t * t | ||
| type t = Lident of string | Ldot of t * string |
There was a problem hiding this comment.
In this repository parsetree0.ml still exposes Longident.t in the frozen v0 PPX AST (for example Ptyp_constr of Longident.t loc * ...), and the AGENTS.md guidance says existing PPXs rely on that frozen version. Removing the Lapply constructor from the shared Longident.t type breaks any existing PPX that was compiled against or constructs the v0 AST with applicative-functor paths, even though the ReScript parser itself never produces them. Please keep the v0-facing shape compatible (or introduce a mapped v0 longident) instead of deleting the constructor from the shared type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is not really true.
It only breaks if the ppx tries to construct Lapply, it can never read this variant from ReScript code. There should be no real world ReScript ppxs that hit this.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8469 +/- ##
==========================================
+ Coverage 74.34% 74.42% +0.08%
==========================================
Files 450 451 +1
Lines 61518 61459 -59
==========================================
+ Hits 45736 45743 +7
+ Misses 15782 15716 -66
🚀 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: |
JonoPrest
force-pushed
the
jono/remove-longident-lapply
branch
from
7bd4d4e to
4731ede
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4731eded6f
ℹ️ 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".
| (* Functor-application paths (Path.Papply) only arise from applicative | ||
| functors, which ReScript's surface syntax cannot express as a path; no | ||
| such path reaches this type-path conversion. *) | ||
| | Path.Papply _ -> assert false |
There was a problem hiding this comment.
Applicative functor paths are still produced internally even if users cannot write F(X).t directly: Typemod.path_of_module returns Papply for module App = F(Arg), and Mtype.strengthen records abstract result types under Papply paths. When later diagnostics call Ctype.lid_of_path for one of those strengthened paths (for example while printing or disambiguating an error involving an abstract type from a functor application), this new assertion aborts the compiler instead of reporting the type error; the previous Longident.Lapply branch handled exactly that internal path shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
If true, this is an issue.
There was a problem hiding this comment.
@cristianoc, I couldn't find a way to hit this from ReScript source. Tried to get codex to hit this path unsuccessfully as well. But either way I've now added a way to handle it and added an internal unit test for this branch.
Applicative functors are on by default, so Path.Papply paths are produced internally (typemod path_of_module, Mtype.strengthen) even though ReScript source cannot reference such a path directly. Replace the assert false in Ctype.lid_of_path's Papply arm (introduced when removing Longident.Lapply) with a graceful Lident rendering via Path.name, so any diagnostic that ever reaches it degrades to a readable name (e.g. F(Arg)) instead of aborting the compiler. Add an ounit test that calls Ctype.lid_of_path on a synthetic Papply path (unreachable from source, so exercised directly) to lock in the graceful behavior.
JonoPrest
force-pushed
the
jono/remove-longident-lapply
branch
from
ca2c07a to
11f6a61
Compare
2 participants
Follow up to #8459
@cknitt suggested that we should remove Longident.Lapply constructor since it's not reachable from ReScript's grammar.
#8459 (comment)
ppxs could technically produce this AST but in practice it's not usable in ReScript