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

update sublimehq/Packages and update tests accordingly #535

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jalil-salame
Copy link

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
Copy link
Collaborator

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
Copy link
Author

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 jalil-salame force-pushed the update-sublime-packages branch 2 times, most recently from dcd6f23 to d27a140 Compare April 28, 2024 16:56
@jalil-salame
Copy link
Author

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

@jalil-salame
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

The linking is done when building the syntax set:

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

@jalil-salame
Copy link
Author

The linking is done when building the syntax set:

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
Copy link
Author

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

@jalil-salame
Copy link
Author

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

@jalil-salame
Copy link
Author

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
Copy link

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

@stefanobaghino
Copy link

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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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.

The new syntax files cause new test to fail (and C# to pass). This
should be looked over more carefully and fixed in the future.
@jalil-salame
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

keith-hall commented May 12, 2024

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
Copy link
Author

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:

@michalvankodev
Copy link

Is there anything I can do to help this move forward? The current syntax references are 7 years old. I'd also like to use typescript without any difficult installation steps.

@jalil-salame
Copy link
Author

You probably want to look at branch point support and extends support. I don't have the time to work on this anymore so feel free to take over all of my work

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.

4 participants