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

Reject additional incomplete syntax definitions #1912

Closed

Conversation

Enselic
Copy link
Collaborator

@Enselic Enselic commented Oct 20, 2021

We currently ship two incomplete syntaxes by default, namely Svelte and Vue Component. This can make bat crash. See #915.

Since we don't want bat to crash, I think we should make sure to not ship any additional incomplete syntaxes. Here is a Draft PR on how we could ensure that.

We might want to make it opt-in (for CI) or opt-out (for users building custom assets), but before we tweak that I would like to hear what you think about the general idea.

I actually don't think it is a viable option to make syntect handle incomplete syntaxes. Even if syntect ignores missing syntaxes, what should it do instead? It seems to me as if the risk is high for follow-up problems if syntect tries to ignores problems like these.

@Enselic Enselic changed the title Disallow the addition of more incomplete syntax definitions Reject additional incomplete syntax definitions Oct 21, 2021
@sharkdp
Copy link
Owner

sharkdp commented Oct 22, 2021

Thank you very much!

I was somehow hoping that we could simply fix the existing "dependency holes" by adding 2-3 missing syntaxes? Or, alternatively, by patching the parent syntax to remove the embeds.

@Enselic
Copy link
Collaborator Author

Enselic commented Oct 23, 2021

I just realized that Sublime handles missing syntaxes just fine. It seems to fall back to "Plain Text" instead, which seems very reasonable.

So my current plan is to make syntect behave like that as well. So instead of crashing, plain text highlighting would be used instead. I think that should work.

Let me know if you can think of why that would might not work.

I will wait until the lazy-loading in syntect is in place though, because that touches the same code that would have to be modified to use Plain Text when references can not be resolved.

Closing this PR for now. I would be happy to keep discussing in this PR though if you want to.

@Enselic Enselic closed this Oct 23, 2021
@sharkdp
Copy link
Owner

sharkdp commented Oct 24, 2021

That does definitely sound like the better solution. Especially for cases where a user adds a custom syntax that embeds other (missing) syntaxes. The general warnings here are still useful though, I think. And I could even imagine for this to be a requirement on the builtin syntax definitions. Ideally, we would only ship fully-functioning syntax definitions. So this PR would still be useful in that case.

@Enselic
Copy link
Collaborator Author

Enselic commented Oct 24, 2021

Yep, I agree. I plan on resurrecting this PR once everything else has landed. Might take a while though, so would be good to keep this closed for now to not pollute the PR inbox.

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.

None yet

2 participants