922812 - Add a SLIME-style "compile-defun" command

Status: VERIFIED FIXED

(DevTools Graveyard :: Scratchpad, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

From my mailing list thread:

We could do some coolness like SLIME[0]'s compile-defun command, which
(translated into the JS world) compiles and evaluates all the code that
your cursor is at (or is selected, or whatever) up until the top level.
Take for example the following source

  function foo () {
    return 'foo'; // 1
  }

  function dodo() {
    return 5;
  }

If your cursor was on the line with the "// 1" comment, it would
re-evaluate the foo function, but not the dodo function.

However in this example:

  function bar() {
    function nested() {
      return 1; // 2
    }
    return nested();
  }

  function qux() {
    return void 0;
  }

If your cursor was on the line with the "// 2" comment, it would
re-evaluate the bar function (which includes re-evaluating the nested
function). The qux function is not re-evaluated.

It shouldn't be too hard to implement with the machinery we have in
place at this time.

Yes, I know you can do an equivalent thing right now by highlighting a
top level function by hand and doing an eval, but that is way too
difficult/manual to be useful. This should be its own keyboard shortcut
for ease of use, or else it is completely useless.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Basically, I think we just need to record the line/col of the cursor, and inside the Program AST node's list of statements, find the one with the range that includes the cursor, and evaluate only the code associated with that statement.

Comment 2

[Jim Blandy :jimb]

Totally. Emacs has eval-defun, which is essentially this.

Comment 3

[Jim Blandy :jimb]

Although... wouldn't it be better if we could sustain the illusion that the code was *always* live?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jim Blandy :jimb from comment #3)
> Although... wouldn't it be better if we could sustain the illusion that the
> code was *always* live?

Yes, but I think we can leave that for bug 771339

Comment 5

[Rob Campbell [:rc] (:robcee)]

One thing that I believe the selection + evaluate behavior gives the user is an explicit control over what's happening. There's feedback there as well.

If we implement something like eval-defun on a keystroke, I think it might be good UX to show the highlight before the eval takes place. Even if it's just a flash selection to indicate which block of code got re-evaluated.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add a Cmd/Ctrl-E keybinding for triggering compile-defun

This is the keybinding bit.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Briefly highlight the statement/function body when doing compile-defun

Here is a small patch that highlights the evaluated code per comment 5.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Briefly highlight the statement/function body when doing compile-defun v2

New version that doesn't touch the stock CM stylesheet.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip 1

doesn't highlight text (about to integrate panos' other patch)

only does FunctionDeclarations, not VariableDeclarations that happen to be functions.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Briefly highlight the statement/function body when doing compile-defun v3

Now with support for both light and dark themes.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip 2 - with highlighting

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-922812-eval-statement.patch

boom baby

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8334868 [details] [diff] [review]
bug-922812-eval-statement.patch

Review of attachment 8334868 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/scratchpad/scratchpad.js
@@ +36,5 @@
>  const promise   = require("sdk/core/promise");
>  const Telemetry = require("devtools/shared/telemetry");
>  const Editor    = require("devtools/sourceeditor/editor");
>  const TargetFactory = require("devtools/framework/target").TargetFactory;
> +const acornLoose = require("acorn_loose");

Leftover!

::: browser/devtools/scratchpad/test/browser_scratchpad_eval_func.js
@@ +11,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
> +    openScratchpad(runTests);
> +  }, true);
> +
> +  content.location = "data:text/html;charset=utf8,test Scratchpad pretty print.";

s/pretty print/evaluate current function/

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/e7e1fdf75d8c

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7e1fdf75d8c

Comment 16

[Ioana (away)]

Tested with Firefox 28 beta 1 on Mac OS X 10.8.5 and Windows 7 32bit. When pressing Ctrl/Cmd+E
* if there are no errors in the code, the right function is highlighted.
* if there are errors in the code, nothing gets highlighted, and the error comment is displayed although the error was in a function that was not supposed to get evaluated (e.g. another function lower on the page and that nothing to do with the one selected for evaluation).

Please let me know if this behavior is by design and if there is anything else that I'm supposed to see happen here.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Ioana Budnar, QA [:ioana] from comment #16)
> Tested with Firefox 28 beta 1 on Mac OS X 10.8.5 and Windows 7 32bit. When
> pressing Ctrl/Cmd+E
> * if there are no errors in the code, the right function is highlighted.
> * if there are errors in the code, nothing gets highlighted, and the error
> comment is displayed although the error was in a function that was not
> supposed to get evaluated (e.g. another function lower on the page and that
> nothing to do with the one selected for evaluation).
> 
> Please let me know if this behavior is by design and if there is anything
> else that I'm supposed to see happen here.

Assuming by "errors in the code" you meant compile-time syntax errors, then yes that is expected because Reflect.jsm (the parser we are using) does not have an error tolerant parser; the whole source must be valid for anything to parse.

If by "errors in the code" you meant some kind of run-time error, then that is unexpected.

Comment 18

[Ioana (away)]

(In reply to Nick Fitzgerald [:fitzgen] [Ðoge:D6jomNp59N9TVfgc538HU3RswwmwZwcrd7] from comment #17)
> Assuming by "errors in the code" you meant compile-time syntax errors, then
> yes that is expected because Reflect.jsm (the parser we are using) does not
> have an error tolerant parser; the whole source must be valid for anything
> to parse.
> 
> If by "errors in the code" you meant some kind of run-time error, then that
> is unexpected.

Yes, I did mean compile-time syntax errors. It works fine with runtime errors for me.