Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Mantis PR#7767: interaction between labels and let rec check #1712

Merged
merged 3 commits into from Apr 11, 2018

Conversation

Projects
None yet
2 participants
@yallop
Copy link
Member

yallop commented Apr 9, 2018

A partially-applied labeled function, like this:

let f () ~y = e in f ~y

is translated into code that builds a function value, like this:

let f () ~y = e in let g = f in let z = y in fun () -> g ~y:z ()

Consequently, the let rec check used to accept definitions whose rhss were partial applications of this form (Mantis PR 7767).

This PR adds a case for partially-applied labeled functions to the new let rec check, to restore the old behaviour.

@@ -1954,6 +1954,9 @@ struct
| Texp_extension_constructor _ -> Static
| Texp_apply ({exp_desc = Texp_ident (_, _, vd)}, _)
when is_ref vd -> Static
| Texp_apply (_,args)
when List.exists (function (_,None) -> true | _ -> false) args ->
Static

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

To someone reading the code without the full typedtree structure in mind, it is very unclear what this case is about (is_ref above is self-explanatory). Maybe you could avoid the when here, have a single (non-ref) Texp_apply case, and have on the test a comment explaining what the test is about? (Or you could make the test an auxiliary function with a self-explanatory name.)

This comment has been minimized.

@yallop

yallop Apr 10, 2018

Author Member

Or you could make the test an auxiliary function with a self-explanatory name.

I picked this option, so that the named function could be reused in the two locations where it's needed.

(* an expression abstracted over a missing argument *)
let arg env (_, eo) = option expression env eo in
let ty = Use.join (list arg env args) (expression env e) in
Use.(join (discard ty) (delay ty))

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

I don't remember the details well, but isn't join (discard ty) (delay ty) equal to delay ty?

I think it could be nice if you merged the two cases. If it is the case that inspect (join a b) is equal to join (inspect a) (inspect b), you could write a single logic, with a test that applies either a delay or an inspect depending on whether the application is delayed by a missing argument.

This comment has been minimized.

@yallop

yallop Apr 10, 2018

Author Member

isn't join (discard ty) (delay ty) equal to delay ty?

It's equal to discard ty, I think.

Thanks for the suggestions. I've merged the two cases, and added a comment.

This comment has been minimized.

@gasche

gasche Apr 11, 2018

Member

I checked and indeed discard, which is an alias for guard, is less permissive than delay, so their union is discard. This is still a bit non-intuitive to me, but I guess that in a call-by-value setting it is natural that evaluating now (and then not using) demands more than evaluating later.

@yallop yallop force-pushed the yallop:letrec-labels branch from 05aaf03 to f5645ab Apr 10, 2018

@yallop yallop force-pushed the yallop:letrec-labels branch from f5645ab to 74e0eeb Apr 10, 2018

@gasche

gasche approved these changes Apr 11, 2018

@gasche gasche merged commit 335e111 into ocaml:trunk Apr 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yallop yallop deleted the yallop:letrec-labels branch Apr 11, 2018

gasche added a commit that referenced this pull request Apr 11, 2018

Merge branch 'letrec-labels-4.07' into 4.07
This is the rebase of GPR #1712.
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 11, 2018

Merged in 4.07 by 92a0f54.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.