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

Error when calling non-closed local server functions #1136

Open
jamescheney opened this issue Apr 28, 2022 · 7 comments
Open

Error when calling non-closed local server functions #1136

jamescheney opened this issue Apr 28, 2022 · 7 comments
Labels

Comments

@jamescheney
Copy link
Contributor

Thanks to #1133 it is now possible to write local / anonymous functions that have client or server annotations. Such functions need not be closed, for example:

fun foo(x) client { replaceNode(
    (fun (y) server  {<div id="xyz"><p><b>{stringToXml(x)}</b>:<b>{stringToXml(y)}</b></p></div>})("xyzzy")
    , getNodeById("xyz"))
}

fun testPage(msg) {
  page
  <html>
    <body>
      <div>Message: {stringToXml(msg)}</div>
        <form l:onsubmit="{foo(x)}" method="post">
          <input type="text" l:name="x"/>
  	<button type="submit">Submit!</button>
        </form>
      <div id="xyz">This should be replaced</div>
    </body>
  </html>
}



fun main () {
  addRoute("",testPage);
  servePages()
}

main()

However, running this and clicking the Submit button yields (I think) the same error @kwanghoon reported in a comment on #907, adjusting for changes:

Uncaught Error: Fatal error: call to server returned an error. Details: ***: Error: File "core/evalir.ml", line 252, characters 18-24: Assertion failed

This error appears in the browser console, but relays an error raised on the server.

Removing the server label from the anonymous function yields correct behavior, presumably because the function is called on the client and it is closure converted correctly there.

Replacing foo with the following:

fun foo(x) client { replaceNode(
    (fun (x,y) server {<div id="xyz"><p><b>{stringToXml(x)}</b>:<b>{stringToXml(y)}</b></p></div>})(x,"xyzzy")
    , getNodeById("xyz"))
}

so that the anonymous server function is lambda-lifted in place, yields correct behavior.

The error is being raised here:

| _, _ -> assert false

which is an "unreachable" case of a pattern match that tries to match up the closure variable and environment. Evidently these are getting out of sync somehow, possibly due to an unhandled case or no-longer-guaranteed invariant in closure conversion. Through random hacking I found that the change in commit 8cee258 "fixes" this problem. But this is almost certainly not the right way to fix it, the right way is to make sure that the function info and environment/argument list do not get out of sync in the first place.

@kwanghoon
Copy link
Contributor

kwanghoon commented Apr 30, 2022

Hello @jamescheney,

In Links, CPS-converted functions are thought to take its environment as their first argument.

When the example program calls a server function with a text "zzzzz", cgi_args is as follows:

   __name: "MjQwMQ=="                                              ===> 2401
   __args: "eyIxIjp7IjIzOTYiOiJ6enp6eiJ9LCIyIjoieHl6enkifQ=="      ===> {"1":{"2396":"zzzzz"},"2":"xyzzy"}
   __env: "e30="                                                   ===> {}
   __client_id: "Y2lkXzA="                                        ===> cid_0

A quick workaround is to change webif.ml

  • to construct a FunctionPtr with Some (hd args) as its environment, and
  • to omit the first argument from the arguments by tl args.

let func =

    let func,args =
      match fvs with
      | `Record [] -> let i_fname = int_of_string fname in
          if Lib.is_primitive_var i_fname
          then `PrimitiveFunction (Lib.primitive_name i_fname, Some i_fname),args
          else match hd args with
               | `Record _ -> `FunctionPtr (int_of_string fname, Some (hd args)), tl args
               | _ -> `FunctionPtr (int_of_string fname, None), args
      | _ -> `FunctionPtr (int_of_string fname, Some fvs), args
    in
    RemoteCall (func, valenv, args)

@jamescheney
Copy link
Contributor Author

jamescheney commented Apr 30, 2022

Thanks. As I understand it, thanks to closure conversion the environment part of the call is supposed to always be empty (there is even a comment in jslib suggesting to get rid of it). So like my workaround mentioned above this seems to fix the symptom but not the root cause of the problem.

I think the issue is that after closure conversion, when we call the server from the client we look up information about the function's arguments and closure variable (if any) using find_fun, and this information is inconsistent with the closure converted version: a closure variable is present, but the environment is being passed as an ordinary argument. My workaround handles that case by patching the argument list with the closure variable if there is one but no environment was passed.

I think the right way to do this is fix the dictionary used by fun_info so that after closure conversion is finished, the closure variable are all added to the argument list, but haven't had the time to look at what is currently done to see if this actually makes sense.

@kwanghoon
Copy link
Contributor

Thanks for the explanation.

