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

Provide built-in Rust support using Tree-sitter #18257

Merged
merged 1 commit into from Oct 19, 2018

Conversation

Projects
None yet
10 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Oct 18, 2018

Description of the Change

This PR adds built-in support for the Rust programming language using Atom's new parsing system, Tree-sitter. Tree-sitter will be enabled by default starting in Atom 1.32.

By using Tree-sitter, we can provide more consistent and performant syntax highlighting than before. Rust users should notice that types, fields, and enum variants should be distinguished more clearly and consistently than before. Syntax highlighting large source files is also much faster with Tree-sitter.

This code used to live at https://github.com/atom/language-rust, and I'm moving it into this repo.

Alternate Designs

Atom does not need to support Rust out of the box; we could continue to require Rust users to install a separate package. The reasons that I think it's appropriate for us to bundle the package are:

  • Rust is now more popular than some of the other languages that we support out of the box
  • I want Rust users to get the benefits of Tree-sitter
  • I helped write the Rust parser (along with @MaximSokolov), so I am able to maintain it

Possible Drawbacks

Bundle Size - The new bundled package language-rust-bundled contains very little besides a dependency on tree-sitter-rust, but that parser does consist of a 2MB binary. That binary gzips down to 168k, so this PR will increase the download size of Atom by 168k.

Maintenance - There is already a popular package for Atom that provides Rust support - language-rust. Supporting Rust out-of-the-box means that Atom's core maintainers will now be responsible for maintaining this functionality.

Luckily, there is very little code in our package. The parser is fairly well tested so I don't think maintaining it should be too much work. I don't plan on adding a wide variety of snippets and things like that to the package. Those can continue to be provided by external packages.

Verification Process

Edit rust code using all of the built-in themes.

/cc @miqh because you maintain language-rust
/cc @mehcode @alexheretic because you work on ide-rust
/cc @nathansobo @as-cii

@maxbrunsfeld maxbrunsfeld changed the title from Provide built-in rust support using Tree-sitter to Provide built-in Rust support using Tree-sitter Oct 18, 2018

@nathansobo

This comment has been minimized.

Contributor

nathansobo commented Oct 18, 2018

⚡️⚡️

@miqh

This comment has been minimized.

miqh commented Oct 18, 2018

@maxbrunsfeld, thank you for the tag, but the language-rust package on apm is actually belongs to @zargony (https://github.com/zargony/atom-language-rust), not me. 🙂

In any case, Tree-sitter sounds like a great and sane alternative to TextMate grammars for providing semantic syntax highlighting. Only read about it now as I've been out of the Atom ecosystem for a while now.

@thomasjo

This comment has been minimized.

Member

thomasjo commented Oct 18, 2018

My only gripe with this is the unfortunate package name — language-rust-bundled. Maybe @zargony is open to transferring the language-rust name to @atom?

If not, I really think we should call it something else. Even language-rust-, language-rusty, or any such is better in my opinion. The bundled suffix introduces questions like "Hmm, are these other language packages not bundled? Why are they not suffixed?"

@Aerijo

This comment has been minimized.

Contributor

Aerijo commented Oct 18, 2018

@thomasjo I agree: let’s rename everything /s

While we’re at it, let’s also knock the plus off of autocomplete-plus. I believe it used to be some community package alternative of the original autocomplete, but the name is just confusing these days. What is it “plus”ing?

@alexheretic alexheretic referenced this pull request Oct 18, 2018

Open

Need a hand? #119

@zargony

This comment has been minimized.

zargony commented Oct 18, 2018

Basically, I’d be happy to transfer the name to @atom. I created language-rust to have basic Rust support in Atom and it seems to have gained a lot of popularity since then. Unfortunately I‘m currently lacking time to maintain it properly. There are only a few real issues, but my knowledge on Atom internals is too limited to fix them easily. So if Rust support gets better (probably faster and better maintained) with tree-sitter, I‘d be happy to transfer the name to you.
My main concern is about dependencies. I’m not sure what other apm packages may depend on language-rust, but if there are any, we should make sure to not break them, i.e. provide the new package with a new major version and keep providing the old package for dependants which need it.
I‘m mentioning this, because language-rust is also published to npm and used by some bigger projects, as far as I know e.g. by VSCode and GitHub (although they may have forked it because they don’t seem to be listed as dependants). So I of course would like to make sure that we don’t cause trouble to these projects.

(I‘m currently on vacations with bad connectivity, so sorry for maybe slow responses)

@miqh

This comment has been minimized.

miqh commented Oct 18, 2018

The Rust extension distributed with VS Code pulls the TextMate grammar file direct from https://github.com/zargony/atom-language-rust as part of their build.

https://github.com/Microsoft/vscode/blob/5e12e53497ade2a92760b155bc790eac6ad3bd39/extensions/rust/package.json#L9

Even if the language-rust package name on apm was ceded to this new Tree-sitter implementation, I don't think it should matter in this scenario.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 18, 2018

I agree the name language-rust-bundled isn't ideal, but I'm worried it would be confusing to change that name to refer to a different package right away.

Things I'm specifically concerned about:

  • Most Rust users will already have language-rust installed. When we ship this change, will they then have two packages named language-rust? I don't want to require them to uninstall language-rust to see this new highlighting functionality.
  • Atom currently still highlights Markdown using the TextMate system, so rust code embedded in markdown will not yet be highlighted by this built-in package. Users will still need language-rust for the time being if they want rust inside of markdown to be highlighted.
  • language-rust provides snippets. I didn't take those into this built-in package, because different people have different opinions about snippets and I'm hesitant to incorporate more of them into core.
@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 18, 2018

I'm in favor of eventually renaming the package to language-rust, but I feel like it might be confusing to take that name over right now.

What about instead of language-rust-bundled, we call it language-rust-plus for now? That appears to not be taken.

/cc @lee-dohm - thoughts?

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 18, 2018

because different people have different opinions about snippets and I'm hesitant to incorporate more of them into core.

Yesterday we merged PRs to add the ability to disable snippets from the settings-view 🙂 So if peoples opinions differ it's much easier for them to disable and make their own.

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Oct 18, 2018

@maxbrunsfeld I think language-rust-plus is fine. If the original language-rust package is transferred to the atom organization and the tree-sitter functionality is added to it, then everyone will just be brought along for the ride and they won't have two language-rust packages installed. That might be the ideal solution if @zargony is open to that.

@Arcanemagus

This comment has been minimized.

Contributor

Arcanemagus commented Oct 18, 2018

I think the concern is users that currently have language-rust installed. If we used the name for the package bundled within Atom then they would have their existing install in ~/.atom/packages as well as the one bundled within Atom (when it gets released).

At least dalek would start yelling at them so they would know something had changed 😆.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 18, 2018

Oh nevermind, the name language-rust-plus is actually taken too 😬 .

I still think that the simplest thing to do for now is just to keep the unique name language-rust-bundled. The name doesn't matter too much, and down the road, after people have started using the new package, we can maybe think about merging with language-rust.

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Oct 19, 2018

If we used the name for the package bundled within Atom then they would have their existing install in ~/.atom/packages as well as the one bundled within Atom (when it gets released).

At least dalek would start yelling at them so they would know something had changed 😆.

Yep, great point @Arcanemagus ... thanks for helping me with that 😀

@maxbrunsfeld maxbrunsfeld merged commit e82629c into master Oct 19, 2018

3 checks passed

Atom Pull Requests #20181018.2 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-bundle-rust-grammar branch Oct 19, 2018

@mmstick

This comment has been minimized.

mmstick commented Oct 24, 2018

This was a huge improvement in the quality of syntax highlighting for Rust. It's now superior to what VS Code currently offers. A comparison:

screenshot from 2018-10-24 12-15-41

screenshot from 2018-10-24 16-56-31

@mmstick

This comment has been minimized.

mmstick commented Oct 26, 2018

Found a scenario where the syntax highlighting breaks:

static BLACKLIST_NVIDIA: &[u8] = br#"# Automatically generated by distinst
blacklist nouveau
blacklist nvidia
blacklist nvidia-drm
blacklist nvidia-modeset
alias nouveau off
alias nvidia off
alias nvidia-drm off
alias nvidia-modeset off
"#;

/// Disables external graphics if switchable graphics is supported.
pub fn disable_external_graphics(mount_dir: &Path) -> io::Result<bool> {
    if let Ok(modules) = Module::all() {
        let product_version = &*product_version();
        let disable_nvidia = has_switchable_graphics(product_version)
            && modules.iter().any(|x| &x.name == "nvidia" || &x.name == "nouveau");

        if disable_nvidia {
            info!("disabling external NVIDIA graphics by default");
            let _ = fs::create_dir_all(mount_dir.join("etc/modprobe.d/"));
            OpenOptions::new()
                .create(true)
                .write(true)
                .truncate(true)
                .open(mount_dir.join(POWER))
                .and_then(|mut file| file.write_all(BLACKLIST_NVIDIA))?;
            return Ok(true);
        }
    }

    Ok(false)
}

Seems to get tripped up by br#"#. It handles r#"# properly.

@Arcanemagus

This comment has been minimized.

Contributor

Arcanemagus commented Oct 26, 2018

Could you file that as a new issue @mmstick so we can track it there properly? Thanks!

maxbrunsfeld added a commit to tree-sitter/tree-sitter-rust that referenced this pull request Oct 26, 2018

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 26, 2018

Thanks for the report @mmstick! I wasn't aware of that syntax. Fixed in tree-sitter/tree-sitter-rust@020b80e.

@nathansobo

This comment has been minimized.

Contributor

nathansobo commented Oct 26, 2018

Damn. That's some turnaround time.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 28, 2018

The fix will ship this coming week in 1.33.0-beta1.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Oct 31, 2018

The fix is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment