update sublimehq/Packages and update tests accordingly

[jalil-salame]

I was looking into issues with newer sublime syntax packages so I updated the testdata/Packages submodule to the last working version.

This required some small tweaks to the parser tests.

The next commit (40ec1f2f) fails to pass the tests due to a failure to parse the YAML:

--- STDERR:              syntect parsing::parser::tests::can_parse_yaml ---
thread 'parsing::parser::tests::can_parse_yaml' panicked at src/parsing/parser.rs:792:67:
called `Result::unwrap()` on an `Err` value: ParseSyntax(MissingMandatoryKey("match"), "testdata/Packages/JavaScript/JSX.sublime-syntax")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

As far as I can tell, this happens due to not handling include: context-name properly. I would like to also fix the issue with 40ec1f2f, but I don't know where to start.

How I found the bad commit

If you want to reproduce the search, what I did was:

#!/bin/sh
# /tmp/good.sh

cd ~/Dev/syntect/ || exit 255
cargo nextest run --release -E 'not test(public_api)' # public_api always fails due to serde

$ cd testdata/Packages
$ git bisect start && git bisect good && git bisect bad master && git bisect run sh /tmp/good.sh

Then fix the parser tests until I got to 40ec1f2f.

Additional thoughts

This would've been easier with insta, as I could've set INSTA_UPDATE=always in good.sh to have the snapshot tests be automatically updated (ignored), and thus found the bad commit slightly quicker. If this sounds desirable, then I would gladly switch snapshots from assert_eq! to assert_debug_snapshot.

[keith-hall]

As far as I can tell, this happens due to not handling include: context-name properly.

This requires support for extends - see #323

[jalil-salame]

As far as I can tell, this happens due to not handling include: context-name properly.

This requires support for extends - see #323

I'll check the CI failures later today or tomorrow, and take the chance to look at #323 a bit closer too!

[jalil-salame]

I was not building with -F metadata, I fixed all tests that were failing because of that.

[jalil-salame]

I'm looking at how to implement extends (in a different PR). My initial idea is to add an error for ExtendNotFound when loading a syntax that extends another one, then (if inside a syntax set) query the set for the missing syntax, and parse it first, then merge with the original syntax.

Do you have any input on that?

[jalil-salame]

The test failure is due to serde auto detecting nightly and trying to enable unstable features.

We could update to a newer nightly to fix the issues, but really, serde should just stop doing that 😠

[keith-hall]

I'm looking at how to implement extends (in a different PR). My initial idea is to add an error for ExtendNotFound when loading a syntax that extends another one, then (if inside a syntax set) query the set for the missing syntax, and parse it first, then merge with the original syntax.

Do you have any input on that?

Sounds reasonable, but I'm not sure about the case where the "extended" syntax (i.e. syntax with extends) is loaded first, followed by the "base" syntax (syntax referenced by extends) - I'm not sure we can rely on load order...
I can see a potential need for a "pre-parse" step when adding a folder of syntax definitions, which would just build a tree of which syntaxes are extending others and compute the order in which to parse them from that...

[keith-hall]

I guess that is why there is a separate linking step at the moment - maybe it would be possible to do the work there?

[jalil-salame]

I guess that is why there is a separate linking step at the moment - maybe it would be possible to do the work there?

Could you point me to the linking step? I am currently hacking everything together on top of the SyntaxSetBuilder::add_from_folder function

[keith-hall]

The linking is done when building the syntax set:

syntect/src/parsing/syntax_set.rs

Line 658 in de715e5

Self::link_context(context, syntax_index, &all_context_ids, &syntaxes);

[jalil-salame]

The linking is done when building the syntax set:

syntect/src/parsing/syntax_set.rs

Line 658 in de715e5

Self::link_context(context, syntax_index, &all_context_ids, &syntaxes);

What I would need to do then, is delay loading syntaxes when I find an extends keyword, and load them during linking? Sounds reasonable?

[jalil-salame]

Rebased on top of master, this pulls in fixes for the nightly tests. Please re-run the CI

[jalil-salame]

I'll look at the failures this afternoon, thanks for running the CI c:

[jalil-salame]

