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

Live Projectors #1420

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from
Open

Live Projectors #1420

wants to merge 25 commits into from

Conversation

disconcision
Copy link
Member

@disconcision disconcision commented Nov 20, 2024

This give projectors access to dynamic info about their underlying term. In particular, it adds a new Probe projector which can be applied to any convex-tile expression term. When an expression is probed, live cells are added to the right of the row where the projector is located. Each live cell represents a closure resulting from that expression's evaluation. Each closure stores a value and and environment, where the environment is restricted to variable references which are locally free in the probed expression (ie occurring in the expression's co-context). Each closure additionally tracks a frame stem, a stack of environment ids which is used to associate closures drawn from the same (nested) function execution(s).

Current interaction model (TL;DR):

Try:

  • Pressing alt/option-v when any convex-tiled expression is indicated to add a probe
  • Clicking/hovering/double-clicking/shift-dragging on a live cell
  • Add multiple projectors across nested function literals and observe the effect of clicking on different live cells.

Current interaction model (detailed):

  • Use the projector panel or alt/option-v to apply a probe projector to any convex-tile expression (you'll have to parenthesize otherly-shaped things, same as with other projectors)
  • If the dynamics are still calculating, or if the expression is never executed, the probe icon will spin
  • If closures are found for that expression, the number of closures are indicated in the probe superscript
  • By default, live cells based on all gathered closures are shown; double-clicking on any cell restricts that probe to show only that one cell.
  • Hovering over a live cell shows the (co-contextual) environment (for expressions which are not variable references which contain variable references).
  • Clicking on a live cell sets a global (across-probe) frame cursor to that closure's frame stem. What this means is that an indication decoration (dotted outline) will be drawn on other cells from other probes that were gathered as part of the same function application (or nested application, for function literals contained inside other function literals). This is slightly complicated to explain, but hopefully evident from clicking around a bit.
  • Note that if some probes are set to show only one live cell (by double-clicking on a cell), which cell is shown may change as you select live cells in other probes. This is because the cell shown for each 'collapsed' probe is the first one compatible with the current frame cursor, i.e. the first one that would have a dotted outline around it as per above.
  • To expand/contract cell size (currently one size setting for each probe), hold shift and carefully drag the first cell of a probe left/right to progressively un/summarize the value. This is still finicky.
  • Adding two probes on the same line will currently result in their cells overlapping; you can manually add a linebreak to work around this
  • You can click on the in-line projector probe UI to zero out the frame cursor (if it's distracting)

Bugs (Critical):

  • i've broken all pattern projector splicing somehow
  • prevent use on patterns (via keyboard shortcut)
  • exercise mode your-impl doesn't get values from your-tests

Features (Critical):

  • make closure collection a projector API flag to prevent perf issues
  • adding a new probe on the same line as another removes the first
  • Improve abbreviate somewhat
  • cut off live cells after 20 or so, add nav arrows
  • per-cell resize states (should this be for a given closure or position in the cell list)

Bugs (Maybe wontfix for now):

  • shift-resize abbreviation is funky
  • shift-resize sometimes loses mouse capture

Features (Maybe wontfix for now):

  • probes on pattern variables
  • consider an annotation view option for projectors (put indicator over syntax instead of replacing it)

@disconcision disconcision marked this pull request as ready for review December 11, 2024 02:34
@disconcision
Copy link
Member Author

disconcision commented Dec 11, 2024

@Negabinary this is only tenuously out of draft but i'd appreciate a look-over when you get the chance, especially regarding the dynamics bits. Currently I'm doing some things a bit hackily and trying to decide how bad they actually are.

There are two dynamics things in particular I'm wondering about:

  1. Regarding exercise mode dynamics work. I was able to hook up to it in a basic sense, but I'm confused about dynamics across cells. Currently, probes in the correct implementation reflect values from the tests, but the user implementation probes don't reflect values from the user tests, only for executions in the user implementation editor itself. Changing the ExercisesMode piping so that the user implementation cell gets the user test dynamics also doesn't seem to make this work. (As a larger issue, it's not 100% clear to me what should get what. Also, I think I/we should consider, possibly as part of this PR, changing the way dynamics output works to make it slightly more in-line with statics. in particular, maybe dynamics should output a map, and there is just implictly a probe on the syntax root. i.e, the previous dynamics results is now 'just' an entry in the map. This clarifies my interfaces a bit but maybe fucks with stepper stuff?)
  2. Another thing I don't quite understand is that in the current implementation, i'm not actually really accumulating a stack trace... it seems to be more of a lexical equivalent. i.e. I would have expected the current implementation to, if i instrument a variable x inside a function bound to f, defined at the top level, which is called inside a function g, also defined at the top level, and then g is called at the top level, to have entries corresponding to both f and g for variable x. but it only has the immediate call. this doesn't actually immediately matter but I want to understand why my understanding of the code is wrong.

P.S. I did some dead code removal in ClosureEnvironment as I was finding the interface was drifting a bit

Base automatically changed from editor-output to dev December 11, 2024 19:46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FontMetrics!?? in core!!????

I'm sure it hurts you as much as it hurts me to see more and more files being slowly dragged into core. Maybe in the new year we can have a conversation about whether we can recover some semblance of modules (along with maybe renaming the 3 out of em).

Part of me wonders whether Editor.re/Perform.re/etc really belong in web, and if we do that whether we can get the core distinction back.

@@ -89,7 +89,7 @@ and exp_term =
| Test(exp_t)
| Filter(stepper_filter_kind_t, exp_t)
| Closure([@show.opaque] closure_environment_t, exp_t)
| Parens(exp_t) // (
| Parens(exp_t, Probe.tag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd vote for making an explicit Probe( ... ) form instead of using Parens. Parens to me feels like there should be no guarantee that they're preserved all the way through execution, and I can easily imagine someone writing a helper function that removes parens and creating unexpected bugs.

@@ -41,6 +41,9 @@ module EvaluatorEVMode: {
let update_test = (state, id, v) =>
state := EvaluatorState.add_test(state^, id, v);

let update_probe = (state, id, v) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like the right way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Measurements in the stepper appear to have gone awry
image

(This comment could be in the wrong place.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda surprises me that you've had to change Fun but not Fix, Let, or Case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems to have dissociated from line number? is this about the code at (new) line 343? if so, that's not the fun case, it's the fun case within app. If not then I'm not sure what this is regarding

@@ -49,6 +49,9 @@ let should_add_space = (s1, s2) =>
&& !Form.is_keyword(s1)
&& String.starts_with(s2, ~prefix="(") =>
false
| _ when String.ends_with(s1, ~suffix="…") =>
/* Hack case for probe projector abbreviations */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all hack cases 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this hack is a good argument for projectors being able to run their own MakeTerm.

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.

2 participants