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.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I'm encountering interesting corner cases with bindings.
Especially with the continues case.
Consider:
Source A:
x = 5
y = 6
z = 7
File B with continues set to [A]:
t = x + y
Since this feature is mostly for Observable, it would be weird to render all of x, y, z, t in a block for B.
That's how it works on the current develop branch (bindings were always flattened and extended, there's no difference between the local and global scope).
We could force Observable users to provide the final expression to avoid this, but it won't be very convenient.
On my overhaul branch (#1119), bindings are represented as a stack of scopes.
Keeping bindings as a stack is technically not necessary (it's a side effect of using mutable bindings, which I rolled back after that benchmark message above). But it solves issues with having to subtract the stdlib from bindings and simplifies a few more things, and it also helps with this problem.
So on the overhaul branch, final bindings for B are represented as: { t: 11 } / { x: 5, y: 6, z: 7, ...rest of stdlib } (that / separator separates scopes in the stack in my current toString implementation). And then if you ask project->getBindings("B"), it returns just the local scope.
Accidentally, it fixes the Observable issue.
Interestingly, it creates a new one: chaining doesn't work anymore. If B continues A and C continues B, then C won't have access to vars from A.
I'm not sure how big of a problem that is. We'll need N^2 elements in continues arrays, and N^2 is bad; but evaluation of each block still happens once so it's tolerable.
Another possible solution would be to split the getBindings API to getBindings (full flattened dictionary with all bindings) and getLocals (only local definitions in the given source). Then we could use getLocals for rendering and getBindings for continues chains.
I think I prefer Rescript way; which libraries the module used for its implementation is its own private business and they shouldn't be exposed in its exports.
For now I'm going to fix/comment out the broken tests and won't add getLocals yet, to finish the branch faster, since we don't use neither continues nor explicit includes anywhere yet.
But we need to figure out the optimal semantics for this.
[later after discussion with Umur on how bindings on long continues chains should be chained]
[...] naive implementation would be O(chain size)-costly.
On Observable notebook with 100 cells the last cell would take 100-checks-deep performance hit for any stdlib lookup (stdlib bindings will be at the bottom of the chain, naturally).
So chains should be relatively shallow, either flat or maybe 2-levels deep (locals -> flattened imported bindings -> stdlib). linkDependencies still should merge maps. But in the solution I propose with getBindings/getLocals it would rely on getBindings, while rendering would be based on getLocals.
Clarification: 2 levels deep at the start of a module.
Each module can contain more nested scopes due to lambdas and inner blocks.
It would be possible to keep bindings flat during the module evaluation, since bindings are immutable, and recusing into evaluating the inner block doesn't affect the outer bindings, so it's easy to keep around a snapshot (not sure how clear this explanation is, it might be hard to understand without reading the code, sorry).
But I'm leaning towards having the code that creates extra chains for nested blocks, anyway, because:
we might need it for other future features (e.g. for debugging, or maybe for my "blocks as records" proposal)
we might prefer to make bindings mutable again if we do SSA.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
-
(extracting from Slack for posterity)
I'm encountering interesting corner cases with bindings.
Especially with the
continues
case.Consider:
Source A:
File
B
with continues set to[A]
:Since this feature is mostly for Observable, it would be weird to render all of
x
,y
,z
,t
in a block forB
.That's how it works on the current
develop
branch (bindings were always flattened and extended, there's no difference between the local and global scope).We could force Observable users to provide the final expression to avoid this, but it won't be very convenient.
On my overhaul branch (#1119), bindings are represented as a stack of scopes.
Keeping bindings as a stack is technically not necessary (it's a side effect of using mutable bindings, which I rolled back after that benchmark message above). But it solves issues with having to subtract the stdlib from bindings and simplifies a few more things, and it also helps with this problem.
So on the overhaul branch, final bindings for
B
are represented as:{ t: 11 } / { x: 5, y: 6, z: 7, ...rest of stdlib }
(that / separator separates scopes in the stack in my current toString implementation). And then if you askproject->getBindings("B")
, it returns just the local scope.Accidentally, it fixes the Observable issue.
Interestingly, it creates a new one: chaining doesn't work anymore. If
B
continuesA
andC
continuesB
, thenC
won't have access to vars fromA
.I'm not sure how big of a problem that is. We'll need N^2 elements in continues arrays, and N^2 is bad; but evaluation of each block still happens once so it's tolerable.
Another possible solution would be to split the
getBindings
API togetBindings
(full flattened dictionary with all bindings) and getLocals (only local definitions in the given source). Then we could usegetLocals
for rendering andgetBindings
for continues chains.Interestingly, semantics for similar cases differ in different languages.
For example, Python inject all imports into the current namespace and reexports them implicitly (unless you set
__all__
manually).But Rescript doesn't: https://rescript-lang.org/try?code=LYewJgrgNgpgBADTgXjgbwFBzrALnAQxTgFYMBfDDUSWOATWM2[…]VGtHgAtJjzacGPPHADGxQQHo1cXAAsAlgGc4BuCADWCvsqFwNWvYeMcQuCkA
I think I prefer Rescript way; which libraries the module used for its implementation is its own private business and they shouldn't be exposed in its exports.
For now I'm going to fix/comment out the broken tests and won't add
getLocals
yet, to finish the branch faster, since we don't use neither continues nor explicit includes anywhere yet.But we need to figure out the optimal semantics for this.
[later after discussion with Umur on how bindings on long continues chains should be chained]
[...] naive implementation would be O(chain size)-costly.
On Observable notebook with 100 cells the last cell would take 100-checks-deep performance hit for any stdlib lookup (stdlib bindings will be at the bottom of the chain, naturally).
So chains should be relatively shallow, either flat or maybe 2-levels deep (locals -> flattened imported bindings -> stdlib).
linkDependencies
still should merge maps. But in the solution I propose withgetBindings
/getLocals
it would rely ongetBindings
, while rendering would be based ongetLocals
.Clarification: 2 levels deep at the start of a module.
Each module can contain more nested scopes due to lambdas and inner blocks.
It would be possible to keep bindings flat during the module evaluation, since bindings are immutable, and recusing into evaluating the inner block doesn't affect the outer bindings, so it's easy to keep around a snapshot (not sure how clear this explanation is, it might be hard to understand without reading the code, sorry).
But I'm leaning towards having the code that creates extra chains for nested blocks, anyway, because:
Beta Was this translation helpful? Give feedback.
All reactions