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

Add ExprSplitter #425

Merged
merged 3 commits into from Sep 7, 2020
Merged

Add ExprSplitter #425

merged 3 commits into from Sep 7, 2020

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 4, 2020

This implements an "expression-splitting iterator" that returns individual expressions ready for evaluation to the caller. This allows the caller to decide what to do with them (e.g., evaluation, signature analysis, whatever). It's basically a proposed replacement for prepare_thunk and split_expressions; by placing the consumer in charge, we get rid of any of the weird "should I eval if I can't lower?" questions that have dogged the previous framework.

For the moment this is parallel to our older framework; I've not yet actually rebased JuliaInterpreter on this, although the Frame(::Module, ::CodeInfo) constructor is a step in this direction. My local copy of Revise#revise3 is now based on this and it seems to work well.

@pfitzseb and @aviatesk, are there callers of these functions outside JuliaInterpreter, Debugger, and Revise? If so we'd want to think about a careful deprecation path. Indeed we might want to do that anyway since split_expressions, even though not exported, is pretty essential in some contexts (so there may be unknown users).

Closes #406
Closes #419

@timholy timholy marked this pull request as draft September 4, 2020 12:33
@pfitzseb
Copy link
Member

pfitzseb commented Sep 4, 2020

I'm pretty sure we're using split_expressions in VSCode and Juno, yes.

@KristofferC
Copy link
Member

Instead of working hard on deprecation and then just have everyone immediately update to the new one (effectively making the work on the deprecation pointless) we could also release a new breaking version and have people just update to that. Of course, this depends on how hard it is to do the deprecation and how hard it is to do the upgrade.

@timholy timholy changed the title RFC: add ExpressionSplitter Add ExprSplitter Sep 6, 2020
@timholy timholy marked this pull request as ready for review September 6, 2020 11:08
@timholy
Copy link
Member Author

timholy commented Sep 6, 2020

Since these are officially internal (though well-used) functions, I decided to go with the breaking change. The transition is pretty straightforward as can be seen by the tests. The main thing I'm unsure of is whether we want to export a utility for detecting and stripping out @doc exprs, or whether that was relevant only for Revise.

@test isa(JuliaInterpreter.prepare_thunk(Main, :(Base.BaseDocs.@kw_str "using")), Nothing)
# Some expressions can appear nontrivial but lower to nothing
# @test isa(Frame(Main, :(@static if ccall(:jl_get_UNAME, Any, ()) == :NoOS 1+1 end)), Nothing)
# @test isa(Frame(Main, :(Base.BaseDocs.@kw_str "using")), Nothing)
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't quite sure what to do about these.

@timholy timholy force-pushed the teh/splititer branch 2 times, most recently from b817367 to 41d398f Compare September 6, 2020 14:34
@timholy
Copy link
Member Author

timholy commented Sep 6, 2020

Ugh, this is ugly getting it over the finish line. The various bugs that different Julia versions have wrt LineNumberNodes are really annoying.

Are the remaining errors anything to worry about? Travis had a failure on Windows 1.4 (a strange string-indexing error), appveyor on 1.2 (a problem with stat). The second seems more worrisome, since it seems to have happened on both 32- and 64-bit. A few older PRs were not triggering this.

src/utils.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
@aviatesk
Copy link
Member

aviatesk commented Sep 6, 2020

Since these are officially internal (though well-used) functions, I decided to go with the breaking change. The transition is pretty straightforward as can be seen by the tests. The main thing I'm unsure of is whether we want to export a utility for detecting and stripping out @doc exprs, or whether that was relevant only for Revise.

as far as I know, the only consumer who uses the utility for at-doc exprs is only Revise, so I would like to say it's your call :)

@timholy
Copy link
Member Author

timholy commented Sep 6, 2020

Thanks! If Revise is the only one, I've already got that covered from within Revise. There's isn't much to it, really, just grabbing the 4th argument of the expression so it wouldn't be much of a utiliy anyway.

@KristofferC
Copy link
Member

Are the remaining errors anything to worry about? Travis had a failure on Windows 1.4 (a strange string-indexing error), appveyor on 1.2 (a problem with stat)

I would not really worry about unmaintained julia versions (1.1 - 1.4). Might be time to drop compat from them completely so they will keep working at the state they are in but not get updates.

@timholy
Copy link
Member Author

timholy commented Sep 6, 2020

Sounds good. Looks like JuliaLang/julia#26685. I just pushed a debug commit; let's see if that reveals an easy fix, and if not I'll just make 1.3 the lowest Julia version supported set the version compat to "~1.0, 1.5".

@timholy
Copy link
Member Author

timholy commented Sep 6, 2020

@codecov
Copy link

codecov bot commented Sep 6, 2020

Codecov Report

Merging #425 into master will increase coverage by 0.22%.
The diff coverage is 92.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
+ Coverage   88.73%   88.96%   +0.22%     
==========================================
  Files          12       12              
  Lines        2006     2256     +250     
==========================================
+ Hits         1780     2007     +227     
- Misses        226      249      +23     
Impacted Files Coverage Δ
src/packagedef.jl 88.57% <ø> (-8.10%) ⬇️
src/interpret.jl 85.88% <57.14%> (-3.85%) ⬇️
src/types.jl 75.71% <66.66%> (-0.90%) ⬇️
src/construct.jl 90.69% <98.55%> (-1.91%) ⬇️
src/precompile.jl 100.00% <100.00%> (ø)
src/utils.jl 87.46% <100.00%> (+3.82%) ⬆️
src/localmethtable.jl 96.49% <0.00%> (-3.51%) ⬇️
... and 7 more

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 64d719f...06ff99c. Read the comment docs.

@timholy
Copy link
Member Author

timholy commented Sep 7, 2020

A JuliaHub search indicates that the only known users of prepare_thunk and split_expressions are the ones we know about. So this crew (I should have also CCed @davidanthoff) can decide.

src/construct.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Member

That is one weird filename

Wow, where did that come from?

@timholy
Copy link
Member Author

timholy commented Sep 7, 2020

I am not sure. Seems likely to be a memory bug or something.

The main motivation is to eliminate their need to `eval`, which feels
like a code smell.
@timholy
Copy link
Member Author

timholy commented Sep 7, 2020

Revise@3.0.0 is officially ready to be announced, pending the resolution of this PR. 👍 or 👎 votes accepted, preferably with reasons for the latter.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

lgtm

@timholy
Copy link
Member Author

timholy commented Sep 7, 2020

Seems like folks are willing to do the work of updating for the breaking change. Many thanks, all!

@timholy
Copy link
Member Author

timholy commented Sep 7, 2020

Before merging I'm going to back out the commit disabling the ambiguity test. That means the test will fail, but we've at least seen the tests pass.

@timholy timholy merged commit aae7864 into master Sep 7, 2020
@timholy timholy deleted the teh/splititer branch September 7, 2020 21:31
aviatesk added a commit to JunoLab/Atom.jl that referenced this pull request Sep 8, 2020
aviatesk added a commit to julia-vscode/DebugAdapter.jl that referenced this pull request Sep 8, 2020
aviatesk added a commit to JunoLab/Atom.jl that referenced this pull request Sep 13, 2020
aviatesk added a commit to JunoLab/Atom.jl that referenced this pull request Sep 13, 2020
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.

None yet

4 participants