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

lang: Split FuncValue into FuncValue and SimpleFn #696

Closed

Conversation

gelisam
Copy link
Contributor

@gelisam gelisam commented Jan 15, 2023

Representing an MCL function value as a golang function from Value to Value was a mistake, it should be a function from Vertex to Vertex.

Why this is a mistake

The output of a function like

$f = fn(x) {
  Shell(Sprintf("seq %d", x))
}

varies over time, while a single Value does not. Thus, code like

Map($f, list(1, 2))

would first produce the value list("1", "1"), but then it would not update to list("1", "2") when "seq 2" produces its second line. That's because with the mistaken design, when Map receives a new FuncValue or a new input list of N elements, Map calls the function from Value to Value N times and produces a single output list of N elements.

Why the corrected design is better

Here's what happens with this new design when Map receives a new FuncValue or a new input list of N elements.

First, Map constructs N item-input nodes, each of which extracts a different entry from the list. Then, Map calls the function from Vertex to Vertex N times, once for each item-input node, and thus obtain N item-output nodes. Finally, Map constructs an item-collecting node which constructs a list out of all of its inputs, and Map connects the N item-output nodes to the item-collecting node. This item-collecting node is the output of Map.

The Vertex to Vertex function constructs and connects its own nodes; in this case, it constructs an Sprintf node and connects the item-input node to it, and then constructs a Shell node and connects the Sprintf node to it, and then returns the Shell node as the item-output node.

The two Shell node in this sub-graph emit a first value "1", which propagates to the item-collecting node and causes it to output a first value list("1", "1"). Then, the second Shell node emits a second value "2", which propagates to the item-collecting node and causes it to output a second value list("1", "2"), as desired.

How this commit brings us closer to the above plan

Changing FuncValue throughout the codebase is a big change. One of the difficulties is that it is not just nodes which are emitting FuncValues, there are also many other places in the code where FuncValue is used to hold a golang function from Value to Value. Some of those places now need to hold a golang function from Vertex to Vertex, but other places still need to hold a golang function from Vertex to Vertex.

Thus, as a first step, we need to split FuncValue into two types.
This commit splits the old FuncValue into two types:

  1. The new FuncValue will hold a function from Vertex to Vertex. FuncValue is a Value.
  2. A new type named "SimpleFn" will hold a function from Value to Value. SimpleFn is not a Value.

This commit replaces occurrences of the old FuncValue with one of those two new types, as appropriate. This commit does not yet adapt the surrounding code to make use of the new representation; that will be done in future commits. I have annotated the missing parts with the following panic message in order to make it easy to find which parts still need to be implemented. The "..." part explains what needs to be implemented.

panic("TODO [SimpleFn]: ...");

Where I need help

One part of the code which is not clear to me are the parts which use reflection. I don't understand the purpose of that code well enough to explain what needs to be implemented. I have annotated those "known unknown" parts of the remaining work with the following panic message in order to make it easy to find which parts still need more thinking and planning:

panic("TODO [SimpleFn] [Reflect]: ...");

@gelisam
Copy link
Contributor Author

gelisam commented Jan 15, 2023

Since I have added a ton of panic calls, I do expect the tests to fail on CI, but I did not expect to see a type error! The error is in a file named lang/funcs/core/generated_funcs.go which I do not have on my machine, so I assume there's some code-generation going on somewhere which now needs to use SimpleFn instead of FuncValue in the generated code?

@gelisam
Copy link
Contributor Author

gelisam commented Jan 15, 2023

Fixed generated_funcs.go.

@gelisam
Copy link
Contributor Author

gelisam commented Jan 15, 2023

Reflowed comments.

Representing an MCL function value as a golang function from Value to
Value was a mistake, it should be a function from Vertex to Vertex.

Here is why this is a mistake:

    The output of a function like

        $f = fn(x) {
          Shell(Sprintf("seq %d", x))
        }

    varies over time, while a single Value does not. Thus, code like

        Map($f, list(1, 2))

    would first produce the value list("1", "1"), but then it would
    _not_ update to list("1", "2") when "seq 2" produces its second
    line. That's because with the mistaken design, when Map receives a
    new FuncValue or a new input list of N elements, Map calls the
    function from Value to Value N times and produces a single output
    list of N elements.

Here is why the corrected design is better:

    Here's what happens with this new design when Map receives a new
    FuncValue or a new input list of N elements.

    First, Map constructs N item-input nodes, each of which extracts a
    different entry from the list. Then, Map calls the function from
    Vertex to Vertex N times, once for each item-input node, and thus
    obtain N item-output nodes. Finally, Map constructs an
    item-collecting node which constructs a list out of all of its
    inputs, and Map connects the N item-output nodes to the
    item-collecting node. This item-collecting node is the output of
    Map.

    The Vertex to Vertex function constructs and connects its own nodes;
    in this case, it constructs an Sprintf node and connects the
    item-input node to it, and then constructs a Shell node and connects
    the Sprintf node to it, and then returns the Shell node as the
    item-output node.

    The two Shell node in this sub-graph emit a first value "1", which
    propagates to the item-collecting node and causes it to output a
    first value list("1", "1"). Then, the second Shell node emits a
    second value "2", which propagates to the item-collecting node and
    causes it to output a second value list("1", "2"), as desired.

Here is how this commit brings us closer to the above plan:

    Changing FuncValue throughout the codebase is a big change. One of
    the difficulties is that it is not just nodes which are emitting
    FuncValues, there are also many other places in the code where
    FuncValue is used to hold a golang function from Value to Value.
    Some of those places now need to hold a golang function from Vertex
    to Vertex, but other places still need to hold a golang function
    from Vertex to Vertex.

    Thus, as a first step, we need to split FuncValue into two types.
    This commit splits the old FuncValue into two types:

    1. The new FuncValue will hold a function from Vertex to Vertex.
       FuncValue is a Value.
    2. A new type named "SimpleFn" will hold a function from Value to
       Value. SimpleFn is not a Value.

    This commit replaces occurrences of the old FuncValue with one of
    those two new types, as appropriate. This commit does not yet adapt
    the surrounding code to make use of the new representation; that
    will be done in future commits. I have annotated the missing parts
    with the following panic message in order to make it easy to find
    which parts still need to be implemented. The "..." part explains
    what needs to be implemented.

        panic("TODO [SimpleFn]: ...");

Here's where I need help:

    One part of the code which is not clear to me are the parts which
    use reflection. I don't understand the purpose of that code well
    enough to explain what needs to be implemented. I have annotated
    those "known unknown" parts of the remaining work with the following
    panic message in order to make it easy to find which parts still
    need more thinking and planning:

        panic("TODO [SimpleFn] [Reflect]: ...");
@gelisam
Copy link
Contributor Author

gelisam commented Jan 15, 2023

The static checks are now passing in CI! Of course, CI is now failing because the tests are failing, understandably so because of the panic calls, but that's intended.

@gelisam
Copy link
Contributor Author

gelisam commented Mar 9, 2023

This PR has now been incorporated into #704. Closing.

@gelisam gelisam closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant