Skip to content

Fix Cannot read properties of undefined (reading 'languageScopeId') - #1640

Open
mauricioszabo wants to merge 4 commits into
pulsar-edit:masterfrom
mauricioszabo:fix-null-scope
Open

mauricioszabo wants to merge 4 commits into
pulsar-edit:masterfrom
mauricioszabo:fix-null-scope

Conversation

@mauricioszabo

Copy link
Copy Markdown
Contributor

Under vim-mode-plus, sometimes the plug-in wants to highlight searches and elements in the screen - but there are moments that it can try to destroy a marker "mid-parsing". The issue - it calls updateSync on the text editor component, and it might cause a crash when there are no "open scopes" because we're falling back to the null layer.

This PR fixes this very hard to find issue, and it also adds some refactorings to the test (essentially, building a "Tree Sitter Grammar" with injections was using too many lines - I extracted that to a helper method).

@mauricioszabo mauricioszabo self-assigned this Sep 5, 2026

@savetheclocktower savetheclocktower left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Beg forgiveness, but let's fix this another way:

The problem is how the NullLayerHighlightIterator behaves. Certain pieces of code expect a certain interface on a HighlightIterator, but NullLayerHighlightIterator doesn't implement some of that interface.

class NullLayerHighlightIterator {
  constructor(languageLayer) {
    this.languageLayer = languageLayer;
    this.depth = languageLayer.depth;
    this.coverShallowerScopes = false;
  }
  seek() {
    return [false, new OpenScopeMap];
  }
  compare() {
    return 1;
  }
  moveToSuccessor() {}
  getPosition() {
    return Point.INFINITY;
  }
  getOpenScopeIds() {
    return [];
  }
  getCloseScopeIds() {
    return [];
  }
}

That seems to give it the things it needs: depth, coverShallowerScopes, and languageLayer.languageScopeId.

Also, when it's instantiated (around line 3942), it should take this instead of zero arguments.

I think that gets us what we want without using openScopes.size as a proxy.

@mauricioszabo

Copy link
Copy Markdown
Contributor Author

@savetheclocktower applied the fix, can you check again?

@savetheclocktower savetheclocktower left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gonna approve because this makes the code better, but I have one more (smaller) fix to request:

First:

let ranges = iterator.languageLayer.getCurrentRanges();

This could now return null, so changing it to

let ranges = iterator.languageLayer.getCurrentRanges() ?? [];

would be sounder. This is unlikely to occur very often, but worth closing it off altogether.

Claude also offers the following feedback about the new prepareTSLanguageMode helper, none of which I caught myself:

  1. Grammar subscriptions now leak. The afterEach disposes the describe-level grammar; several refactored tests used to assign it (grammar = new WASMTreeSitterGrammar(...), no const). The helper creates jsGrammar/regexGrammar locally and never hands them back, so nothing disposes them. Worth assigning to the outer grammar or returning them.
  2. Every helper call now installs a regex_pattern injection point with coverShallowerScopes: true and registers the regex grammar — including tests that previously had neither. Most notably reports results correctly when scope ranges have been adjusted uses let foo = /patt?ern/; and asserts the keyword scope is found, while its sibling test asserts the same scope is not found precisely because of coverShallowerScopes. The only thing keeping them apart now is that the first one omits await wait(100), so the injection hasn't populated when the assertions run. CI is green on Tests (ubuntu-latest), so it works today, but that test's outcome is now timing-dependent in a way it wasn't. I'd make the injection opt-in (or at least default coverShallowerScopes to false).
  3. regexQuery = '' blanks the query rather than emptying it. setQueryForTest assigns this[queryType] = contents, and LanguageLayer gates on if (grammar[queryType]) — so '' means the regex grammar ends up with no highlightsQuery at all (its real one from the cson is clobbered too). Probably fine, but '; (placeholder)' would be more honest about intent.
  4. Minor: the new regression test's expect(...).not.toThrow() passes trivially if the injection layer isn't built yet. The two seek() assertions above it help, but asserting the actual scopes from scopeDescriptorForPosition would make it harder to silently stop testing the bug.

So please fix the thing I mentioned above, and perhaps tweak the helper a bit… but there's no point in rejecting when I did so already and caught new stuff on the second. After that change, as long as CI is green, you can merge this.

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.

2 participants