Support incremental evaluation in split_expressions

[timholy]

timholy/Revise.jl#475 involves a couple of issues. With timholy/Revise.jl#497 on the way, the big obstacle is how Revise organizes its information: it first parses a file, splits the expressions out into modules, and then inserts them into a Dict as keys. The actual evaluation from includet (currently) iterates over the Dict keys.

The problems with this design are that (1) when an expression is repeated exactly, you still only get one key/value pair in the Dict, and (2) order information is lost (other than the fact that these are SortedDicts) in terms of the order in which expressions in different modules should be executed. In a way it's kind of amazing that the old system worked as well as it did.

To fix this properly, it seems that the best strategy is to evaluate as you go, mimicking include. To me it seemed that the best place to put this is split_expressions, so now there's a new keyword eval::Bool. I also took the occasion to clean up how :toplevel expressions are handled and be more expansive in saving LineNumberNodes, thinking that given issues like timholy/Revise.jl#437 and similar that we'd rather not risk losing location information.

Technically, changing the output of split_expressions! this way is a breaking change, so perhaps we should go to version 0.8. How do folks feel about that? There's always a certain amount of Pkg-churn when we make incompatible releases, but for this package I don't think it's too bad.

[timholy]

Fascinating. Turns a @test_broken into a @test, though this was not my intention.

[KristofferC]

Technically, changing the output of split_expressions! this way is a breaking change, so perhaps we should go to version 0.8. How do folks feel about that?

It looks like the only packages using it are Atom and Revise. So unless it breaks existing versions of Atom / Revise I would personally just make it a patch release.

[timholy]

I hadn't checked explicitly (good idea, thanks), but sure enough it's badly breaking for Revise.

[codecov]

Codecov Report

Merging #406 into master will decrease coverage by 0.23%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
- Coverage   87.70%   87.47%   -0.24%     
==========================================
  Files          11       11              
  Lines        2009     2099      +90     
==========================================
+ Hits         1762     1836      +74     
- Misses        247      263      +16     

Impacted Files	Coverage Δ
src/breakpoints.jl	93.90% <66.66%> (ø)
src/utils.jl	83.85% <76.66%> (-0.95%)	⬇️
src/precompile.jl	93.39% <92.30%> (-6.61%)	⬇️
src/construct.jl	88.40% <100.00%> (-0.69%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 602248b...2cb90dd. Read the comment docs.

[timholy]

I'm more interested in doing this right than rushing it, so I plan to leave this open until @KristofferC has had a moment to catch his breath and think about it. (I am sure your task list is long and includes big-ticket items like, "get Julia 1.5 out" which I strongly support.)

When there's time to think about it, here are a few more thoughts:

• there are other ways Revise could fix includet removes equal code in same module scope timholy/Revise.jl#475. For instance, includet could include the file and then parse it for signatures retrospectively like it does with packages. That involves parsing the file twice but that's not typically the dominant contribution to the total time. If we really dislike the idea of going to 0.8, this is probably the best option.
• I think the strongest argument to change things is probably the view that

  JuliaInterpreter.jl/src/construct.jl

  Lines 472 to 479 in 480983e

  	if isempty(lex.args) || (isexpr(ex, :global) && all(item->isa(item, LineNumberNode), lex.args))
  	push!(modexs, (mod, copy(ex)))
  	else
  	push!(lex.args, ex)
  	push!(modexs, (mod, copy(lex)))
  	empty!(lex.args)
  	end
  	end

  is a pretty bad hack. It's designed so that

  JuliaInterpreter.jl/src/construct.jl

  Line 365 in 480983e

  eval && Core.eval(mod, thunk)

  won't throw an error, but I'm not sure it's comprehensive or complete. The new must_be_toplevel here is an attempt to do better, but while at it I decided to try to have split_expressions itself not have to make such decisions, and just preserve whatever setting the original expression has.
• there are quite a few other things here; the controversial commit is ecca5e9. If it's easier to review, I'd be happy to split the rest out into a separate PR.
• RFC: Revise 3.0, rewrite based on LoweredCodeUtils.CodeEdges timholy/Revise.jl#497 currently needs this PR but could be reworked to go without it, as needed.

[timholy]

I'm reconsidering this (and it may take a while), so for the time being don't merge.