One thing still strange to me is that in the web interface, only FunctionPtr with None as the second argument is created
if the environment part of the remote call is supposed to always be empty (i.e., Record []), whatever the closure variable (z) is.
I don't know what I am missing.

links/core/webif.ml

Lines 31 to 52 in 474137f

let parse_remote_call (valenv, _, _) cgi_args =
let fname = Utility.base64decode (assoc "__name" cgi_args) in
let args = Utility.base64decode (assoc "__args" cgi_args) in
let args = Value.untuple (U.Value.load (Yojson.Basic.from_string args)) in
let fvs = U.Value.load (Yojson.Basic.from_string (Utility.base64decode (assoc "__env" cgi_args))) in
(* There is an artificial distinction between primitive
functions (builtin functions) and function pointers (functions
defined as part of a Links program). This is the point where we
have to do something about it in order for attempts to remotely
call primitive functions to work properly. *)
let func =
match fvs with
| `Record [] -> let i_fname = int_of_string fname in
if Lib.is_primitive_var i_fname
then `PrimitiveFunction (Lib.primitive_name i_fname, Some i_fname)
else `FunctionPtr (int_of_string fname, None)
| _ -> `FunctionPtr (int_of_string fname, Some fvs)
in
RemoteCall (func, valenv, args)

@jamescheney
Copy link
Contributor Author

It seems in the server-side IR, after closure conversion things are still represented as "closures", but the function bodies refer to the environment through the closure variable defined in the functions fun_def entry. Thus, the IR code isn't changed so that the IR manages the environment itself, instead whenever a closure is created the current environment is stored in it and when a call happens, that environment is passed in bound to the closure variable, if any. However, when compiling code to the client for server calls we can't do this because we are generating really-closed JS functions.

It looks like Record [] is being used to encode NONE, since environments / closure variables only get created if the function had some local variable references.

I suspect a reason it'd be annoying to fully closure-convert the IR code (i.e. make the closure variable a normal parameter, and rewrite calls/closure creation to manage the environment using IR code rather than having the interpreter do it) is that IR typing would become more complicated: functions with the same input/output types but different environments would have different types. This problem can be solved by adding existential types so that the type of a closure-converted function can be something like "exists Env. ((Env,t1,...,tn) -> t , Env)" but we currently don't have this even in the IR. That being the case, since the mismatch seems to arise at the point where we create a FunctionPtr for a server-side call decoded from a client request, I think my suggestion is: when the server-side function has a closure variable, but the environment provided is empty, pop off the first parameter and treat it as the environment. My fix does the same thing but in a much more mysterious way (and one which, if there were other bugs leading to mismatches between the function info and optional closure in server side code, would potentially lead to other hard-to-debug situations).

@jamescheney
Copy link
Contributor Author

Having looked at some of the generated IR to see how the above two examples differ, I came up with a smaller example that seems to result in the same problem. The following code works:

mutual {
fun testPage(msg) {
  page
  <html>
    <body>
	<div>Message: {stringToXml(msg)}</div>
      <div id="x">
        <p>GAGA</p>
      </div>
      <a href="" l:onclick="{changeContent()}">Click me</a>
    </body>
  </html>
}

fun snippet (time,testPage) server {
        <a l:href="{testPage(intToString(time))}">href handled OK</a>
}

fun changeContent() client {
  var time = clientTime();
  replaceNode(snippet(time,testPage),getNodeById("x"))
}
}

fun main () {
  addRoute("",testPage);
  servePages()
}

main()

Here, I've modified snippet to get rid of the time but also be completely closed, i.e. not refer to the other functions in the same mutual block.
This version where snippet is lifted out of the mutual block however fails with the same error as above:

fun snippet (testPage) server {
        <a l:href="{testPage("42")}">href handled OK</a>
}

mutual {
fun testPage(msg) {
  page
  <html>
    <body>
	<div>Message: {stringToXml(msg)}</div>
      <div id="x">
        <p>GAGA</p>
      </div>
      <a href="" l:onclick="{changeContent()}">Click me</a>
    </body>
  </html>
}

fun changeContent() client {
  replaceNode(snippet(testPage),getNodeById("x"))
}
}

fun main () {
  addRoute("",testPage);
  servePages()
}

main()

I think what may be happening is that for some reason, the call to testPage in the second version is being attempted as a client call, which is of course silly and unnecessary. But even by annotating testPage as server in the second version the same error still occurs, which suggests that somewhere a client closure is being incorrectly created.

@jamescheney
Copy link
Contributor Author

Hmm. So changing the working version to:

