Tree-sitter rolling fixes (February) #906

savetheclocktower · 2024-02-01T16:58:27Z

Fresh off the merging of #859! This one will also land before the 1.114 release.

DeeDeeG · 2024-02-01T20:21:57Z

Just popping in to suggest we might shoot for two of these PRs per month? May mess with your PR titles if we do that, maybe Feb part 1, Feb Part 2? I dunno. In any case, I am thankful for this continued work.

savetheclocktower · 2024-02-01T20:23:10Z

Yeah, I'll just have to start naming them differently. (I created this branch on January 31, so I'm already cheating.)

…that were added to the theme generator in pulsar-edit/ppm#123.

…and add some `punctuation` scopes for the generic type delimiters.

This code snippet sets up logging so that a worker process can call `console.log` and its siblings and have them show up in the renderer process’s console… but it’s trying to destructure the antiquated magical `arguments` collection (which is like an array, but isn’t an array!) and the engine doesn’t seem to like it. Logging didn’t work for me until I changed this. This is such a tiny fix, and I don’t have the energy to shelve everything on this branch just to make a separate PR.

New changes since then

Changes: * `#define FOO 1` and `#define FOO` will _always_ scope `FOO` as `constant.other.c`, even when `FOO` is mixed-case or lower-case. * `#define FOO()` will _always_ scope `FOO` as `entity.name.function.preprocessor.c`, even when `FOO` is mixed-case or lower-case. * Usages of bare identifiers in other contexts in preprocessor directives should always scope them as `constant.other.c` — unless I’m made aware of any counterexamples where that shouldn’t happen. For example: in `#if TABLE_SIZE > 200`, `TABLE_SIZE` should always be `constant.other.c`, no matter its casing. * All-caps variable declarations and assignments (`int FOO = 0`, etc.) should scope `FOO` as `variable` rather than `constant`. We should only scope an arbitrary identifier as `constant` if we think it’s been used in the context of a macro definition. However: * When deciding whether something outside of a directive refers to a macro constant, we have no choice but to use the `ALL_CAPS` heuristic. Ideally we’d be able to determine that from some sort of analysis of the source code files — but even if we could keep track of that sort of thing in a single buffer, it’d fall down when macro constants _defined in other files_ are used. All we can do here is make a best guess.

…such as in multi-line self-closing tags and multi-line opening tags. Also fix an issue in fold handling that made it impossible to use `@_IGNORE_` captures in `folds.scm`.

savetheclocktower · 2024-02-09T22:23:26Z

This one's ready for review — or will be after the inevitable spurious CI failures.

DeeDeeG

Looking through/commenting on the off-topic stuff first.

DeeDeeG · 2024-02-10T04:41:27Z

static/variables/syntax-variables.less

+@syntax-color-string: #97C378;
+@syntax-color-comment: #888;


Just to be sure: These are fallback variables, so this shouldn't really impact any existing themes, right? Existing themes either already provided these values themselves and were using them internally to the specific theme, or else any usages of these variable names would have resulted in LESS errors, I suppose.

So this should only add to the list of valid LESS variable names theme authors can reference in their themes? (And for users to reference in their user theme.) Doesn't seem like it would have a chance to cause any conflicts or issues.

Yes. There's still the matter of adding these variables to the built-in themes, and if I'd had the time for that then this change would've gone into a PR along with those changes. Failing that, I wanted to get this change in as soon as possible, which is why it's in this PR where it doesn't belong.

It'd be good to reference the specific commit adding them here, and this PR as well, if doing a follow-up to add it to the default themes, then, IMO.

DeeDeeG · 2024-02-10T04:44:20Z

src/task.js

I am slightly tempted to ask for this to be reverted here and I could volunteer to cherry-pick it and post it as its own PR. But I would struggle to provide much of a description in such a PR body other than copy-pasting your commit message.

So, I think I'll just call this out here and acknowledge that it's included, even if it's not strictly/narrowly related to the rest of the PR changes.

