8000 GrammarRegistry.loadGrammar does not catch errors from addGrammar · Issue #23041 · atom/atom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

GrammarRegistry.loadGrammar does not catch errors from addGrammar #23041

Open
1 task done
winstliu opened this issue Sep 28, 2021 · 3 comments
Open
1 task done

GrammarRegistry.loadGrammar does not catch errors from addGrammar #23041

winstliu opened this issue Sep 28, 2021 · 3 comments

Comments

@winstliu
Copy link
Contributor
winstliu commented Sep 28, 2021 •
edited
Loading

Prerequisites

Description

While investigating winstliu/language-hclrs#4, I root-caused the issue to using a newer tree-sitter version than Atom supported. However, this was non-obvious, and the only place it was reported was in the console.

When loading grammars, loadGrammar checks to make sure that readGrammar succeeds:

atom/src/grammar-registry.js

Lines 552 to 553 in 3f4b251

this.readGrammar(grammarPath, (error, grammar) => {
if (error) return callback(error);

However, there are no checks for the subsequent addGrammar call:

atom/src/grammar-registry.js

Lines 554 to 555 in 3f4b251

this.addGrammar(grammar);
callback(null, grammar);

Even though addGrammar can fail when (eventually) calling TreeSitterLanguageMode.parse, which itself calls parser.setLanguage which can throw.

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

Line 117 in 3f4b251

parser.setLanguage(language);

Resulting in e.g.
Incompatible tree-sitter version

Steps to Reproduce

  1. Install language-hclrs@0.0.2
  2. Set language to HCLRS

Expected behavior:

A helpful error notificaton.

Actual behavior:

Nothing! Why isn't my syntax highlighting working?

Reproduces how often:

100%

Versions

1.58.0, Windows 11 Insider.

Additional Information

N/A
@icecream17
Copy link
Contributor
icecream17 commented Sep 28, 2021 •
edited
Loading

Wow, this information here is more helpful than the information I gave at #22129... going to update that

@icecream17 icecream17 mentioned this issue Sep 28, 2021
Open
1 task
@winstliu
Copy link
Contributor Author

Whoops, didn't search for the right keywords 😊. Figures that this has already been reported.

I'll keep this one open to track the fact that loadGrammar is unreliable at reporting errors, and #22129 seems better suited to tracking how Atom can support languages that use tree-sitter 0.19.0+ :).

@sadick254 sadick254 mentioned this issue Oct 5, 2021
Closed
@claytonrcarter
Copy link
Contributor

This also seems to be happening outside of GrammarRegistry.loadGrammar. In particular, if a "working" tree-sitter grammar is loaded but a "not working" grammar is injected into some part of it. Also, because injections can happen in multiple places, this could trigger multiple errors.

@claytonrcarter claytonrcarter mentioned this issue May 5, 2022
Open
13 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@claytonrcarter @winstliu @icecream17
0