Enable JuliaSyntax.jl as the default Julia parser - #46372

Can we drop the implied Mmap usage and dependency? Doing mmap leads to segfaults and bus errors, where a read would have just gotten a permission error or file truncation error, and can lead to other problems like that, with cleanup and continued edits by the user on the original file.

It is really cool to see how small this is! And exciting!

Can we drop the implied Mmap usage and dependency

Yes :-) Already done on a WIP branch of mine, will be merged soon.

BTW, for anyone wondering what's happening here - all the development is happening over at JuliaSyntax.jl which is why this PR is just sitting around at the moment. I'll get back to this PR once I've sorted out a few remaining large usability problems (Particularly generating Expr(:incomplete) for REPL completions. Which isn't hard in itself, but has proven to be a bit of a rabbit hole leading to other things.)

Now that version 0.2 of JuliaSyntax has been released, can this PR be updated to use it? We have 2 weeks left if we want to merge this in the 1.9 timeframe.

only enabled if the JULIA_USE_NEW_PARSER=true environment variable is set.

Can it be merged, and with it on by default? Would it be good for PkgEval, or should it alternatively be run with:

$ JULIA_USE_NEW_PARSER=true julia

I'm not necessarily thinking it should be in Julia 1.9-rc1, it just think it might be good to have it from now or the feature freeze (up to then) to check, and not have to explain the ENV to users (and you will likely not get feedback from master users with a non-default option), though this should also be a possibility:

$ JULIA_USE_NEW_PARSER=false julia

We have 2 weeks left if we want to merge this in the 1.9 timeframe.

Yep I've got julia-1.9 in mind for this. Will probably do a JuliaSyntax-0.3 release prior to that.

Can it be merged, and with it on by default

I don't think we're ready to turn this on by default. PkgEval would be good though...

PkgEval would be good though...

PkgEval currently doesn't configuring environment flags, so to test this either add a commit that enables the new parser by default, or add a Make flag that results in the new parser being used by default (which can then be set using the configuration keyword to Nanosoldier's runtests, resulting in a Make.user entry).

Tried running this under PkgEval with the new parser enabled by default (using the latest JuliaSyntax.jl, not the one from this PR), but Julia fails to bootstrap:

error during bootstrap:
LoadError("sysimg.jl", 19, LoadError("/source/usr/share/julia/stdlib/v1.9/JuliaSyntax/src/JuliaSyntax.jl", 1, LoadError("/source/usr/share/julia/stdlib/v1.9/JuliaSyntax/src/parser_api.jl", 137, LoadError("/source/usr/share/julia/stdlib/v1.9/JuliaSyntax/src/parser_api.jl", 137, UndefRefError()))))
getproperty at ./Base.jl:37 [inlined]
getindex at ./refvalue.jl:56 [inlined]
docm at ./docs/Docs.jl:521
unknown function (ip: 0x7f90895bcc8d)
_jl_invoke at /source/src/gf.c:2447 [inlined]
ijl_apply_generic at /source/src/gf.c:2629
jl_apply at /source/src/julia.h:1866 [inlined]
do_apply at /source/src/builtins.c:730
@doc at ./boot.jl:534

I'll probably add an environment option to PkgEval then.

@nanosoldier runtests(ALL, vs = "%self", configuration = (buildflags=["DEPS_GIT=JuliaSyntax"], environment=["JULIA_USE_NEW_PARSER=true"]))

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

Packages that seem to occur a lot in failure logs: Macrotools, Unitful, and CpuId.

Unitful, AbstractAlgebra, and OrdinaryDiffeq all have failures that I have filed issues about.

In total, 8352 packages were tested, out of which 3596 succeeded, 3615 failed and 1141 were skipped.

Testing took 5 hours, 3 minutes, 31 seconds (or, sequentially, 20 days, 6 hours, 50 minutes, 52 seconds to execute 16704 package tests suites).

This seems rather bad, but comparing to some random other PkgEval, about half failing isn't unusual (for all kinds of reasons including syntax):

#46372 (comment)

I'm amazed by how long it takes (and not scalable, with package ecosystem increasing), but is the timing maybe (only slightly?) better here?

This one has timing: #45646 (comment)

I'm amazed by how long it takes (and not scalable, with package ecosystem increasing), but is the timing maybe (only slightly?) better here?

The timings cannot be compared, as quite some packages now fail to precompile and thus never execute any test.

It is honestly kind of silly to use PkgEval to test this, since we could just test that everything parses the same between the 2 versions (which would allow us to skip loading the code and running the tests).

The CpuId failure is due to something like commas after new lines:

julia> [1
       , 2]
ERROR: ParseError:
Error: Expected `]`
@ REPL[33]:2:1
[1
, 2]

However, that parses fine if you use the current main branch of JuliaSyntax.jl

yeah. this pr is still using 0.1 but should be using 0.2

It is honestly kind of silly to use PkgEval to test this, since we could just test that everything parses the same between the 2 versions (which would allow us to skip loading the code and running the tests)

Using existing infrastructure to quickly get a result instead of having to write something tailor made that wouldn't have gotten any better results in the end seems to me like an efficient use of resources, not something silly.

fair :). I'm coming from the perspective that we'll probably have to run this between 10 and 100 times, at which point the balance might shift.

especially since the custom version could give more information per run since pkgeval doesn't give us errors for packages that have a failing dependency and does give us spurious errors for the thousand or so packages with bad CI

@gbaraldi said that error has been seen in other PRs, so it doesn't seem to be specific to this one.

Well, I've seen it in #50144, which is a PR that does mess with the GC, but I do think we near the limit when compiling the sysimg at 32 bit.

#50205 it's broken also on master.

@giordano thanks, what a relief. I also tried to repro the LLVM out of memory on the i686 windows build locally with cygwin, and could not.

I think this is good to go. I'll do one last nanosoldier run and have a look through those results before merging.

@nanosoldier runtests()

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

Thank you so much @c42f for all the work to make this a reality!

Ah. I must admit I was really really looking forward to pushing the merge button on this myself. It kind of felt symbolic given this was like 1.5 years of work. (Not if it meant missing feature freeze, but that's tomorrow morning my time IIUC?)

I was just preparing a final minor bugfix based on the latest nanosolider run. I'll make a separate PR for that.

Thanks for the enthusiasm anyway!

I think there may be some elements, such as the method of merging, that would be better handled by @c42f . If you feel it's preferrable, we wouldn't mind if the PR were to be temporarily reverted and reland it quickly with minor fixes, what are your thoughts? When it comes to major PRs like this one, it often seems best if the person who created the PR decides matters such as whether to squash the commits. Regarding the feature freeze, it's set to be extended until the 27th of June, so there's no need to get hurry just yet:)

At this point I'd rather not revert this - that will just cause extra churn. I think it'll be fine just to put a separate PR in for any minor fixes because this was very, very close and the remaining bug I know about doesn't seem very disruptive.

Luckily I'd already rearranged the commits into logical order and done a bunch of squashing so that side of it was in pretty good shape.