For anyone out there trawling the git blame view and finding this PR: Yes, we know this is unrelated! I hope the commit message is helpful enough if this in fact caused any issues.

Some docs, if this ends up puzzling anyone in the future looking back on this:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments <-- the old thing

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters <-- the new thing

* A new predicate called `fold.invalidateOnChange` that can be used when a change should automatically invalidate the fold cache for each row in a node's range. * The ability to use custom predicates in `folds.scm` files. (Previously, captures from `folds.scm` did not consult an instance of `ScopeResolver` in the processing phase.)

savetheclocktower · 2024-02-14T05:21:11Z

Rolled back an attempted bump of tree-sitter-bash because it caused a “memory access out of bounds” exception. But only on the CI build! Not locally. I hate it.

DeeDeeG

Once again, I wish I understood more deeply, but this all seems good to the extent I can understand it, and I have read up on some of the issues these changes are fixing.

(I've also read the commit messages and diff, including code comments.)

To the best of my ability, since it's better to have an underqualified but sincere attempt at a review than a pure rubber-stamp, here's my Approve. 👍 Thanks as always.

DeeDeeG · 2024-02-10T05:04:37Z

.eslintignore

👍

I'm personally still fine with tweaking these files until linters are okay with our current code. (Mentioning again so folks don't have to go through older PRs to find the discussion/justification for updating these.)

(For the vendor/ dir, I definitely agree with linters just ignoring that code. Because that code's from elsewhere, and we more or less don't want to be tweaking it just because a linter told us we might want to.)

DeeDeeG · 2024-02-15T00:46:05Z

packages/language-c/grammars/tree-sitter-c/highlights.scm

👍 I like the amount of comments in this, I sort of almost can follow along with this. (One of these days I need to read what I believe I heard you've written about how to write .scm files, so I know what's specifically going on in these. But I can sense the comments will come very much in handy then.)

It's probably because of how not-at-home I feel inside C/C++ as opposed to most of the other languages for which I've written highlights.scm files.

I was gonna say, at risk of being mildly off-topic or tangenty: C seems really tricky based on reading through this (plus some stuff I'm dealing with outside of Pulsar lately that's in C...)

DeeDeeG · 2024-02-15T00:49:03Z

packages/language-c/grammars/tree-sitter-c/highlights.scm

-  declarator: (identifier) @variable.declaration.c)
+  declarator: (identifier) @variable.other.declaration.c)


Why are we marking so many of these as .other, in addition to the various other CSS classes being added?

I can guess it helps make sure rules in stylesheets actually apply, by adding this class on to the rule's selector to increase its specificity as needed, without resorting to !important... Is this basically the reason, or anything else I'm missing?

DeeDeeG · 2024-02-15T00:52:20Z

packages/language-html/grammars/tree-sitter-html/folds.scm

Knowing the issue this is addressing, these comments are great and very readable, IMO.

Even though I can't read .scm files that well right now, I know what purpose this is accomplishing. 👍. Thanks.

DeeDeeG · 2024-02-15T00:56:42Z

spec/wasm-tree-sitter-language-mode-spec.js

