Add support for Rust #1606

Xanewok · 2018-08-31T20:49:38Z

This sets up preliminary support for Rust in Buck.
It uses the Rust Language Server (RLS) LSP server to provide necessary IDE features.

Since RLS and Cargo (the Rust package manager) don't support custom build systems out of box yet, this uses a hacky workaround for the moment. The user has to explicitly build a Rust target in Buck using the Buck toolbar - this in turn executes the #save-analysis flavor build and passes location of the resulting JSON save-analysis (index) files to the RLS. For this, the files should be saved first, before executing the build.

Until rust-lang/rust#53870 lands and the updated RLS will ship with a Rust toolchain, it has to be built manually using recent Rust nightly compiler and to have its executable pointed at by the user.

Currently the workflow is fragile - if the users starts editing the files without the explicit build first, the RLS can crash because how much it's currently tied to Cargo. This should be fixed when rust-lang/rls#1026 is implemented (at least stop it from crashing).

cc @jsgf @mostafaeweda @sunshowers

facebook-github-bot · 2018-08-31T20:49:48Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Xanewok · 2018-08-31T20:50:23Z

pkg/nuclide-rust/lib/BuckIntegration.js

+  const buildTarget = normalizeNameForBuckQuery(task.buildTarget);
+
+  // TODO: Filter by known Rust build targets
+  const files = await getRustInputs(task.buckRoot, buildTarget);


This still requires explicit filtering for rust_* target kinds.

please do this filtering to avoid calling the expensive buck query unless we're sure this is a rust target & this call will have value.

Done in 8d4772f. Forgot to remove the comment, sorry!

Xanewok · 2018-09-03T15:01:20Z

@jsgf I added a disclaimer (b948e1f) that warns the user to run the Buck build in a Rust project first. Is that desirable/okay for now?

jsgf · 2018-09-04T22:02:15Z

@Xanewok I think its OK as a placeholder. I hope we can get something more elegant in the longer term though.

Xanewok · 2018-09-05T07:40:16Z

I hope we can get something more elegant in the longer term though.

Definitely, I am conviced this will not be an issue anymore once the RLS natively supports custom build systems.

Update: rust-lang/rust#53870 has been merged and the newest Rust nightly (2018-09-04) contains the RLS binary supporting external simple build systems (needed for this) out of box, as well as external Rustfmt.

mostafaeweda · 2018-09-05T18:40:04Z

pkg/nuclide-rust/lib/BuckIntegration.js

+  const buildTarget = normalizeNameForBuckQuery(task.buildTarget);
+
+  // TODO: Filter by known Rust build targets
+  const files = await getRustInputs(task.buckRoot, buildTarget);


please do this filtering to avoid calling the expensive buck query unless we're sure this is a rust target & this call will have value.

mostafaeweda · 2018-09-05T18:42:38Z

pkg/nuclide-rust/lib/BuckUtils.js

