Implement sublime-syntax Build 4075 features#610
Conversation
Add support for the remaining sublime-syntax features from Sublime Text Build 4075, tracked in issue trishume#323: - `extends`: single syntax inheritance. Child syntaxes can extend a parent, inheriting its contexts and variables. The builder resolves extends chains before linking, merging contexts according to the child's merge mode and re-resolving regexes with merged variables. - `meta_prepend` / `meta_append`: when a child redefines a parent context, these directives insert child patterns before or after the parent patterns instead of replacing them outright. - `apply_prototype: true`: when including a context from another syntax, this flag causes the external syntax's prototype to also be included alongside the referenced context. - `version: 2`: parsed and propagated to SyntaxReference so the parser can apply version-conditional scope fixes. Two fixes implemented: * `set` no longer applies the old context's meta_content_scope to the matched text. * Multiple push with clear_scopes uses only the topmost context's value instead of summing them. Each MatchPattern now stores its original YAML regex string so that variable re-resolution after extends merging can re-apply the full regex pipeline (resolve variables, replace POSIX classes, newline handling). Regenerated default packdumps and updated the public API snapshot.
keith-hall
left a comment
There was a problem hiding this comment.
Wow, nice, thanks for taking this on - looks good to me!
We will for sure want to support this in a future PR, in order to support TSX etc. https://github.com/sublimehq/Packages/blob/master/JavaScript/TSX.sublime-syntax#L7-L9 |
|
the raw regex storage should put us closer to #541 which is nice funny seeing that the failure is a formatting difference with the MSRV. formatting could probably be dropped from that job given that MSRV is more of a "it compiles" thingy |
I still fixed it in eb4c554, seemed sensible. 🙂 |
Purely out of curiosity, I'd be interested to see the results when running with |
|
I just wanted to let you know that I'm doing this because 2 years ago I couldn't get syntax highlighting for a preview feature of Java that has been removed since (but that was somehow already supported by the Sublime syntax definitions). 😄 |
|
I've skimmed the code through and have no objections as long as I am not seen as responsible for maintaining the code over time :) If someone didn't already try, it would be good to test this implementation against a bunch of real-world syntaxes with bat that we couldn't use before because of the missing features. So we know it works for real, and not just with unit tests. If I were to review this I would prefer to split this into smaller PRs instead of implementing all features in one giant PR. But I don't really want to do a detailed review :) so if others are fine with the code and functionality that split is not needed as far as I'm personally concerned. |
|
One more thing: If we can implement this without changing the public API surface of syntect that would be even better, but I understand if that wouldn't work. |
I pointed the |
|
@Enselic Thanks for your feedback. To be clear, my hope here is to be helpful and from your message I'm under the impression that this contribution might be counterproductive. In the interest of making this contribution sustainable, I'd like to make sure there's alignment. I apologize in advance if I'm asking any dumb question, but I hope those ensure that this contribution is welcome:
I hope these questions don't come across the wrong way, all I hope is for this contribution to be genuinely helpful. 🙂 All I really want is for |
|
the SublimeHQ devs and most of the more active 3rd-party plugin devs are on Discord: in case you had questions with regard to the currently-not-yet-implemented stuff. |
|
Any plans to address #271? |
Not as part of this PR, but to the best of my understanding that is unrelated, right? |
|
This PR does not close #323, it only partially addresses it. Multiple inheritance might be rare, but I know for certain, I've got an open PR to the default packages for JSON where I use a list value for |
|
Would it be nice to have multiple inheritance? Yes. Should it block this? I say no. And the same for A huge percentage of extended syntaxes are just tweaks to a base language to fix interpolations when it's embedded. Holding that up for TSX and stale PRs for C and JSON seems like a bad trade-off. If the maintainers want the PR split or API stable, that's one thing. But dropping new functionality requirements is quite another. I wish I had the familiarity with syntect to review the code itself, but I don't. |
Fair point, I reworded the PR description. I'd appreciate the maintainers letting me know whether they'd rather merge as is, apply any change they see fit, split into smaller PRs, or get #323 fully implemented. Happy to act on any guidance. |
|
PR looks good from here. I (or any maintainer) can mark whatever is partially resolved through this PR in the list at the top of #323 as "done". |
|
I think it is fine without splitting into smaller PRs. I suspect it should be possible to de-duplicate some of the code, but as syntect hasn't seen much love recently, I think it is worth merging as it is. We can always iterate on it later. (Gatekeeping at this point seems like it would detract from the goal and demotivate contributors.) I was hoping to bump the Packages submodule to see how the syntax tests perform with the changes, but until we get branch point support, the failures would be too noisy. I unfortunately don't currently have time to test manually with syntaxes using v2 and inheritance etc. In either case, this PR doesn't break any existing functionality and it has unit tests, so I am inclined to merge. Edit: forgot to say, I'm pretty sure the public API changes are necessary and make sense 👍 |
|
Continuing the work in #612. |
CI on |
I saw that when pushing my changes for the follow-up with #612. Not sure of whether the problem is caused strictly by what has been as part of this PR, since this check passed on CI to the best of my understanding. In any case, #612 should take care of it, specifically a2d1823. |
|
After having a second look, I just realize it was indeed in this PR that that check started failing. Sorry for not fixing that before, if I had noticed that I would have pushed a patch. As mentioned though, I ended up fixing the issue in #612 to make sure I had a green branch. 🙂 |
Makes progress towards #323.
This adds support for the remaining
.sublime-syntaxfeatures introduced in Sublime Text Build 4075.hidden_file_extensionsandpop: Nwere already handled; this PR covers the rest.What changed
extends(single syntax inheritance)A child syntax can now declare
extends: Packages/Foo/Foo.sublime-syntaxto inherit all contexts and variables from a parent. The builder resolves extends chains (including transitive ones) in a fixed-point loop before the existing linking phase:meta_prependormeta_appendis used.maincontext but lacked__start/__main, the initial contexts are generated.Missing or circular parents produce a warning on stderr and are skipped.
meta_prepend/meta_appendWhen a child context uses
meta_prepend: true, its patterns are placed before the parent's patterns.meta_append: trueplaces them after. Parent meta scopes and clear_scopes are inherited when the child does not set them.apply_prototype: trueA new
Pattern::IncludeWithPrototypevariant. When iterating patterns, the external syntax's prototype is walked alongside the included context (respectingmeta_include_prototype).version: 2Parsed from YAML and stored on both
SyntaxDefinitionandSyntaxReference. The parser reads it at match time and applies two behavioral fixes inpush_meta_ops:meta_content_scopeis no longer applied to the matched text.clear_scopesis used instead of summing all of them.Raw regex storage
Each
MatchPatternnow carries an optionalraw_regex_str-- the regex as written in YAML before variable resolution. This is needed so that extends resolution can re-resolve parent regexes with overridden variables.Files touched
src/parsing/syntax_definition.rsextends,version,raw_regex_str,merge_mode), new enumContextMergeMode, new pattern variantIncludeWithPrototypesrc/parsing/yaml_load.rsextends,version,meta_prepend,meta_append,apply_prototype; store raw regex; re-resolution helperssrc/parsing/syntax_set.rsresolve_extendsphase inbuild();find_parent_indexhelper; updated linking/backref/prototype code for new pattern variantsrc/parsing/parser.rscurrent_syntax_versionhelper; version-conditional branches inpush_meta_opsassets/*.packdumpmake packs)tests/snapshots/public-api.txtNot included
extendsas a list) -- rare in practice; deferred.version: 2edge cases (embed_scope replacement, set+clear_scopes, embed escape meta scopes) -- they need more investigation into the exact Sublime behavior.Test plan
cargo test).--features metadata(129 pass) and--no-default-features --features regex-fancy(114 pass).