mutual {
fun testPage(msg) {
  page
  <html>
    <body>
	<div>Message: {stringToXml(msg)}</div>
      <div id="x">
        <p>GAGA</p>
      </div>
      <a href="" l:onclick="{changeContent()}">Click me</a>
    </body>
  </html>
}

fun snippet (time,f) server {
        <a l:href="{f(intToString(time))}">href handled OK</a>
}

fun changeContent() client {
  var time = clientTime();
  replaceNode(snippet(time,testPage),getNodeById("x"))
}
}

fun main () {
  addRoute("",testPage);
  servePages()
}

main()

makes it fail again. So it seems that there may be another issue, relating to scoping/resolution of names in mutual blocks.

jamescheney added a commit that referenced this issue May 26, 2022
Orbion-J pushed a commit to Orbion-J/links that referenced this issue Jun 30, 2022
Orbion-J pushed a commit to Orbion-J/links that referenced this issue Jul 8, 2022
Orbion-J added a commit to Orbion-J/links that referenced this issue Jul 8, 2022
desalias inline

alias = effectname and typename | init

merge into 1 construct ok; still 2 contexts

merge type and effect aliases contexts

effectname body desugared into effectname
allow better printing

Revert "desalias inline"

This reverts commit 49a9a61.

Revert "effectname body desugared into effectname"

This reverts commit 5150615.

"effectname" in links-mode.el

cleaning before pr

re-cleaning

effectname -> typename _::Kind

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/desugarDatatypes.mli

Co-authored-by: Daniel Hillerström <[email protected]>

Update bin/repl.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/defaultAliases.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/desugarDatatypes.ml

Co-authored-by: Daniel Hillerström <[email protected]>

Update core/desugarDatatypes.ml

Co-authored-by: Daniel Hillerström <[email protected]>

rename alias_env -> tycon_env as originally

Update core/desugarDatatypes.ml

Co-authored-by: Daniel Hillerström <[email protected]>

idem

fixes & add primary kind in aliases

new error kind mismatch

fix : embeded errors

fixes

fix : underscore in effect app

various fixes

short type in effectname + short fun non desugar

comment

Attempted workaround for links-lang#1136 (links-lang#1138)

always desugar op type with type application

correction tests

default arg in repl