+    it('understands custom predicates', async () => {
+      const grammar = new WASMTreeSitterGrammar(atom.grammars, htmlGrammarPath, htmlConfig);
+
+      await grammar.setQueryForTest('foldsQuery', scm`


Does this scm`templateStringHere` stuff call the scm() function with the template string as an argument? This syntax is a tad confusing to read, IMO. If it can use parentheses, maybe that'd be better? Just a style nitpick, I suppose. Hmm.

DeeDeeG · 2024-02-15T00:57:16Z

spec/wasm-tree-sitter-language-mode-spec.js

Thanks for adding specs for this fold.invalidateOnChange feature!

DeeDeeG · 2024-02-15T01:01:40Z

src/wasm-tree-sitter-language-mode.js

-      } else {
+      } else if (capture.name.startsWith('fold.')) {
        let key = this.keyForDividedFold(capture);
        boundaries = boundaries.insert(key, capture);
      }


Is there any downside to making this basic else "fallback" block become specific/conditional? Could anything be missed if ! capture.name.startsWith('fold.') here?

No — and, in fact, the old code was breaking things. I had thought I'd made it so that you could safely use @_IGNORE_ as a capture name in any .scm file, but when I tried it in folds.scm, I discovered that I'd written this in such a way (almost a year ago) that only three different kinds of capture names were envisioned in a folds.scm. So the @_IGNORE_ was being treated like a divided fold (@fold.start or @fold.end) when it clearly isn't.

In general, if a query file doesn't assign a special meaning to a certain capture name, it should be silently ignored.

DeeDeeG · 2024-02-15T01:03:55Z

src/wasm-tree-sitter-language-mode.js

+    for (let range of foldRangeList) {
+      // The fold cache is automatically cleared for any range that needs
+      // re-highlighting. But sometimes we need to go further and invalidate
+      // rows that don't even need highlighting changes.
+      this.languageMode.emitFoldUpdate(range);
+    }


Any insights on this? I'm curious about why this is needed, but I'm not sure I have context to get it, to be honest. Optional but appreciated if you can give a go at explaining this further.

In general, whenever syntax highlighting changes, that's a good indicator that we should re-investigate whether the affected lines have changed from foldable to not-foldable (or vice versa). That's why emitRangeUpdate (the method that signals the display layer for re-highlighting) actually calls emitFoldUpdate on the same ranges.

But sometimes (rarely!) a row's foldability might be affected by a change that would not (should not) result in re-highlighting. The case I added a test for was the one very specific place that I needed this.

(There's a similar feature — the highlight.invalidateOnChange predicate — that scratches this same itch on the highlighting side. And I've only ever needed it once: when a /* comment chanes into a JavaDoc-style /** comment and several lines need to be re-highlighted at once.)

DeeDeeG · 2024-02-15T01:04:39Z

src/wasm-tree-sitter-language-mode.js

+  // Checks whether a given {Point} lies within one of this layer's content
+  // ranges — not just its extent. The optional `exclusive` flag will return
+  // `false` if the point lies on a boundary of a content range.
  containsPoint(point, exclusive = false) {


Yay more code comments 👍 !

savetheclocktower · 2024-02-15T05:10:32Z

I'm a big fan of your willingness to wade into these PRs and ask questions about them. I should be at least as gracious for your devops-related PRs, so that'll be a resolution of mine for the coming month. Thanks!

savetheclocktower mentioned this pull request Feb 1, 2024

MEGA-ISSUE: Syntax highlighting in language-php #876

Open

This comment was marked as outdated.

Sign in to view

savetheclocktower added 5 commits February 1, 2024 21:35

[language-php] Fix block comments failing to re-highlight

9829178

[language-typescript] Highlight escape characters

32c464b

Add fallback values for new compulsory syntax variables…

69fb61e

…that were added to the theme generator in pulsar-edit/ppm#123.

[language-php] Update tree-sitter-phpdoc…

3131464

…and add some `punctuation` scopes for the generic type delimiters.

savetheclocktower marked this pull request as draft February 3, 2024 07:33

savetheclocktower force-pushed the tree-sitter-february branch from 3be2023 to 5628d08 Compare February 3, 2024 07:35

Forgot to bump parserSource (the system works!)

10ce3b1

savetheclocktower mentioned this pull request Feb 4, 2024

MEGA-ISSUE: Syntax highlighting in language-c (C/C++) #877

Open

savetheclocktower added 4 commits February 4, 2024 13:36

[language-c] Add missing anchor to constant pattern

e332953

Don’t try to lint JS in the vendor directory

c188a26

[language-html] Fix indentation after self-closing tags

cc20d70

[language-html] Allow for folding of attribute lists…

89686ce

…such as in multi-line self-closing tags and multi-line opening tags. Also fix an issue in fold handling that made it impossible to use `@_IGNORE_` captures in `folds.scm`.

savetheclocktower marked this pull request as ready for review February 9, 2024 22:23

DeeDeeG reviewed Feb 10, 2024

View reviewed changes

savetheclocktower added 2 commits February 9, 2024 22:33

[language-typescript] Add syntax highlighting for enums

82eac22

savetheclocktower mentioned this pull request Feb 13, 2024

MEGA-ISSUE: Syntax highlighting in language-shellscript #873

Open

savetheclocktower force-pushed the tree-sitter-february branch from 0510a0f to 9b1e8e5 Compare February 14, 2024 05:19

DeeDeeG approved these changes Feb 15, 2024

View reviewed changes

DeeDeeG mentioned this pull request Feb 15, 2024

Fold enhancements for modern Tree-sitter #918

Closed

savetheclocktower merged commit ec12b2f into pulsar-edit:master Feb 15, 2024
104 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree-sitter rolling fixes (February) #906

Tree-sitter rolling fixes (February) #906

savetheclocktower commented Feb 1, 2024

DeeDeeG commented Feb 1, 2024

savetheclocktower commented Feb 1, 2024 •

edited

Loading

This comment was marked as outdated.

savetheclocktower commented Feb 9, 2024

DeeDeeG left a comment

DeeDeeG Feb 10, 2024 •

edited

Loading

savetheclocktower Feb 10, 2024

DeeDeeG Feb 10, 2024

DeeDeeG Feb 10, 2024 •

edited

Loading

DeeDeeG Feb 10, 2024

savetheclocktower commented Feb 14, 2024 •

edited

Loading

DeeDeeG left a comment

DeeDeeG Feb 10, 2024

DeeDeeG Feb 15, 2024

savetheclocktower Feb 15, 2024

DeeDeeG Feb 15, 2024

DeeDeeG Feb 15, 2024 •

edited

Loading

DeeDeeG Feb 15, 2024

DeeDeeG Feb 15, 2024 •

edited

Loading

DeeDeeG Feb 15, 2024

DeeDeeG Feb 15, 2024 •

edited

Loading

savetheclocktower Feb 15, 2024

DeeDeeG Feb 15, 2024

savetheclocktower Feb 15, 2024 •

edited

Loading

DeeDeeG Feb 15, 2024

savetheclocktower commented Feb 15, 2024

		declarator: (identifier) @variable.declaration.c)
		declarator: (identifier) @variable.other.declaration.c)

		@syntax-color-string: #97C378;
		@syntax-color-comment: #888;

Tree-sitter rolling fixes (February) #906

Tree-sitter rolling fixes (February) #906

Conversation

savetheclocktower commented Feb 1, 2024

DeeDeeG commented Feb 1, 2024

savetheclocktower commented Feb 1, 2024 • edited Loading

This comment was marked as outdated.

savetheclocktower commented Feb 9, 2024

DeeDeeG left a comment

Choose a reason for hiding this comment

DeeDeeG Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeeDeeG Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savetheclocktower commented Feb 14, 2024 • edited Loading

DeeDeeG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeeDeeG Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeeDeeG Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeeDeeG Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savetheclocktower Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savetheclocktower commented Feb 15, 2024

savetheclocktower commented Feb 1, 2024 •

edited

Loading

DeeDeeG Feb 10, 2024 •

edited

Loading

DeeDeeG Feb 10, 2024 •

edited

Loading

savetheclocktower commented Feb 14, 2024 •

edited

Loading

DeeDeeG Feb 15, 2024 •

edited

Loading

DeeDeeG Feb 15, 2024 •

edited

Loading

DeeDeeG Feb 15, 2024 •

edited

Loading

savetheclocktower Feb 15, 2024 •

edited

Loading