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

Allow Kinds to be registered by packages outside JuliaSyntax #461

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

c42f
Copy link
Member

@c42f c42f commented Jul 16, 2024

Extensible kinds are quite tricky. We want

  • To use a small number of bits for them
  • To have the string representation in the source, but have the compiler able to fully inline the integer representation.
  • Allow modules with different kinds to cooperate together on the same integer representation.
  • Not trigger invalidation when new kinds are added
  • Different Kind modules to not require cooperation

This is a very hard set of constraints to satisfy. The last one is already impossible in a single flat namespace so in this design we've given up on it and require cooperation between all kind extension modules, including module authors allocating non-colliding id's for their modules, in addition to non-colliding kind names.

Why do we need this? It's very convenient for JuliaLowering to extend the set of Kinds, rather than needing to clone/convert Kind just to add a few lowering-specific forms.

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 74.07407% with 14 lines in your changes missing coverage. Please review.

Project coverage is 95.69%. Comparing base (ae7d6ac) to head (ebea012).
Report is 6 commits behind head on main.

Files Patch % Lines
src/kinds.jl 74.07% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #461      +/-   ##
==========================================
- Coverage   96.43%   95.69%   -0.74%     
==========================================
  Files          14       13       -1     
  Lines        4208     3973     -235     
==========================================
- Hits         4058     3802     -256     
- Misses        150      171      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@c42f
Copy link
Member Author

c42f commented Jul 18, 2024

Ok codecov, you are partly right I forgot to cover some obscure error pathways for this obscure feature :-/
Unsure it's worth covering them but let's do it I guess.

Extensible kinds are quite tricky. We want
* To use a small number of bits for them
* To have the string representation in the source, but have the compiler
  able to fully inline the integer representation.
* Allow modules with different kinds to cooperate together on the same
  integer representation.
* Not trigger invalidation when new kinds are added
* Different `Kind` modules to not require cooperation

This is a very hard set of constraints to satisfy. The last one is
already impossible in a single flat namespace so in this design we've
given up on it and require cooperation between all kind extension
modules, including module authors allocating non-colliding id's for
their modules, in addition to non-colliding kind names.
@c42f c42f merged commit f99b76f into main Jul 18, 2024
36 checks passed
@c42f c42f deleted the caf/extensible-kinds branch July 18, 2024 05:40
@c42f
Copy link
Member Author

c42f commented Jul 18, 2024

Let's go with this. I don't love it. But I've thought a lot about a better solution and haven't found one so far.

@c42f c42f mentioned this pull request Jul 23, 2024
@c42f
Copy link
Member Author

c42f commented Jul 23, 2024

Chatted to @JeffBezanson about this - I think his opinion was roughly

hmm don't do this, I don't think we want extensible kinds. Oh no, especially not with __init__()

Sorry Jeff 😂 For what it's worth, I tried other options and they also were bad!

To explain a few things about how this implementation is one of the less terrible options:

  • "But just add the lowering kinds to JuliaSyntax.jl?". This is the easiest option and I tried this. But it's extremely inconvenient to have to edit JuliaSyntax to add new kinds for JuliaLowering: you're basically always on a JuliaSyntax.jl branch making CI hard downstream and introducing really tight coupling.
  • The implementation in this PR plays nice with precompilation and macro expansion in the sense that K"function" (for example) can be safely macro expanded to Kind(0x001f), and this also works with kinds from "Kind extension modules"
  • It's extremely convenient for JuliaSyntaxFormatter to have its own token kinds for whitespace. The alternatives aren't very satisfying and involve inventing an inefficient replacement for Kind for use in that package, such as Union{Kind,JuliaSyntaxFormatterKind}.

One thing which is currently rather bad about this implementation (in light of the very recent juliac work from JuliaCon) is that we do have significant code in __init__(). But I think we could remove nearly all of that and limit it to a single push! to a Vector{Any}. At least JuliaSyntax itself doesn't suffer any inefficiency and doesn't need to use __init__().

I'm not sure what the long term plan for this should be. For now I still think it's an improvement from the previous status quo and can be an internal API I guess.

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.

1 participant