Effect aliasing (links-lang#1141)

I added the possibility to write effect aliases, similarly as what already exists for types.
- there is a new keyword `effectname`
- to write an alias for an effect row : `effectname MyEffectRow(a, ... ,e::Eff, ...) = { Op1 : type, ... | e }` the arguments being type variable of any kind (kinds other than type must be explicit)
- we can have, as above, an open row (the row variable is a parameter of the effect alias) or a closed one (just do not write the `| e`
- To use it in a signature or in another type or effect alias just apply it with the right arguments as for `typename` things.
- In arrows, we can use them as row variables: `() -MyEffectRow(args)-> ()` (idem with `~>`)
- However, due to lack of kind inference, row variables and aliases have to be used carefully so that links does not think they are of kind type. We need to write them most of the time between braces ` { | ... }`. For instance, if you have `effectname E(a::Eff) = {X : ... | a }` and a row variable `e::Eff`, you will have to write `E({ |e})`. (Idem for another effect alias instead of the variable). This makes the usage of several nested aliases a bit messy, it would be nice if we could avoid it.
- We cannot write recursive effect aliases for now. In the branch `visitor`, I added another transformer that makes possible simple recursion by inlining a mu type in one pass.
- For now the aliases are replaced by the row they correspond to : we do not keep aliases.
- In the repl, effect alias definitions are printed but without the braces ! Rows are in general printed without braces and the alias body is a row. => this might need to be enhanced

About implementation, I copied and then merged most of the time what existed for `typename`.

Co-authored-by: Daniel Hillerström <[email protected]>

Fix assert error with relational lenses (links-lang#1143)

The `Lens` type traversal was unimplemented and filled in with an `assert false`. As a result, all RL code fails.

I don't think there is any really sensible default traversal due to the complexity of the Lens types, so I have just filled it in with the identity. This doesn't stop someone implementing a traversal -- they'll just need to write one for `Lens.Type.t` type and plug it in as usual.

Make `custom_js_runtime` a multi option (links-lang#1146)

Added the ability to link multiple custom js runtime files.

Co-authored-by: s1908422 <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>

Allow 'default' as an argument to settings in the REPL (links-lang#1147)

Some settings has the value 'default'. Prior to this patch the value 'default' could not be written in the REPL, because it is token. This patch rectifies this problem by allowing the token 'default' to appear in settings argument position in the REPL.

Co-authored-by: Daniel Hillerström <[email protected]>

Allow record extension in presence of temporal projections (links-lang#1129)

* Allow record extension in presence of temporal projections

This allows record extension to play nicely with temporal projections.
I was forgetting that all arguments to reduce_artifacts are already
eta-expanded, so we can work with record literals.

Fixes links-lang#1124.

* Allow extension to work with (shallow) temporal projections

We don't have the full generality due to links-lang#1130, but this patch should
allow record extension to be used on temporal projections on variables /
record literals.

Removal of unused headless testing (links-lang#1152)

The headless testing has been unused for about a year, because it is unmaintained. Even though it is not used, it generates a ton of security warnings here on GitHub. I am not interested in dealing with those, therefore this patch removes the headless testing directory from the source tree.

Attempted workaround for links-lang#1136 (links-lang#1138)

always desugar op type with type application

correction tests

default arg in repl

Option -> Maybe fix (links-lang#1142)

The `Option -> Maybe` refactoring patch links-lang#1131 missed one instance: the
`max` function. This patch changes the type signature of `max` to use
`Maybe` rather than `Option`.

Effect aliasing (links-lang#1141)

I added the possibility to write effect aliases, similarly as what already exists for types.
- there is a new keyword `effectname`
- to write an alias for an effect row : `effectname MyEffectRow(a, ... ,e::Eff, ...) = { Op1 : type, ... | e }` the arguments being type variable of any kind (kinds other than type must be explicit)
- we can have, as above, an open row (the row variable is a parameter of the effect alias) or a closed one (just do not write the `| e`
- To use it in a signature or in another type or effect alias just apply it with the right arguments as for `typename` things.
- In arrows, we can use them as row variables: `() -MyEffectRow(args)-> ()` (idem with `~>`)
- However, due to lack of kind inference, row variables and aliases have to be used carefully so that links does not think they are of kind type. We need to write them most of the time between braces ` { | ... }`. For instance, if you have `effectname E(a::Eff) = {X : ... | a }` and a row variable `e::Eff`, you will have to write `E({ |e})`. (Idem for another effect alias instead of the variable). This makes the usage of several nested aliases a bit messy, it would be nice if we could avoid it.
- We cannot write recursive effect aliases for now. In the branch `visitor`, I added another transformer that makes possible simple recursion by inlining a mu type in one pass.
- For now the aliases are replaced by the row they correspond to : we do not keep aliases.
- In the repl, effect alias definitions are printed but without the braces ! Rows are in general printed without braces and the alias body is a row. => this might need to be enhanced

About implementation, I copied and then merged most of the time what existed for `typename`.

Co-authored-by: Daniel Hillerström <[email protected]>

Fix assert error with relational lenses (links-lang#1143)

The `Lens` type traversal was unimplemented and filled in with an `assert false`. As a result, all RL code fails.

I don't think there is any really sensible default traversal due to the complexity of the Lens types, so I have just filled it in with the identity. This doesn't stop someone implementing a traversal -- they'll just need to write one for `Lens.Type.t` type and plug it in as usual.

Make `custom_js_runtime` a multi option (links-lang#1146)

Added the ability to link multiple custom js runtime files.

Co-authored-by: s1908422 <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>

wip

Allow 'default' as an argument to settings in the REPL (links-lang#1147)

Some settings has the value 'default'. Prior to this patch the value 'default' could not be written in the REPL, because it is token. This patch rectifies this problem by allowing the token 'default' to appear in settings argument position in the REPL.

Co-authored-by: Daniel Hillerström <[email protected]>

Allow record extension in presence of temporal projections (links-lang#1129)

* Allow record extension in presence of temporal projections

This allows record extension to play nicely with temporal projections.
I was forgetting that all arguments to reduce_artifacts are already
eta-expanded, so we can work with record literals.

Fixes links-lang#1124.

* Allow extension to work with (shallow) temporal projections

We don't have the full generality due to links-lang#1130, but this patch should
allow record extension to be used on temporal projections on variables /
record literals.

Removal of unused headless testing (links-lang#1152)

The headless testing has been unused for about a year, because it is unmaintained. Even though it is not used, it generates a ton of security warnings here on GitHub. I am not interested in dealing with those, therefore this patch removes the headless testing directory from the source tree.
@jamescheney
Copy link
Contributor Author

This issue seems to have been worked around by #1138. I think it should be closed, and another issue collecting ideas for a more principled design created.

Some of the examples in this page still encounter the "Error: Cannot call client side function '_$ClosureTable.apply' because of before server page is ready" problem also covered by #1134, but I think that is a separate problem - the examples there illustrate that the problem can occur with local server-annotated functions that are closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants