Split SyntaxNode into TreeNode & SyntaxData

[timholy]

Closes #192

Pleasantly straightforward. Note that this object is slightly bigger since val has basically become two fields now, one for children and one for the actual value. I hope that's a tolerable increase.

I haven't timed anything to see if there are any regressions. (If constprop works, I think there won't be any.) Is there a task I should use?

[c42f]

This seems pretty reasonable. But I'm curious how much this helps you downstream? It looks like you don't get much functionality with TreeNode{SomeOtherNodeData} as the methods are still specialized to SyntaxNode here.

In terms of benchmarks, there's not really any formal benchmarks at this point. As a rough guide, converting all the way to Expr should be about 5x faster than the existing parser. You could try the following code before and after the changes? (I'll PR this to test/benchmark.jl shortly. I'm not sure what reliable infrastructure we have for benchmarking automatically...)

using BenchmarkTools
using JuliaSyntax

include("test_utils.jl")

function concat_base()
    basedir = joinpath(Sys.BINDIR, "..", "share", "julia", "base")
    io = IOBuffer()
    for f in find_source_in_path(basedir)
        write(io, read(f, String))
        println(io)
    end
    return String(take!(io))
end

all_base_code = concat_base()

b_ParseStream = @benchmark JuliaSyntax.parse!(JuliaSyntax.ParseStream(all_base_code), rule=:toplevel)
b_GreenNode   = @benchmark JuliaSyntax.parseall(JuliaSyntax.GreenNode, all_base_code)
b_SyntaxNode  = @benchmark JuliaSyntax.parseall(JuliaSyntax.SyntaxNode, all_base_code)
b_Expr        = @benchmark JuliaSyntax.parseall(Expr, all_base_code)

@info "Benchmarks" ParseStream=b_ParseStream GreenNode=b_GreenNode SyntaxNode=b_SyntaxNode Expr=b_Expr

[timholy]

But I'm curious how much this helps you downstream?

We might still need to broaden some other methods but I was planning to do that focally as I build out functionality. For now this is all I need, and it seems to clearly pave the way for elegantly handling both typed and untyped syntax trees.

We can wait to merge this if you prefer that I have JuliaDebug/Cthulhu.jl#345 re-implemented. The only thing I really need to know is whether this approach is acceptable, otherwise I'll quit on this approach and start working on a different one. But I'm guessing from your comment above is that you're fine with this, and might simply prefer to take it when there won't be a lot of further changes.

[timholy]

Thanks for the benchmarks; the last three are unchanged, but surprisingly there's a ~30% regression on the first one, which drops to ~15% if I add Base.@constprop :aggressive in front of getproperty. Personally I wouldn't worry about that, but you might, so I thought I'd better raise it.

[c42f]

there's a ~30% regression on the first one

That would be troubling, but it's very confusing because SyntaxNode shouldn't be involved in that benchmark!

[c42f]

I think you should go ahead and merge this if you think it's the right way forward.

It's surprisingly non-breaking ❤️

[c42f]

(I would like to understand whether the performance regression is real, it does bother me and it should be impossible. But I don't know if I have space in my head to dig deep into it right now.)

[timholy]

Before I merge:

• understand the performance regression
• check test coverage and see if it needs to be improved before it's safe to merge this refactor (the most recent commit has some val -> children changes that arguably should have caused test failures in previous iterations of this PR).

[timholy]

I'm not quite sure what was happening before, but today when I run the benchmarks there are no signs of trouble.

[codecov]

Codecov Report

Merging #193 (24843d3) into main (9fa5661) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
+ Coverage   94.86%   94.87%   +0.01%     
==========================================
  Files          15       15              
  Lines        3776     3789      +13     
==========================================
+ Hits         3582     3595      +13     
  Misses        194      194              

Impacted Files	Coverage Δ
src/syntax_tree.jl	95.10% <100.00%> (+0.37%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[c42f]

today when I run the benchmarks there are no signs of trouble

Cool this makes sense! The only thing I could think of was that perhaps the getproperty overloading was causing some weird inference limit to be hit somewhere in supposedly unrelated code. But that seemed like a long shot.