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

Replace Swift grammar #6603

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Conversation

jtbandes
Copy link
Contributor

@jtbandes jtbandes commented Nov 5, 2023

👋 Hi! I'm the maintainer of the Swift language grammar at textmate/swift.tmbundle. Recently, I've migrated development of the grammar over to a new repo under my account at jtbandes/swift-tmlanguage.

This PR replaces the submodule with one pointing at the new repo. (I had some difficulties using the devcontainer on macOS arm64, so I ended up making some of the changes manually.)

I've recently implemented some much-needed updates to the grammar, including support for new language features like regular expression literals and macros. I would like to test out how these look with GitHub's syntax highlighting, but I'm not aware of any way to test other than waiting for a new Linguist release and then making changes if needed for the following release. Please let me know if there's a better way!

Checklist:

@jtbandes jtbandes requested a review from a team as a code owner November 5, 2023 09:06
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

Note: this PR will not be merged until close to when the next release is made. See here for more details.

@jtbandes
Copy link
Contributor Author

jtbandes commented Nov 9, 2023

Thanks! Do you have any tips on how I could test this before the release happens?

@lildude
Copy link
Member

lildude commented Nov 9, 2023

We used to have a tool, but it was withdrawn a while ago. The closest we've got now is @Nixinova's tool at https://novalightshow.netlify.app/ I'm not sure how update-to-date it is, but it should give you a good idea.

@jtbandes
Copy link
Contributor Author

jtbandes commented Nov 9, 2023

Great, thanks for the tip. Results look promising :)

image

@lildude lildude added this pull request to the merge queue Dec 5, 2023
Merged via the queue into github-linguist:master with commit f4c35bf Dec 5, 2023
5 checks passed
@jtbandes jtbandes deleted the update-swift branch December 5, 2023 17:30
@lildude
Copy link
Member

lildude commented Dec 12, 2023

Our grammar compiler seems to have missed that there are errors in this regex which our syntax highlighter didn't.

It looks like several of those / need to be escaped if https://regex101.com/r/Q7kxmc/1 is right. Note, we use PCRE in the syntax highlighter for performance reasons.

I need to make a new release today so I'm going to have to revert this. Sorry @jtbandes

lildude added a commit that referenced this pull request Dec 12, 2023
@lildude
Copy link
Member

lildude commented Dec 12, 2023

NM. A bit more digging revealed its a bug in the version of pcre used by the syntax highlighter which differs from the one used by the compiler. Bringing the two inline resolves the issue.

@jtbandes
Copy link
Contributor Author

Thanks! Let me know if any other issues. Happy to make tweaks if necessary to get it working well.

@lildude lildude mentioned this pull request Dec 12, 2023
3 tasks
@jtbandes
Copy link
Contributor Author

See also: a small bug fix which would be awesome to include if possible: #6627 (comment)

@lildude
Copy link
Member

lildude commented Dec 13, 2023

It's too late now. The release has been made. This will be pulled in with the next release.

@jtbandes
Copy link
Contributor Author

I've noticed some new errors with GitHub's highlighting now: jtbandes/swift-tmlanguage#7 I'll try and look into this later, but just wondering if you think this might be related to the PCRE version update you mentioned? The results are different to what I see on https://novalightshow.netlify.app:

GitHub NovaLightshow
image

@lildude
Copy link
Member

lildude commented Dec 13, 2023

It'll be indirectly related in that the two use different regex libraries. GitHub uses PCRE (the upgraded library should improve things). I believe that site uses JavaScript's regex or Oniguruma (Ruby's default and what VS Code used) which aren't 💯 compatible.

@jtbandes
Copy link
Contributor Author

Are there any instructions on compiling and running grammars locally so I can test it? Would hate to have to guess at the required changes and wait months to see whether the fixes work or not 😅

@lildude
Copy link
Member

lildude commented Dec 13, 2023

Unfortunately not as the highlighter code is not public. The compiler shipped with linguist is more of a basic PCRE validator and converts the grammars to JSON. You can see the results of that by running script/build-grammars-tarball and extracting the tarball to find your file.

I tend to use regex101.com to check regexes so that could help validate the regexes you think are misbehaving.

@Alhadis is also a whizz at grammars and regexes so might have some time to offer some tips.

@Alhadis
Copy link
Collaborator

Alhadis commented Dec 22, 2023

@lildude Something's changed with GitHub's handling of TextMate-powered highlighting. There's now a runaway-match issue with my .gitconfig grammar, and I distinctly recall seeing no issues with it on GitHub until relatively recently. Compare GitHub's rendering of the file to that of Atom's:

GitHub screenshot Atom screenshot
GitHubAtom

This doesn't appear to be an issue with regex engine flavours, or how recent they are. I can see @jtbandes's example is affected by the same highlighting failing to terminate at the end pattern specified by the TextMate grammar.

Also, @lildude: I e-mailed you recently with a follow-up about PrettyLights. I don't want to twist this issue into an excuse to expedite the process, but… well… 😉

@lildude
Copy link
Member

lildude commented Dec 22, 2023

I wonder if this has something to do with the issue I encountered when trying to update the highlighting engine with the new grammar. I'll bring it up with the maintainers when I'm back at work in the new year.

I'll chase them for access to the prettylights code too. Sorry this has taken so long @Alhadis

@jtbandes
Copy link
Contributor Author

In case it helps you can access my example code at https://github.com/jtbandes/swift-tmlanguage/blob/9d90fa826213245388560b8336c6624f8cae3c08/grammar-test.swift#L432. Thanks for looking into it!

@lildude
Copy link
Member

lildude commented Dec 22, 2023

I'll bring it up with the maintainers when I'm back at work in the new year.

I've opened an issue with them for follow up next year so I don't forget after a bit too much festive cheer 😁

@jtbandes
Copy link
Contributor Author

FWIW I'm also noticing some issues with other languages, for example TOML: (I can't vouch for whether this issue is new or not, but it sure looks similar to the other ones reported so far)

https://github.com/rust-lang/cargo/blob/master/Cargo.toml

image

@lildude
Copy link
Member

lildude commented Jan 2, 2024

I've opened #6668 to track the highlighting issues in one place

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants