Implementing tree-sitter based indentation logic #18321

chfritz · 2018-10-25T04:46:45Z

Issue or RFC Endorsed by Atom's Maintainers

This approach was supported by @maxbrunsfeld in atom/language-javascript#594 (comment). It also addresses a number of indentation logic related issues such as #6655, atom/language-javascript#560, and potentially others.

Description of the Change

Implements a tree-sitter based indentation logic. This was previously developed and tested in the sane-indentation package (> 0.9). Refer to atom/language-javascript#594 (comment)

By itself this does nothing. The new logic is only used if the language package for the current language contains the necessary configuration (e.g., which scopes to indent on). So this PR goes together with, e.g., atom/language-javascript#608 in language-javascript.

Alternate Designs

I went through a number of iterations developing this code in the sane-indentation package. I first used the scopes array directly, but found that it only got me so far because it was lacking context. So I switched to this new tree-walk approach. It is more concise and has more context to make indentation decisions on. As reported in atom/language-javascript#594 (comment), I found this enough to implement proper indentation for a variety of languages.

Possible Drawbacks

I have not found any cases where the new indentation logic is worse that the previous, regex based logic. In particular it uses the already-present parse tree from tree-sitter, so it seems faster and more comprehensive.

Verification Process

I developed this code over the course of the last two months in the sane-indentation package (which monkey patches the suggestIndentForBufferRow function to replace the atom indentation logic with this new logic. As proposed by @maxbrunsfeld (atom/language-javascript#594 (comment)), I worked on adding support for other languages besides javascript to make sure the logic is general enough to support various types of languages. As reported in atom/language-javascript#594 (comment), I found this logic sufficient for implementing proper indentation for Javascript (incl. JSX), HTML, Python, C and C++. I have not yet tried others.

The most comprehensive test was using this new logic in my every-day development (Javascript, JSX, and HTML) for over a month now.

chfritz · 2018-10-25T17:44:24Z

Sorry about those linting errors. I'll fix them.

maxbrunsfeld · 2018-10-25T18:13:06Z

This is very, very cool.

@chfritz Just letting you know - because this is a big and important new feature, (and we're currently addressing regressions that we discovered after enabling Tree-sitter by default), it will probably take some time for us to get to this and review it. But I'm definitely very interested.

Some initial questions:

It seems like we'd want to have one place where indentation is configured for a language, as opposed to having one place in a .cson file, and another place that involves JavaScript code. Is there a way to convert the JS code into a declarative format that can be loaded through JSON/CSON? If not, maybe the entire configuration should be through a JS API.
Even more meta - do you think it's feasible to have Atom automatically infer these indentation rules from an example source code file? That seems like the holy grail, but is perhaps too complicated.

chfritz · 2018-11-06T04:45:40Z

@maxbrunsfeld : sorry for the delay in responding. I somehow broke my dev mode (it keeps crashing with a segfault), and haven't been able to try a few things on Item 1 that I wanted to try. I'll get back to you as soon as I get past this impasse.

On Item 2: that's a really interesting idea. I need to think this through further. I'm not yet sure whether the correct (i.e., desired) indentation can be inferred from just one example in all cases, or how many variables the implied set of equations has. If the only logic is scopes, then yes, this should be possible. But those additional conditions (Item 1) seem to span a much bigger space.

The current solution actually already provides a fair bit of room for customization, because users can decide for themselves which scopes should cause indentation. That way, issues that are more of a matter of opinion such as atom/language-javascript#298 can be made a thing of the past.

chfritz · 2019-01-07T02:45:42Z

@maxbrunsfeld at long last I've gotten my dev-mode back into a working condition to test changes to this PR.

I evaluated translating the code-based config to something declarative, but that was just getting too convoluted. So I've now gone the other way you suggested and moved all config to code, see my example for language-javascript.

I think this is ready for review now.

…core See this PR in atom/atom: atom/atom#18321. Once the other PR is merged, this one then fixes atom#594. Updated: now without the need for a callback function as part of configuration

chfritz · 2019-01-12T04:08:02Z

Good news, @maxbrunsfeld , I was wrong! I did find a way to get rid of the non-declarative part of the configuration after all. The language-specific configuration is now all declarative as it should be: https://github.com/atom/language-javascript/pull/608/files.

maxbrunsfeld · 2019-01-12T04:16:16Z

Woah, that’s looking very very clean. Nice work, @chfritz!

chfritz · 2019-01-12T20:34:49Z

Thanks!

lee-dohm · 2019-01-15T22:17:26Z

@chfritz This sounds pretty awesome. Unfortunately, before we can spend a bunch of time reviewing it, we're going to need comprehensive tests added so that we can ensure we don't accidentally regress any of the functionality you've painstakingly built here.

chfritz · 2019-01-16T05:45:39Z

I'd be happy to write some tests, but unfortunately I'm still having a hose of issues with my source build on Linux. My dev mode crashes all the time, and when I try to run tests I get:

> atom --dev --test --timeout 60 spec/config-spec.js 
...../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption detected. 
Renderer process crashed, exiting

This doesn't seem to depend on the specs I'm testing. Even some of the tiniest specs fail:

> atom --dev --test --timeout 60 spec/typescript-spec.coffee 
.../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption detected. 
Renderer process crashed, exiting

This is after a clean checkout of master, ./script/clean and ./script/bootstrap.

Furthermore ./scripts/build also crashes with:

Verifying if snapshot can be executed via `mksnapshot`
/home/cfritz/github/atom/out/startup.js:1
var snapshotAuxiliaryData={lessSourcesByRelativeFilePath:{"static/atom-ui/_index.less":{content:'// Atom UI\n\n// Private! Don\'t use these in packages.\n// If you need something, feel free to open an issue and it might can be made public\n@import "styles/private/scaffolding.less";\n\n@import "styles/private/alerts.less";\n@import "styles/private/close.less";\n@import "styles/private/code.less";\n@import "styles/private/forms.less";\n@import "styles/private/links.less";\n@import "styles/private/navs.less";\n@import "styles/private/sections.less";\n@import "styles/private/tables.less";\n@import "styles/private/utilities.less";\n\n\n// Public components\n// Open the Styleguide to see them in action\n@import "styles/badges.less";\n@import "styles/button-groups.less";\n@import "styles/buttons.less";\n@import "styles/git-status.less";\n@import "styles/icons.less";\n@import "styles/inputs.less";\n@import "styles/layout.less";\n@import "styles/lists.less";\n@import "styles

Error: Cannot require module "fs".
To use Node's require you need to call `snapshotResult.setGlobals` first!
    at require (/home/cfritz/github/atom/out/startup.js:1:660391)
    at customRequire (/home/cfritz/github/atom/out/startup.js:1:660842)
    at get_fs (/home/cfritz/github/atom/out/startup.js:11:258467)
    at getElectronPath (/home/cfritz/github/atom/out/startup.js:11:258623)
    at Object.../../../../node_modules/electron/index.js (/home/cfritz/github/atom/out/startup.js:11:258976)
    at customRequire (/home/cfritz/github/atom/out/startup.js:1:660732)
    at Object.../src/atom-environment.js (/home/cfritz/github/atom/out/startup.js:1:665279)
    at customRequire (/home/cfritz/github/atom/out/startup.js:1:660732)
    at Object.<anonymous> (/home/cfritz/github/atom/out/startup.js:1:661073)
    at Object.../src/initialize-application-window.js (/home/cfritz/github/atom/out/startup.js:1:665145)
Error: Command failed: /home/cfritz/github/atom/out/atom-dev-1.36.0-dev-4979677-amd64/atom /home/cfritz/github/atom/script/verify-snapshot-script /home/cfritz/github/atom/out/startup.js

I have been struggling with issues of this sort since end of October and frankly, they make developing for atom.io quite annoying. Any idea what might be going wrong? By now I've spent more time trying to debug why the development environment doesn't work than I've spent on the actual contribution I'm trying to make.

For completeness:

> apm --version
apm  2.1.3
npm  6.2.0
node 8.9.3 x64
atom 1.34.0
python 2.7.12
git 2.7.4
> lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.5 LTS
Release:	16.04
Codename:	xenial

chfritz · 2019-01-20T00:58:30Z

I was able to develop and add tests by using the deb-package installation of atom (9 tests, 292 assertions). The bulk of the test cases are defined in terms of correctly indented sample files. For each of these tests it is verified that the proposed indentation matches the one in the file.

…core See this PR in atom/atom: atom/atom#18321. Once the other PR is merged, this one then fixes atom#594. Updated: now without the need for a callback function as part of configuration

rafeca · 2019-05-31T21:21:26Z

Hey! 👋

We have recently started using prettier and this has caused major style changes on the Atom JS codebase (you can check the related PR).

This is good news: now the Atom code is more consistent and it's much easier to re-format the code to follow the codestyle (now you only need to run script/lint --fix).

This change caused conflicts on your PR that we have automatically solved, hope you don't mind 😄

With ❤️, the Atom team.

chfritz · 2019-09-05T02:59:10Z

It's now been almost a year since I've opened this PR and more than six months since finishing all requested changes. Are there any plans to still consider this, or will this PR go the same route as the one I opened three years ago, which never got merged? It's a little frustrating to work on something that was endorsed by the core developers and then see it being ignored forever and ever.

chfritz · 2019-09-05T03:02:14Z

PS: needless to say that this PR is yet again out of date by now I need to put more work into catching up with changes that happened elsewhere. But I'd like to have some assurance that all that work is not for nothing.

chfritz · 2021-01-06T17:10:08Z

This is amazing. Apparently this PR has inspired tree-sitter based indentation logic for both emacs and neovim. Both of these seem to have been merged. Maybe if we give it another two years we can put a plan together to merge this into atom as well?

sadick254 · 2021-05-21T05:05:16Z

@chfritz I know this has been long overdue. I have resolved some conflicts on this PR to give you an easy time should you choose to fix the failing tests.
In case you are busy, I would like to offer to continue working on this PR and fix the failing tests. I am interested in this feature and would like to see it land on Atom stable version. Let me know.

chfritz · 2021-05-21T16:12:59Z

Thanks, @sadick254 . Glad to hear this effort may not have been in vein.

I'd be happy to try and fix the failing tests, but to be honest, I can't quite decipher them. What has

cp : Cannot find path 'D:\Library\Logs\DiagnosticReports' because it does not exist.

to do with my code?

But I can try to work on this one, which seems to be about the should-be indentation.

sadick254 · 2021-05-21T18:51:08Z

@chfritz For now you can ignore any errors that are not relevant to the indentation work.

chfritz · 2021-05-25T23:25:13Z

I think the test that is failing is just about specifying what is correct indentation. The new logic changes the indentation -- to the better, I would argue, of course. So the "fix" would be to change the specification. ~~Can you help me find "spec/tree-indenter-spec.js"? I can't find it in the source tree.~~ -- nevermind, sorry, it's been too long. That's my own file! 🤦

sadick254 · 2021-05-26T05:47:42Z

@chfritz No worries. Just let me know if you need any assitance. I am keen to see this go through.

chfritz · 2021-05-28T00:59:47Z

I've been trying to compile atom for the last two days, with no luck. It gets stuck on this bizarre error:

> [email protected] postinstall /home/cfritz/github/_atom/node_modules/vscode-ripgrep
> node ./lib/postinstall.js

Finding release for v12.1.1
GET https://api.github.com/repos/microsoft/ripgrep-prebuilt/releases/tags/v12.1.1
Deleting invalid download cache
Downloading ripgrep failed: Error: Request failed: 401

Any advise?

> node --version && npm --version && lsb_release -r
v12.22.1
6.14.12
Release:	20.04

chfritz · 2021-05-28T03:25:35Z

ok, I got past that by upgrading the version of vscode-ripgrep to the latest. Now I get:

> ./out/atom-dev-1.59.0-dev-a3d4f9cdb-amd64/atom 
A JavaScript error occurred in the main process
Uncaught Exception:
Error: The module '/home/cfritz/github/_atom/out/atom-dev-1.59.0-dev-a3d4f9cdb-amd64/resources/app.asar.unpacked/node_modules/nslog/build/Release/nslog.node'
was compiled against a different Node.js version using
NODE_MODULE_VERSION 72. This version of Node.js requires
NODE_MODULE_VERSION 80. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or `npm install`).

sadick254 · 2021-05-28T14:26:47Z

Could you try running npx electron-rebuild -v 9.4.4.

- Four days of work to get those damn test to run again, just for a two character change to fix the tests. - Now also added all other, minor changes to sane-indendation over the last five years since the PR for this change was opened.

chfritz · 2021-05-31T03:21:49Z

@sadick254 : at long last I've been able to get atom to compile again (in a lxc container), migrate my changes (had to manually re-add all changes, since the package-locks seem to have been messing things up), and fixed the issue. The fix was trivial -- literally just removing a + 1 that became unnecessary with atom 1.40 -- ancient history! :-)

All tests are now passing.

Looking forward to having this PR finally get merged -- five years after making my first attempt to improve the indentation logic.

Thanks for your help and for picking this back up!

sadick254

This looks very good. It's working well as I expected. Thank you for the awesome work @chfritz.

This was referenced Oct 25, 2018

Adding configuration for new, tree-sitter based indentation logic atom/language-javascript#608

Open

Implement new indentation logic based on tree-sitter atom/language-javascript#594

Open

rsese added the more-information-needed label Oct 26, 2018

chfritz force-pushed the tree-sitter-indentation branch 2 times, most recently from b3e5448 to 98c7578 Compare November 2, 2018 03:47

rsese added work-in-progress and removed more-information-needed labels Nov 6, 2018

chfritz force-pushed the tree-sitter-indentation branch from 98c7578 to 2f321e4 Compare November 8, 2018 04:31

Aerijo mentioned this pull request Nov 26, 2018

Indentation for single-statement bodies inside "for", "while", and "if" atom/language-c#96

Open

chfritz force-pushed the tree-sitter-indentation branch 2 times, most recently from 33da130 to c547d98 Compare January 7, 2019 02:41

chfritz force-pushed the tree-sitter-indentation branch from c547d98 to 9e32f61 Compare January 7, 2019 05:30

chfritz force-pushed the tree-sitter-indentation branch from c8257b3 to 92550d0 Compare January 12, 2019 04:04

chfritz force-pushed the tree-sitter-indentation branch 2 times, most recently from f35e656 to 4979677 Compare January 12, 2019 04:49

chfritz force-pushed the tree-sitter-indentation branch 3 times, most recently from 798fd8a to 35677c5 Compare January 19, 2019 23:10

rsese mentioned this pull request May 30, 2019

Quote character in comment causes auto-indent error atom/language-javascript#648

Closed

Dreomite mentioned this pull request Aug 2, 2019

Native Syntax Highlighting onivim/oni2#161

Closed

FelipeLema mentioned this pull request Aug 13, 2020

Better and smart indentation for major modes emacs-tree-sitter/elisp-tree-sitter#34

Closed

theHamsta mentioned this pull request Aug 21, 2020

Use tree-sitter for indentation nvim-treesitter/nvim-treesitter#332

Closed

darangi added the triaged label Jan 25, 2021

sadick254 requested a review from maxbrunsfeld February 5, 2021 05:42

sadick254 force-pushed the tree-sitter-indentation branch 2 times, most recently from 30b6344 to 0f2e90a Compare May 20, 2021 19:38

chfritz force-pushed the tree-sitter-indentation branch 3 times, most recently from c964f1c to 10c4b7b Compare May 31, 2021 01:47

chfritz force-pushed the tree-sitter-indentation branch from 10c4b7b to 98f3082 Compare May 31, 2021 02:32

sadick254 approved these changes May 31, 2021

View reviewed changes

sadick254 merged commit 9826355 into atom:master May 31, 2021

sadick254 mentioned this pull request Jun 3, 2021

Better auto-indent for TextMate grammars #17866

Closed

renovate bot mentioned this pull request Jul 28, 2021

fix(package): fix integration test failure for atom-beta version v1.61.0-beta0 smhxx/atom-ts-transpiler#83

Closed

1 task

Implementing tree-sitter based indentation logic #18321

Implementing tree-sitter based indentation logic #18321

Uh oh!

Conversation

chfritz commented Oct 25, 2018

Issue or RFC Endorsed by Atom's Maintainers

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Uh oh!

chfritz commented Oct 25, 2018

Uh oh!

maxbrunsfeld commented Oct 25, 2018

Uh oh!

chfritz commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chfritz commented Jan 7, 2019

Uh oh!

chfritz commented Jan 12, 2019

Uh oh!

maxbrunsfeld commented Jan 12, 2019

Uh oh!

chfritz commented Jan 12, 2019

Uh oh!

lee-dohm commented Jan 15, 2019

Uh oh!

chfritz commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chfritz commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafeca commented May 31, 2019

Uh oh!

chfritz commented Sep 5, 2019

Uh oh!

chfritz commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chfritz commented Jan 6, 2021

Uh oh!

sadick254 commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chfritz commented May 21, 2021

Uh oh!

sadick254 commented May 21, 2021

Uh oh!

chfritz commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sadick254 commented May 26, 2021

Uh oh!

chfritz commented May 28, 2021

Uh oh!

chfritz commented May 28, 2021

Uh oh!

sadick254 commented May 28, 2021

Uh oh!

chfritz commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sadick254 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

chfritz commented Nov 6, 2018 •

edited

Loading

chfritz commented Jan 16, 2019 •

edited

Loading

chfritz commented Jan 20, 2019 •

edited

Loading

chfritz commented Sep 5, 2019 •

edited

Loading

sadick254 commented May 21, 2021 •

edited

Loading

chfritz commented May 25, 2021 •

edited

Loading

chfritz commented May 31, 2021 •

edited

Loading