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

Fix CSTParser dep #148

Closed
wants to merge 1 commit into from
Closed

Fix CSTParser dep #148

wants to merge 1 commit into from

Conversation

MasonProtter
Copy link
Contributor

The project is currently set to "~3.3.0", but this unnecessarily restricts CSTParser to a version that's broken in julia v1.11.

Unless there's a very good reason to do so, the preferred thing is to trust that package authors are following semantic versioning, and won't release a v3.4 that breaks existing APIs. In this case, 3.4 adjusts to accommodate the fact that the 3.3 was broken on julia v1.11.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Mar 2, 2024

Hm, actually no this does not appear to solve the problem in v1.11. I'm still getting

julia> JuliaSnail.start(10051) ; # please wait, time-to-first-plot...
MethodError(convert, (Tokenize.Lexers.Lexer{Base.GenericIOBuffer{Vector{UInt8}}, Tokenize.Tokens.RawToken}, Tokenize.Lexers.Lexer{IOBuffer, Tokenize.Tokens.RawToken} at position: 0), 0x000000000000675b)
MethodError(convert, (Tokenize.Lexers.Lexer{Base.GenericIOBuffer{Vector{UInt8}}, Tokenize.Tokens.RawToken}, Tokenize.Lexers.Lexer{IOBuffer, Tokenize.Tokens.RawToken} at position: 0), 0x000000000000675b)
ERROR: type Array has no field span
Stacktrace:
 [1] getproperty
   @ ./Base.jl:49 [inlined]
 [2] pathat(cst::Vector{Any}, offset::Int64)
   @ Main.JuliaSnail.CST ~/.emacs.d/straight/repos/julia-snail/JuliaSnail.jl:511
 [3] moduleat(encodedbuf::String, byteloc::Int64)
   @ Main.JuliaSnail.CST ~/.emacs.d/straight/repos/julia-snail/JuliaSnail.jl:556
 [4] forcecompile()
   @ Main.JuliaSnail.CST ~/.emacs.d/straight/repos/julia-snail/JuliaSnail.jl:727
 [5] start(port::Int64; addr::String)
   @ Main.JuliaSnail ~/.emacs.d/straight/repos/julia-snail/JuliaSnail.jl:885
 [6] start(port::Int64)
   @ Main.JuliaSnail ~/.emacs.d/straight/repos/julia-snail/JuliaSnail.jl:857
 [7] top-level scope
   @ REPL[1]:1

where the offending function is

function pathat(cst, offset, pos = 0, path = [(expr=cst, start=1, stop=cst.span+1)])
   if cst !== nothing && !CSTParser.isnonstdid(cst)
      for a in cst
         if pos < offset <= (pos + a.span)
            return pathat(a, offset, pos, [path; [(expr=a, start=pos+1, stop=pos+a.span+1)]])
         end
         # jump forward by fullspan since we need to skip over a's trailing whitespace
         pos += a.fullspan
      end
   elseif (pos < offset <= (pos + cst.fullspan))
      return [path; [(expr=cst, start=pos+1, stop=offset+1)]]
   end
   return path
end

So it appears that something is causing for a in cst to give a::Array...

@gcv
Copy link
Owner

gcv commented Mar 2, 2024

CSTParser has a long-standing history of breaking its APIs, and its maintainers have not even bothered to acknowledge my request for something as simple as a changelog (julia-vscode/CSTParser.jl#240). I’ll try to fix this breakage soon, but I’m very short on time. Also not sure if newer versions of CSTParser work with LTS versions of Julia, which I’d like Snail to support.

This might be a good time to eliminate Snail’s dependency on CSTParser and switch to tree-sitter.

@MasonProtter
Copy link
Contributor Author

Ah, I see, that's unfortunate. Yeah, best to just drop CSTParser then. Tree-sitter would be a good option, though the writer of julia-ts-mode has apparently switched to neovim and won't be working on julia-ts-mode, and it doesn't seem that a new maintainer has been found yet: JuliaEditorSupport/julia-emacs#205, so that's maybe also not the best option.

Is JuliaSyntax.jl unable to give the info you need from CSTParser.jl?

@gcv
Copy link
Owner

gcv commented Mar 2, 2024

I haven't looked at JuliaSyntax.jl in detail, but I imagine it should work, at least for Julia 1.10 and newer. Still, I'd rather retain support for 1.6 for as long as it's an LTS release. This may be a good way to go, but:

I'm pretty sure Snail can use tree-sitter without julia-ts-mode. It just needs tree-sitter itself and the Julia grammar (https://github.com/tree-sitter/tree-sitter-julia). tree-sitter is now standard in Emacs 29 (and I hope can be installed in 27 and 28 from an ELPA repository). Installing the grammar is mildly annoying, but I hope I can either document it sufficiently or automate it. Once it's available, all the questions Snail asks of CSTParser can be answered without leaving Elisp. This will be a huge benefit, not only by eliminating an unstable dependency, but also because Julia startup time will improve.

It'll be great to ditch all Julia-side dependencies for core Snail functionality and leave them to extensions.

@gcv
Copy link
Owner

gcv commented Mar 3, 2024

I think the fix for Julia 1.11 compatibility has not yet been merged into CSTParser: julia-vscode/CSTParser.jl#385

Once that goes in, I'll bump the CSTParser version dependency in Snail. I don't really trust CSTParser to not break on minor version numbers, though, so plan to keep to the "tilde" version specifier string.

It's still valuable to switch tree-sitter, though. #149

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Mar 4, 2024

I haven't looked at JuliaSyntax.jl in detail, but I imagine it should work, at least for Julia 1.10 and newer. Still, I'd rather retain support for 1.6 for as long as it's an LTS release. This may be a good way to go, but:

JuliaSyntax.jl works fine on LTS.

❯ julia +1.6 -q
julia> using JuliaSyntax

julia> parsestmt(SyntaxNode, "(x + y)*z", filename="foo.jl")
line:col│ tree                                   │ file_name
   1:1  │[call-i]                                │foo.jl
   1:2  │  [call-i]
   1:2  │    x
   1:4+
   1:6  │    y
   1:8*
   1:9  │  z

julia> VERSION
v"1.6.7"

But more generally, JuliaSnail should maybe be a julia package on the general registry so that users can install whatever specific version of the package / deps work with their julia installation.

@gcv
Copy link
Owner

gcv commented Mar 5, 2024

But more generally, JuliaSnail should maybe be a julia package on the general registry so that users can install whatever specific version of the package / deps work with their julia installation.

This complicates keeping the Emacs and Julia sides in sync. Right now, mutual compatibility of the Julia and Elisp sides of an julia-snail installation is guaranteed because JuliaSnail.jl is part of the Emacs package. But you're right that the current situation leaves much to be desired. It's yet another reason to switch to tree-sitter and have JuliaSnail.jl have no Julia-side dependencies.

Anyway: it looks like CSTParser 3.4.2 fixes this problem, and I pushed a changed Project.toml. Please let me know if it works.

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.

2 participants