Add Tree-sitter-based syntax-highlighting support for Dart#58480
Conversation
|
We require contributors to sign our Contributor License Agreement (CLA), and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added. Sourcegraph teammates should refer to Accepting contributions for guidance. |
|
@cla-bot cla has been signed |
|
@keynmol Snapshots have been generated, and this is now ready for review! |
|
@matthewnitschke I've re-titled this PR and the linked issue to make it clearer that they're related to syntax highlighting, not search-based code nav for locals (which we also support via Tree-sitter), and not related to implementing the grammar itself (tree-sitter-x is conventionally used for the grammar names). |
Interesting, I was hoping I'd be able to utilize symbol searching in dart with this update, but that does not sound like the case. Is there a future where this functionality (referring to |
|
@matthewnitschke yes,
I would recommend handling that in a separate PR, since this PR is already quite big. I think that should be enough, but if we need to add separate |
| List<String> fruits = ['apple', 'banana', 'cherry']; | ||
| // ^^^^ IdentifierType | ||
| // ^ IdentifierOperator |
There was a problem hiding this comment.
If possible, the < and > here should be marked as PunctuationBracket as they're acting like brackets, not like operators.
There was a problem hiding this comment.
This should be resolved within: UserNobody14/tree-sitter-dart#61
There was a problem hiding this comment.
tree-sitter-dart has been updated to correctly mark these as @punction.bracket, but it appears as though the test framework no-longer annotates these markings
Is this intentional? or is there something wrong within the highlights.scm side of things?
| int age; | ||
| // ^^^ IdentifierType | ||
| // ^^^ IdentifierBuiltin |
There was a problem hiding this comment.
Fields like age should be Identifier not IdentifierBuiltin
There was a problem hiding this comment.
Resolved within UserNobody14/tree-sitter-dart#62
| var numbers = <int>[1, 2, 3, 4, 5]; | ||
| // ^^^ Keyword | ||
| // ^^^^^^^ IdentifierBuiltin | ||
| // ^ IdentifierOperator | ||
| // ^ IdentifierOperator |
There was a problem hiding this comment.
If possible, the < and > here should be treated as punctuation brackets instead of IdentifierOperator
There was a problem hiding this comment.
Same as https://github.com/sourcegraph/sourcegraph/pull/58480#discussion_r1555004826, doesn't look like @punction.bracket is included anymore
|
@varungandhi-src Oof, looks like the existing tree-sitter-dart implementation is not in the place I was hoping it was, I'll need to spend some time digging into the project As far as fixes go, would the preferred route be fixing the original tree-sitter-dart, and then updating sourcegraph's fork once its ready? Just looking to address this stuff in the cleanest way possible |
Yes, that would be the best way to do things. |
| tree-sitter-rust = "0.20.3" | ||
| tree-sitter-typescript = "0.20.2" | ||
|
|
||
| tree-sitter-dart = { git = "https://github.com/UserNobody14/tree-sitter-dart", rev = "923fbe0d3e62340ea78422551166ce946bc05222" } |
There was a problem hiding this comment.
I mirrored the changed applied to https://github.com/sourcegraph/tree-sitter-dart, in the upstream: UserNobody14/tree-sitter-dart#67
Unsure if there were other reasons for sourcegraph/tree-sitter-dart, but I think that repo can be archived now
|
A bit of a delay on this one... but I've gone ahead and addressed all of varun's original comments in the upstream Should be ready for another round of reviews |
| ; (hex_floating_point_literal) | ||
| ] @number | ||
|
|
||
| (symbol_literal) @symbol |
There was a problem hiding this comment.
If you see const MATCHES_TO_SYNTAX_KINDS: &[(&str, SyntaxKind)] in the Rust code, there is no SyntaxKind mapping for symbol, unlike string or constant.builtin. So we should use something else here.
| ; -------------------- | ||
| [ | ||
| (assert_builtin) | ||
| (break_statement) |
There was a problem hiding this comment.
| (break_statement) | |
| (break_builtin) |
Otherwise we'll end up marking the full range of the statement as a keyword.
Please add test cases for all the non-string keywords at least.
There was a problem hiding this comment.
Resolved and added additional test cases for these keyword types
| //^^^^^^ IdentifierType | ||
| // ^^^^^^ IdentifierType | ||
| // ^^^^^^^^^ IdentifierFunction | ||
| return Future.delayed(Duration(seconds: 2), () => "Data loaded"); |
There was a problem hiding this comment.
Looks like we're missing highlighting for seconds - is that deliberate?
varungandhi-src
left a comment
There was a problem hiding this comment.
LGTM modulo the minor remaining change for null. I'll do a quick end-to-end test before merging.
|
@matthewnitschke I've asked internally if we can trigger CI for PRs from forks. If not, I can recreate the PR on a separate branch. |
|
@matthewnitschke can you rebase on top of the latest changes on |
@varungandhi-src Merged |
|
The CI passed, but there is a 'Semgrep OSS' mandatory check that isn't being triggered for some reason, and I can't merge without that. I've asked in our internal Slack channel about it. |
Implementation of tree-sitter-dart as discussed in #57880 and #57987
Fixes #57880
Test plan
Added new snapshot tests