I don't understand how the tests work, specifically, how make assets works, what is failing and how I go about fixing it. I figured out I could update the known failures. I can push that to see if CI passes then, but I would be introducing more failures to syntect then...

[stefanobaghino]

make assets succeeds locally for me, I'm not sure of what the problem with CI is... 🤔

[stefanobaghino]

Oh, I missed that the actually failure is on the following command

cargo run --release --example syntest -- testdata/Packages testdata/Packages --summary | diff -U 1000000 testdata/known_syntest_failures.txt -

I'm able to reproduce locally. I'll have a look and see if I can find out something...

[stefanobaghino]

Does it make sense to change the known failure files? If so, I've done it in jalil-salame#1. Although this seems relatively sensible within the context of this change (please do correct me if I'm wrong), I suspect it might not be desirable to be merged on master and might need to be worked on as a single piece of work in #536 to make the new failures go away. Thoughts?

[jalil-salame]

Does it make sense to change the known failure files? If so, I've done it in jalil-salame#1. Although this seems relatively sensible within the context of this change (please do correct me if I'm wrong), I suspect it might not be desirable to be merged on master and might need to be worked on as a single piece of work in #536 to make the new failures go away. Thoughts?

I think the files need to be updated, just like the tests, but I don't know how to do that (or if it should be done at all). I think the syntax definitions changed (which is why it is failing), but I need some input from someone who knows.

[stefanobaghino]

I believe if you merge jalil-salame#1, the tests will pass. But I, too, could use some guidance with regard to whether this is acceptable. Maybe @keith-hall knows more? 🙂

[keith-hall]

It could be acceptable, but first I think it's worth taking some time to identify what those failures are, and whether they are simply due to existing known bugs. Upgrading the syntaxes and having them highlight more wrongly than before updating them would kind of defeat the point... 😅

I'll try to take a look into it myself at some point in the near future, but I don't get much free time these days unfortunately

[jalil-salame]

It could be acceptable, but first I think it's worth taking some time to identify what those failures are, and whether they are simply due to existing known bugs. Upgrading the syntaxes and having them highlight more wrongly than before updating them would kind of defeat the point... 😅

I'll try to take a look into it myself at some point in the near future, but I don't get much free time these days unfortunately

Some scope names have changed so it might be because of that, but I don't know how to check what the errors are (just what grammars failed).

[keith-hall]

Some scope names have changed so it might be because of that

it is running the syntax tests which are alongside the syntax definitions, which would also cover scope name changes - these tests pass directly in Sublime Text, for example.

One can manually run the syntest example against a specific syntax test file - without the --summary flag, it should show exactly which tests are failing.

[jalil-salame]

After giving it another look, I think the right thing to do is to update the known-failures; we don't make any code changes in this PR, so any new failures are strictly because of deficiencies in syntect which we can address separately.

I'll probably even give it a shot later c:

I updated the files, could you rerun the CI @keith-hall ? Thanks

[jalil-salame]

One of the HTML errors is probably due to a change in the theme file (html::tests::strings; base-16-ocean.dark has changed).

Whereas the other html::tests::tokens is probably due to failures in the Markdown parsing (Markdown has been added to known failures).

What do you think we should do @keith-hall?

The first case seems pretty uncontroversial to just update, the second one though is a bit more complicated:

1. Create an issue about the Markdown file and mark the test as skip or known-failure.
2. Delete the test outright.
3. Drop the PR (or at least try to rollback the packages to a better supported version).

I don't like any of the options. I hope someone has a better idea.

[keith-hall]

Markdown is failing due to lack of branch point support in syntect: sublimehq/Packages@c08f853
Likely almost no Markdown content would be scoped correctly, I'm not sure we'd want to merge until it is fixed. Unfortunately it's a much bigger change than supporting extends...
Rolling back to a better supported version could be an option, however.

[jalil-salame]

Markdown is failing due to lack of branch point support in syntect: sublimehq/Packages@c08f853 Likely almost no Markdown content would be scoped correctly, I'm not sure we'd want to merge until it is fixed. Unfortunately it's a much bigger change than supporting extends... Rolling back to a better supported version could be an option, however.

I'll look for the last working commit then, and take a look at the issue you linked c: