Lazy strengthening #119
Lazy strengthening #119
Conversation
This change ensures that in ``` module M : S functor F(X:S) = ... module N = F(M) ``` we don't unfold `S` when typing `F(M)` and just do a quick shallow comparison. However, it can cause new "unused value" and "unused module" warnings since we no longer mark things inside of `S` as used when typing `F(M)`. These warnings seem sensible and an improvement over the previous state. The new test cases demonstrate when they happen.
| Psig_module {pmd_type = {pmty_desc = | ||
| Pmty_with (mty, [Pwith_module (_,lid)])}}}])} | ||
| when str.txt = "strengthen" -> Some (mty, lid) | ||
| | _ -> None |
There was a problem hiding this comment.
I think these should probably be done via the new stuff in parsing/extensions.ml and parsing/extensions_parsing.ml. Maybe talk to @antalsz or @goldfirere about getting a PR to add support for module types to those first and then rebase and use it?
Looking at how those files work, it looks like we'd use an encoding like:
functor (_ : [%extension.strengthen]) -> S with module L = Lwhich is maybe a little easier than the encoding here and probably works better with ppxen.
There was a problem hiding this comment.
Yes -- I'd be happy to throw that together.
There was a problem hiding this comment.
functor (_ : [%extension.strengthen]) -> S with module L = L
I'm not doubting you're right but I'd like to understand why this is better. I'm a bit worried that this would replace a signature with a functor type.
There was a problem hiding this comment.
It's a valid worry.
The problem with your original encoding is that ppxen don't generally look inside of extension nodes -- meaning your strengthened module types will be invisible to all ppx transformations, which could be bad.
Instead, the general-purpose extensions architecture is designed to be consulted essentially at every inspection of a parsetree type. The actual encoding I chose (in #141/#142) is functor (_ : [%extensions.strengthen]) (_ : S) -> L. (I decided not to use with module L = L to avoid duplication.) I know the L is a module, not a module type, but the parsetree just wants a Longident.t there, so it doesn't care about the distinction. (Though maybe this is bad, in case a ppx is looking for module identifiers? Feedback welcome, at https://github.com/ocaml-flambda/ocaml-jst/pull/142/files#r1131011021)
As for your worry: we must be careful that every time we look at a module type, we check to see if it's the special form of a syntax extension. I've done this in #141. But the check that I've done it right is just "look to see if it looks right". Because we can't edit the parsetree, we can't really be sure of not making a mistake here. (Indeed, in #140, I found a few places where we might be getting this wrong for some expression extensions.)
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This is a significant win in some cases. Note that ocaml-flambda/ocaml-jst#119 changes this area significantly so I didn't want to refactor too much here.
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This is a significant win in some cases. Note that ocaml-flambda/ocaml-jst#119 changes this area significantly so I didn't want to refactor too much here.
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This is a significant win in some cases. Note that ocaml-flambda/ocaml-jst#119 changes this area significantly so I didn't want to refactor too much here.
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This is a significant win in some cases. Note that ocaml-flambda#119 changes this area significantly so I didn't want to refactor too much here.
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This substantially reduces the peak heap size (and improves compile times, albeit to a lesser degree) in some cases. Note that ocaml-flambda#119 changes this area significantly so I didn't want to refactor too much here.
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This substantially reduces the peak heap size (and improves compile times, albeit to a lesser degree) in some cases. Note that #119 changes this area significantly so I didn't want to refactor too much here. Co-authored-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This substantially reduces the peak heap size (and improves compile times, albeit to a lesser degree) in some cases. Note that ocaml-flambda/ocaml-jst#119 changes this area significantly so I didn't want to refactor too much here.
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This substantially reduces the peak heap size (and improves compile times, albeit to a lesser degree) in some cases. Note that ocaml-flambda/ocaml-jst#119 changes this area significantly so I didn't want to refactor too much here.
We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This substantially reduces the peak heap size (and improves compile times, albeit to a lesser degree) in some cases. Note that ocaml-flambda/ocaml-jst#119 changes this area significantly so I didn't want to refactor too much here. Co-authored-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
…1184) We were calling `expand_module_alias` to get the unstrengthened, non-lazy type (potential deep copy), then checking whether it's an ident and then strengthening it, thus throwing most of it away. This patch avoids doing that by working directly on the lazy representation. This substantially reduces the peak heap size (and improves compile times, albeit to a lesser degree) in some cases. Note that #119 changes this area significantly so I didn't want to refactor too much here. Co-authored-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
No description provided.