+  );
+}
+
+export function getSaveAnalysisTargets(


change to use async / await :

export async function getSaveAnalysisTargets( ... const deps = await BuckService.query(buckRoot, query, []); return deps.map(dep => dep + '#save-analysis'); }

mostafaeweda · 2018-09-05T18:46:42Z

pkg/nuclide-rust/lib/BuckIntegration.js

+  logger.debug(`analysisTargets: ${analysisTargets.join('\n')}`);
+  const artifacts: Array<string> = [];
+
+  const buildReport = await BuckService.build(task.buckRoot, analysisTargets);


how long does this typically take for large rust targets? do we need to report its progress?

It depends; using cargo check as example, complete check build can even take a minute or two; rechecking the leaf only shouldn't take longer than a couple of seconds. Third party deps are vendored and pre-compiled from what I understand, so this should also further cut down compilation time. @jsgf would know more about how long exactly these #save-analysis builds take in the Buck environment.

It would certainly be good to report the progress - do you know if it's possible to pass a callback or report the progress gracefully using the Buck API here?

Changed this to display a busy spinner indicator while the save-analysis build executes.

mostafaeweda · 2018-09-05T18:50:47Z

pkg/nuclide-rust/lib/BuckIntegration.js

+  const fileUri = task.buckRoot + '/' + files[0];
+  logger.debug(`fileUri: ${fileUri}`);
+
+  const langService = await service.getLanguageServiceForUri(fileUri);


There's an invalid assumption here that the multi-project language service will only have one LSP server that we need to update its build command (while in reality there can be multiple buck roots, each having a different LSP server -- according to the project files config below).

Do you need to getAllLanguageServices or observeLanguageServices?
Can you test with multiple buck roots to see if things work properly?

I understood that MultiLspLanguageService spawn separate LSP server per appropriate 'top-level' project config files like .hhconfig and that calling this seems like a good way to retrieve a handle to appropriate LSP server that's responsible for the fileUri we're building.

However, I did assume there's going to be a single Buck root; I'll see if it works with multiple ones.

@mostafaeweda sorry it took so long! Just tested this with multiple buck roots, using https://github.com/Xanewok/rust-buck-skeleton and building both //server:server and nested//:nested. This correctly retrieved the LSP server at root dir and nested/, respectively, so this looks good so far =)

mostafaeweda · 2018-09-05T18:51:47Z

pkg/nuclide-rust/lib/main.js

+
+class Activation {
+  _rustLanguageService: AtomLanguageService<LanguageService>;
+  _buckTaskRunnerService: ?BuckTaskRunnerService;


we don't seem to need this to be an instance variable, right?

mostafaeweda · 2018-09-05T18:52:17Z

pkg/nuclide-rust/lib/main.js

+  }
+
+  consumeBuckTaskRunner(service: BuckTaskRunnerService): IDisposable {
+    service.onDidCompleteTask(task =>


that's a non disposed registration, maybe this function should just:

return service.onDidCompleteTask...

Also consume Buck task service to later rebuild index on save

After hitting Buck build button, the relevant save-analysis Buck build is performed and RLS is supplied with a dummy build command to retrieve necessary save-analysis indexing files. Things left to do/worth noting: * We should detect if Buck is an appropriate build system for a given LSP service and if not, not override RLS' external build command * Until path remapping lands in rustc (rust-lang/rust#53110) the index-based features (find refs, goto def etc.) only work for source files in the buck-out/**/$TARGET#save-analysis-container source files. * RLS needs some bugfixin' since currently external build command does not properly refresh file dirty state, leading to an endless build loop.

For example, running `buck build cell//:target` will create output with paths relative to the cell, and not the current Buck root.

I could've sworn it worked with relative directories - it boiled down to ```js const buildFile = ...; // relative return fsPromise.findNearestFile('.buckconfig', buildFile); ``` which *should* work for relative files, I believe?

Xanewok · 2018-09-14T22:16:17Z

@mostafaeweda @jsgf Rebased and pushed two changes: one being displaying busy indicator while the Buck builds the save-analysis build and the other one working around a Buck bug (9f64faf), where outputs are relative to the cell, even when these are executed from the outer Buck root.

Does this still need more work or can we land it as the 1st increment?

mostafaeweda

This seems ready to roll 🥇(the only comment I have here is trying to avoid unnecessary buck queries if the build task completed isn't actually a rust target).

@jsgf to land this diff, you'd need to click 'Import to Phabricator' and once landed internally (source of truth), in 5 minutes, it'll sync the changes to github.

pkg/nuclide-rust/lib/BuckIntegration.js

facebook-github-bot · 2018-10-02T13:09:16Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Xanewok · 2018-10-02T13:32:26Z

@jsgf signed the CLA here, just in case.

facebook-github-bot

mostafaeweda has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Xanewok commented Aug 31, 2018

View reviewed changes

Xanewok changed the title ~~WIP: Add support for Rust~~ Add support for Rust Sep 3, 2018

mostafaeweda suggested changes Sep 5, 2018

View reviewed changes

Xanewok added 16 commits September 14, 2018 20:53

Add Rust language service via nuclide-rust

b4dc64b

Run eslint

2cd3106

Clean up and add more LSP capabilities

0809729

Clean up and use createPackage format

a446661

Also consume Buck task service to later rebuild index on save

Allow to override RLS path

6722738

Adjust after API change and prep for PR

15d4b3c

Fix issues reported by ESLint

9d6297d

...and for main.js as well

4435578

Don't use invariant nor assert

798fed2

Ignore Buck non-Rust build targets

ab2bc89

Add startup disclaimer

ca76f6c

Address feedback

9ce74be

Work around Buck bug with outputs relative to Buck cell

9f64faf

For example, running `buck build cell//:target` will create output with paths relative to the cell, and not the current Buck root.

Fix fetching Buck root for a build target

8797da0

I could've sworn it worked with relative directories - it boiled down to ```js const buildFile = ...; // relative return fsPromise.findNearestFile('.buckconfig', buildFile); ``` which *should* work for relative files, I believe?

Display busy spinner when running the indexing build

384c616

Xanewok force-pushed the nuclide-rust branch from cb2ee59 to 384c616 Compare September 14, 2018 21:40

mostafaeweda reviewed Sep 26, 2018

View reviewed changes

pkg/nuclide-rust/lib/BuckIntegration.js Show resolved Hide resolved

facebook-github-bot added the CLA Signed label Oct 2, 2018

Add code action capabilities

2779b30

Check for Rust rule before doing Buck query

7b9c326

mostafaeweda approved these changes Oct 4, 2018

View reviewed changes

facebook-github-bot reviewed Oct 4, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Rust #1606

Add support for Rust #1606

Xanewok commented Aug 31, 2018

facebook-github-bot commented Aug 31, 2018

Xanewok Aug 31, 2018

mostafaeweda Sep 5, 2018

Xanewok Sep 6, 2018

Xanewok commented Sep 3, 2018

jsgf commented Sep 4, 2018

Xanewok commented Sep 5, 2018

mostafaeweda Sep 5, 2018

mostafaeweda Sep 5, 2018

mostafaeweda Sep 5, 2018

Xanewok Sep 6, 2018

Xanewok Sep 14, 2018

mostafaeweda Sep 5, 2018

Xanewok Sep 6, 2018

Xanewok Sep 14, 2018

mostafaeweda Sep 5, 2018

mostafaeweda Sep 5, 2018

Xanewok commented Sep 14, 2018

mostafaeweda left a comment

facebook-github-bot commented Oct 2, 2018

Xanewok commented Oct 2, 2018

facebook-github-bot left a comment

Add support for Rust #1606

Are you sure you want to change the base?

Add support for Rust #1606

Conversation

Xanewok commented Aug 31, 2018

facebook-github-bot commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Sep 3, 2018

jsgf commented Sep 4, 2018

Xanewok commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Sep 14, 2018

mostafaeweda left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 2, 2018

Xanewok commented Oct 2, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment