-
Notifications
You must be signed in to change notification settings - Fork 23
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
Reducer dev #226
Reducer dev #226
Conversation
format only reducer reformat lint multi-line test spelling multi-line semantic mapping todo multi-line eval multi-line tests todo change context to bindings simplify tests rename exception test methods bindings is an expression value make bindings callable reformat Emphasize the nature of Lisp AST Initial definition of macros make functions private fixed functionNode type casting macro call skeleton sort ReducerInterface fix test macros skeleton bindings is not a value assignment semantics let semantics defined format reformat reformat TODO function calls and list hd variables are confused reformat tmp works reformat reformat add test reformat add test
✅ Deploy Preview for squiggle-documentation canceled.
|
❌ Deploy Preview for squiggle-components failed.
|
packages/squiggle-lang/src/rescript/Reducer/Reducer_Dispatch/Reducer_Dispatch_BuiltIn.res
Show resolved
Hide resolved
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.res
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some preliminary comments, may come back for a more substantial pass.
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.res
Show resolved
Hide resolved
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.res
Outdated
Show resolved
Hide resolved
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression_T.res
Show resolved
Hide resolved
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression_T.res
Show resolved
Hide resolved
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.res
Show resolved
Hide resolved
This is not par with old squiggle, I think? we allow the evaluation of expressions in a non-bottom-of-file spot. It's not an important feature, but we should think twice about removing it. @OAGr |
packages/squiggle-lang/__tests__/Reducer/Reducer_MathJs/Reducer_MathJsParse_test.res
Outdated
Show resolved
Hide resolved
packages/squiggle-lang/src/rescript/Reducer/Reducer_MathJs/Reducer_MathJs_ToExpression.res
Show resolved
Hide resolved
I think I don't care too much about this case. However, this might break the functionality we currently have of showing each item that's rendered, no matter what the output is. However, my guess is that that's not going to work with the Reducer set up, as is, anyway, so it doesn't matter much. (I'd think) |
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.res
Show resolved
Hide resolved
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.res
Show resolved
Hide resolved
It is assignment; ... assignment; expr. Because assignments return bindings as a result. If you simply insert an expression then that expression's result passes up as bindings. Oops. To simulate statements in the middle, one has to convert them to "fake" assignments. Your feature is possible. But I need to count the array index and prepend a "_ = " assignment to every statement if they are not the last one. Could be handled as a feature. However, I don't see a use for it in a functional language. There are other ways of creating side effects in the middle. (to be discussed). |
{1; 2} => {_ = 1; 2} //feasible feature |
Additional note. The philosophy of reducer is connecting a parser with the reducer using "functional semantics". Not implementing it per grammar. The semantic definition assignments is really a function that return bindings. But eventually any grammar is possible. Of course as long as it is a functional language. Not functionalish. Semantic definitions are how you define the meaning of the parsed grammar.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to see a couple of critical features that might require you to give up on the concept of lisp like purity within the reducer:
- I think it's critical that calls to evaluate also return the bindings that were created during that expression. This is because I want to share these variables between different executions. I think evalWBindings will be the main part of the API. But to get the bindings object from previous executions it needs to return the bindings at some point
- It should be possible to return nothing in an execution and give only variables, or return multiple things in an execution. Both of these features would prove useful in practice.
Other than that. I really can't wait to see this in! I'm super excited about this work. Thank you!
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.res
Outdated
Show resolved
Hide resolved
Ok. I am adding this as a feature to do. eval will return just an answer but evalWBindings will return the bindings as well |
@umuro I think it's fine for us to punt these to later, they don't have to be part of this PR. (Especially given that they weren't part of the previous version anyway) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be cool to see this merged, and punt returning the environment down the line. Would love to see that somewhat soon though considering it's the last step needed to get Squiggle notebooks
umuro#58 Added as a feature. I will provide bindings as a return from evalWBindings. |
A) Return multiple things. Just build a record or array We have records. Instead of relying on internal structures of resolver. Don't create a script with invisible assumptions. Instead of hunting variables in the bindings, why don't you write Moreover, we can add the semantic rule that returning nothing is auto constructing a record of variables and returnint it :) |
Instead of hunting variables in the bindings, why don't you write |
packages/squiggle-lang/src/rescript/Reducer/Reducer_Expression/Reducer_Expression.resi
Too many changes. Hot target. Preventing development
squiggle
Hey! Sorry about the review earlier, I realised that I probably didn't give enough context and it could have seemed overly harsh. I'm very new to coding with other people. I'd love to see this merged soon, the bindings thing can be done later. I'll put more context as to why bindings is important in #234. But basically the reason I put that review in is because I looked into the future a tad (by merging this on my local machine), saw variables working, but only needed the environment to be returned to recreate a Squiggle Notebook. Because I thought it was a minor change (although I might be wrong), and is to me the most important feature (because it gets us back to the Squiggle Notebook milestone) I thought it would be nice to include it here. |
However, that can easily be another story |
Hey @umuro , here's your patch for the JS tests to pass diff --git a/packages/squiggle-lang/src/js/index.ts b/packages/squiggle-lang/src/js/index.ts
index 1a53aa6..4b8cdda 100644
--- a/packages/squiggle-lang/src/js/index.ts
+++ b/packages/squiggle-lang/src/js/index.ts
@@ -88,6 +88,7 @@ function tag<a, b>(x: a, y: b): tagged<a, b> {
export type squiggleExpression =
| tagged<"symbol", string>
| tagged<"string", string>
+ | tagged<"call", string>
| tagged<"array", squiggleExpression[]>
| tagged<"boolean", boolean>
| tagged<"distribution", Distribution> |
// Multiple statements and variables
describe("assignment", () => {
testEvalToBe("x=1; x", "Ok(1)")
testEvalToBe("x=1+1; x+1", "Ok(3)")
testEvalToBe("x=1; y=x+1; y+1", "Ok(3)")
testEvalToBe("1; x=1", "Error(Assignment expected)")
testEvalToBe("1; 1", "Error(Assignment expected)") //No position info from MathJs parse...
testEvalToBe("x=1; x=1", "Error(Expression expected)")
})