Skip to content

Conversation

@ericphanson
Copy link
Member

@ericphanson ericphanson commented Jul 17, 2025

ExplicitImports needs to be loaded in the same Julia session as the user package being analyzed, because it does a dynamic analysis via introspection, not purely a static analysis.

This presents some issues, because:

  • the compatibility requirements of ExplicitImports itself can clash with the package being analyzed
  • folks often put ExplicitImports in their global dev env, but other dev tools sometimes have different compat requirements. This has already come up 3 times, with Cthulhu on 1.11 and global env issues here and here

I have tried to avoid too many deps in ExplicitImports to reduce this issue, but we still have Compat, AbstractTrees, JuliaSyntax, Markdown, Pkg, PrecompileTools, and TOML. Of these, all issues so far have been with JuliaSyntax. So here I propose vendoring JuliaSyntax.

I've also introduced a comment syntax based on JuliaFormatter's to disable ExplicitImports, so we can tell it to not recurse into vendored code.

I setup a script vendor/run.jl to make it easy to update versions, and added a .gitattributes to mark the code as vendored:

git check-attr -a vendor/JuliaSyntax/src/literal_parsing.jl
vendor/JuliaSyntax/src/literal_parsing.jl: linguist-vendored: set

git check-attr -a vendor/run.jl # no results, i.e. correctly not-vendered

If we like this approach we could also vendor more deps, e.g. AbstractTrees.

This is a different approach than VSCodeServer's which uses submodules since I don't know that those will work well with Pkg. That package is distributed by the language server not Pkg so it's a different scenario. My approach (literally copying the src files) should be simple and robust.

Notes:

  • the vendored dep is obviously fully locked down; we don’t get patches “for free” like when Pkg manages it. If we want a patch we need to update it and release our own patch release
  • We don’t share functions/types with the “real” package. For us this is basically neutral/good I think, we don’t need that.
  • not sure we’d want to vendor Pkg, I think we do want the one appropriate to the running Julia version bc I think there can be some coupling there. There’s also globals and hooks and extensions and things in Pkg that seem more complicated
  • likewise PrecompileTools can be tied to the running Julia version, so I don't think we should vendor it
  • Besides those, the other packages, even the stdlibs, seem like they could be vendored the same way if we wanted.

@ericphanson ericphanson changed the title Vendor JuliaSyntax to avoid compatibility issues RFC: Vendor JuliaSyntax to avoid compatibility issues Jul 17, 2025
@ericphanson ericphanson requested a review from fredrikekre July 17, 2025 15:34
@ericphanson ericphanson changed the base branch from main to eph/drop-pre-110 July 17, 2025 16:12
@ericphanson
Copy link
Member Author

ericphanson commented Jul 17, 2025

This is now based on #122 since I'm using the module dispatch syntax for extensible ignoring of modules. That PR also removes Compat as a dependency so we should have fewer compat conflicts (though Compat.jl hasn't had a breaking change in a while so it's not as likely anyway).

@@ -0,0 +1 @@
vendor/*/** linguist-vendored
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 think this might only "take effect" once the PR is merged, but I believe it means github will mark diffs to the vendored code as uninteresting during PR review in the future

@ericphanson ericphanson mentioned this pull request Jul 17, 2025
@palday
Copy link
Contributor

palday commented Jul 18, 2025

  • the vendored dep is obviously fully locked down; we don’t get patches “for free” like when Pkg manages it. If we want a patch we need to update it and release our own patch release

I think this is worth it.

  • We don’t share functions/types with the “real” package. For us this is basically neutral/good I think, we don’t need that.

Fully agree. And since this is mostly used at test/development time and not at user execution time, I don't think the (rather minimal) overhead is any real concern.

  • not sure we’d want to vendor Pkg, I think we do want the one appropriate to the running Julia version bc I think there can be some coupling there. There’s also globals and hooks and extensions and things in Pkg that seem more complicated

I would not want to vendor Pkg; that seems like an utter nightmare. Pkg is also much more stable.

  • likewise PrecompileTools can be tied to the running Julia version, so I don't think we should vendor it

ibid

  • Besides those, the other packages, even the stdlibs, seem like they could be vendored the same way if we wanted.

Yes, but I don't think it's necessary. I think JuliaSyntax is a little bit special here because it's just started to see uptake across the ecosystem and has had a major breaking release (0.4 -> 1.0) in the very recent past that hasn't been adopted by all the packages using it. The public interface for PrecompileTools and the stdlibs seems to be much more stable.

(Commented more than I usually would since you marked this as RFC 😄 ❤️ )

Base automatically changed from eph/drop-pre-110 to main July 19, 2025 15:48
@ericphanson ericphanson mentioned this pull request Jul 19, 2025
@ericphanson ericphanson merged commit b6c1746 into main Jul 19, 2025
6 checks passed
@ericphanson ericphanson deleted the eph/vendor-js branch July 19, 2025 16:01
"""
ignore_submodules(::ModuleDispatcher{T}) where {T} = ()

ignore_submodules(::ModuleDispatcher{ExplicitImports}) = (Vendored,)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of dispatching on module (ExplicitImports), could we instead dispatch on symbols? This would allow us to keep support for older Julia versions.

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.

3 participants