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

Tree-sitter rolling fixes: 1.121 edition #1085

Merged
merged 28 commits into from
Sep 17, 2024
Merged
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
4a701e2
Move indentation-related tasks to their own class…
savetheclocktower Aug 17, 2024
338f688
[language-css] Bump `tree-sitter-css` to `0.21.1`
savetheclocktower Aug 17, 2024
74ef36a
Fix bugs in invocations of `resolveMatch`
savetheclocktower Aug 17, 2024
6eee25f
Tweaks
savetheclocktower Aug 17, 2024
c5f87a6
Pass `comparisonRow` for `match` capture interpretation
savetheclocktower Aug 23, 2024
3834c3f
Add comments and make further indentation tweaks…
savetheclocktower Aug 24, 2024
50dbbd1
Add a spec for `@match.next`…
savetheclocktower Aug 24, 2024
738536f
[language-php] Handle case-insensitive constants like `TRUE`/`FALSE`…
savetheclocktower Aug 27, 2024
02f1563
Fail `(starts|ends)OnSameRowAs` when the node position descriptor fails
savetheclocktower Aug 27, 2024
9db65c0
[language-java] Fix the scope on `foo` in `foo.bar()`
savetheclocktower Aug 27, 2024
26be874
[language-typescript] Scope `!` in variable declarators
savetheclocktower Sep 4, 2024
87a9322
Upgrade `web-tree-sitter` to 0.23.0…
savetheclocktower Sep 7, 2024
f64b44f
(Whoops, left some logging statements in there)
savetheclocktower Sep 8, 2024
d433753
Fix regression with indentation
savetheclocktower Sep 8, 2024
4d696a7
Rewrite warning message in `web-tree-sitter`
savetheclocktower Sep 8, 2024
72b1a6e
[language-yaml] Update to newest `tree-sitter-yaml`
savetheclocktower Sep 8, 2024
c674d8f
Update README for `web-tree-sitter`
savetheclocktower Sep 8, 2024
265d7e0
[language-(java|type)script] Scope `??=`, `++`, etc., as assignments
savetheclocktower Sep 8, 2024
fbdf18a
`coverShallowerScopes` shouldn't affect “sibling” injections…
savetheclocktower Sep 8, 2024
03ba902
[language-(java|type)script]: Update Tree-sitter parsers to 0.23.0…
savetheclocktower Sep 9, 2024
44ad31f
[symbol-provider-tree-sitter] Fix specs…
savetheclocktower Sep 10, 2024
cdad4ff
[language-(java|type)script] Update injections…
savetheclocktower Sep 10, 2024
44d7734
[language-gfm] Migrate to `tree-sitter-markdown`…
savetheclocktower Sep 10, 2024
0f0050f
[language-html] Update to latest Tree-sitter parsers…
savetheclocktower Sep 14, 2024
99521c7
Prevent race condition during startup…
savetheclocktower Sep 14, 2024
986c697
[language-html] Update `parserSource` fields
savetheclocktower Sep 14, 2024
74479ba
Fix failing tests
savetheclocktower Sep 14, 2024
c1d3a8c
Address feedback
savetheclocktower Sep 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 34 additions & 14 deletions src/wasm-tree-sitter-language-mode.js
Copy link
Member

Choose a reason for hiding this comment

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

I'm tapping out reading all of this refactor line by line before review.

I can appreciate the motivation given in the PR body as for why this is being done. So, I will take some of the content on trust, it is simply too large to review closely in a timely manner, IMO. But hopefully this brings benefits as laid out in the PR body.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I'm angry at the diff; it really does overstate the change. I took three or four methods' worth of logic and put them in a new class at the bottom. The diff makes it seem like I changed this entire file!

Original file line number Diff line number Diff line change
Expand Up @@ -4176,17 +4176,7 @@ class IndentResolver {

let originalControllingLayer = options.controllingLayer;

let comparisonRow = options.comparisonRow;
if (comparisonRow === undefined) {
comparisonRow = row - 1;
if (options.skipBlankLines) {
// It usually makes no sense to compare to a blank row, so we'll move
// upward until we find a line with text on it.
while (this.buffer.isRowBlank(comparisonRow) && comparisonRow > 0) {
comparisonRow--;
}
}
}
let comparisonRow = options.comparisonRow ?? this.getComparisonRow(row, options);

let existingIndent = 0;
if (options.preserveLeadingWhitespace) {
Expand Down Expand Up @@ -4817,6 +4807,7 @@ class IndentResolver {
let { languageMode } = this;
const line = this.buffer.lineForRow(row);
const currentRowIndent = this.indentLevelForLine(line, tabLength);
let comparisonRow = options.comparisonRow ?? this.getComparisonRow(row, options);

// If the row is not indented at all, we have nothing to do, because we can
// only dedent a line at this phase.
Expand Down Expand Up @@ -4912,6 +4903,12 @@ class IndentResolver {
if (node.startPosition.row !== row) { continue; }
// Ignore captures that fail their scope tests.
if (!scopeResolver.store(indent)) { continue; }
let passed = this.applyTests(indent, {
currentRow: row,
comparisonRow,
tabLength
});
if (!passed) return;

let force = this.getProperty(indent, 'force', 'boolean', false);

Expand Down Expand Up @@ -4962,6 +4959,18 @@ class IndentResolver {
return currentRowIndent;
}

getComparisonRow(row, { skipBlankLines = true } = {}) {
let comparisonRow = row - 1;
if (skipBlankLines) {
// It usually makes no sense to compare to a blank row, so we'll move
// upward until we find a line with text on it.
while (this.buffer.isRowBlank(comparisonRow) && comparisonRow > 0) {
comparisonRow--;
}
}
return comparisonRow;
}

indentLevelForLine(line, tabLength) {
let indentLength = 0;
for (let i = 0, { length } = line; i < length; i++) {
Expand Down Expand Up @@ -4990,7 +4999,9 @@ class IndentResolver {
return undefined;
}

// `indent.match` used to be called `indent.matchIndentOf`.
let matchIndentOf = this.getProperty(capture, ['match', 'matchIndentOf'], 'string', null);
// `indent.offset` used to be called `indent.offsetIndent`.
let offsetIndent = this.getProperty(capture, ['offset', 'offsetIndent'], 'number', 0);

if (!matchIndentOf) return undefined;
Expand All @@ -5012,31 +5023,39 @@ class IndentResolver {
return Math.max(result, 0);
}

// Look up an `indent.` capture property applied with a `#set!` directive,
// optionally coercing to a specified type or falling back to a default
// value.
//
// `names` can be an array in cases where the property may have several
// aliases. The first one that exists will be returned. (Omit the leading
// `indent.` when passing property names.)
Comment on lines +5202 to +5208
Copy link
Member

Choose a reason for hiding this comment

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

capture property applied with a #set! directive, [ . . . ]

Is this a verb "capture" or a noun "capture"? If a verb: Who captures the property? And dare I ask: why? Or if it's a noun "capture": is this telling us a capture property was applied with a #set! directive, and how does type coercion or fallbacks play into all that?

https://i.kym-cdn.com/photos/images/newsfeed/002/029/841/a26.png

Sorry, just surfacing some of my specific confusion areas around this subject, as I see it was requested in the PR body to ask clarifying questions if I don't understand something. And joking around a bit, I hope you don't mind the humor. (See PNG above.)

Copy link
Member

Choose a reason for hiding this comment

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

I see the following line adds quite a bit of context, just from the function name and argument names alone, but still. I'm not sure I totally understand.

getProperty(capture, names, coercion = null, fallback = null) {

Copy link
Sponsor Contributor Author

@savetheclocktower savetheclocktower Sep 17, 2024

Choose a reason for hiding this comment

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

is this telling us a capture property was applied with a #set! directive, and how does type coercion or fallbacks play into all that?

It's a noun capture! Capture properties are things like the #set! clause in

((foo) @bar
 (#set! indent.offset 1))

Despite the fact that it doesn't require quotes, the 1 in this example will be returned to us as a string. It's all strings to Tree-sitter. Hence the coercion.

We also have indent.force (which should be coerced to a boolean), and indent.match) (which should stay a string). So three different data types — hence the coercion.

And joking around a bit, I hope you don't mind the humor. (See PNG above.)

STOP DOING HUMOR

  • words were not intended to be spoken deceitfully
  • YEARS of satire yet all we ever do is get MORE SARCASTIC
  • “An escalator can never break; it can only Become Stairs. I spilled spot remover on my dog; he’s gone now.” — Statements dreamed up by the Utterly Deranged

“Why did the chicken cross the road???“

they have played us for absolute fools

getProperty(capture, names, coercion = null, fallback = null) {
if (typeof names === 'string') {
names = [names];
}
for (let name of names) {
let fullName = `indent.${name}`;
let { setProperties: props = {} } = capture;
if (!(fullName in props)) {
continue;
}
if (!(fullName in props)) { continue; }
return this.coerce(props[fullName], coercion) ?? fallback;
}
return fallback;
}

coerce(value, coercion) {
switch (coercion) {
case String:
case 'string':
if (value == null) return "";
return value;
case Number:
case 'number': {
let number = Number(value);
if (isNaN(number)) return null;
return number;
}
case Boolean:
case 'boolean':
if (value == null) return null;
return true;
Expand Down Expand Up @@ -5065,6 +5084,7 @@ class IndentResolver {
}
}


IndentResolver.TESTS = {
// Returns `true` if the position descriptor's row equals that of the current
// row (the row whose indentation level is being suggested).
Expand Down