Pluto.jl icon indicating copy to clipboard operation
Pluto.jl copied to clipboard

Bad variable parsing in comprehensions for loops

Open damourChris opened this issue 1 year ago • 5 comments

Helloo!! I stumbled on a bug that seems to be caused with the new variable explorer (not really familiar with it). In the MRE, I show that a JS error arises in a cell while parsing variables in a cell that contains the combination of a comprehension for loop and and macro within that loop. Pretty much an edge case but it seems some notebooks might not be able to start and crash because of that. It only seems to affect notebook with a large starting time with the error causing the notebook to fail to launch (this is purely based on the single experience I had fixing the zombie attack notebook which is now running fine).

I found this bug from using the inspector and the built in browser debugger which told me that there was a problem in the update() function while the cursor type was a "ForStatement". Considering the notebook I was trying to fix only had a for loop with the @htl macro inside of it, I pruned it and found that it seems to be affected by any macro inside a comprehension for loop.

Perhaps the algo for finding variable dependencies has trouble dissecting the macro's internal? Not a clue

MRE

mre_bad_variable_parsing_comprehension_loops_macro

https://gist.github.com/damourChris/93d7d700530646d658f9aea50ee8adb9

Specs

  • Windows 11
  • Firefox 131.0.3
  • Julia 1.10.5
  • Pluto v.20.1

The Full Javacript error

Something went wrong while parsing variables... TypeError: bindings is undefined 
    explore_variable_usage scopestate_statefield.js:779
    explore_variable_usage scopestate_statefield.js:592
    explore_variable_usage scopestate_statefield.js:505
    explore_variable_usage scopestate_statefield.js:505
    update scopestate_statefield.js:1101
    update index.es.js:1825
    applyTransaction index.es.js:2661
    ensureAddr index.es.js:2061
    EditorState index.es.js:2597
    applyTransaction index.es.js:2661
    get state index.es.js:2311
    update index.es.js:15904
    dispatchTransactions index.es.js:15864
    dispatch index.es.js:15886
    applyDOMChange index.es.js:15161
    flush index.es.js:15652
    observer index.es.js:15346
    DOMObserver index.es.js:15329
    EditorView index.es.js:15873
    CellInput CellInput.js:563
    Preact 8
    setStatePromise Editor.js:361
    setStatePromise Editor.js:361
    new_task Editor.js:1138
    promise callback*update_notebook Editor.js:1086
    add_remote_cell_at Editor.js:553
    add_remote_cell Editor.js:568
    Cell Cell.js:383
    Preact 35
    setStatePromise Editor.js:361
    setStatePromise Editor.js:361
    new_task Editor.js:1138
scopestate_statefield.js:1107:21

Let me know if you have any other questions!

Happy debugging xx

damourChris avatar Oct 27 '24 08:10 damourChris

Hey! The error should be harmless and you can ignore it, but maybe you can share more info about the notebook that crashed?

fonsp avatar Oct 27 '24 14:10 fonsp

Thanks for the reply, that was also my initial thought.

I was trying to solve the issue with the zombie notebook!

Well it seems to be inconsistent, but the notebook wasn't crashing, it would just not start. All the processes under Status were not starting, which made me think that it was the JS that was causing the problem. After getting rid of that error, the notebook was starting and working fine haha.

I can't seem the replicate the issue on my machine so I'll try in isolated env as soon as I can.

damourChris avatar Oct 28 '24 16:10 damourChris

On another note, I just realised that it takes quite some time to start which is not really ideal. It maybe be worth splitting the notebook into a few parts maybe?

I have some extra time in December, I'd be happy to give it a rework, let me know what you think.

damourChris avatar Oct 28 '24 16:10 damourChris

Hey! I think it's a cool notebook, so worth the load time! If you have time in December then you are of course welcome to write a new notebook! Send me an email to discuss!

fonsp avatar Oct 28 '24 21:10 fonsp

MWE is:

r_v = rand(2)
[@show (index) for index in eachindex(r_v)]

fonsp avatar Jan 17 '25 11:01 fonsp