Infer appropriate crate target from workspace by Xanewok · Pull Request #438 · rust-lang/rls

Xanewok · 2017-08-10T15:51:46Z

This is only a proof-of-concept and needs some ironing out. Most importantly it'd be good to handle properly inferred/default and explicitly stated values when updating the config via LSP message. Additionally, it'd be good not to duplicate code that creates a Workspace, although I'm not really sure how to do that, as Workspace needs to be created with borrows and I'm not sure how to cleanly separate a struct that would hold the data and Workspace without running into self-borrowing issues when initializing.

Right now it works like this: On RLS initialize, call infer_config_defaults which creates a Workspace and queries appropriate package's targets to see which crate targets are available (including inferred lib and bin ones). Then, these are set as follows: prioritize lib over bin, and prefer default bin over a first bin on the list. These are just mindlessly copied into current config and during config change, we just ignore those (which we shouldn't, it needs work).

Everything marked with TODO: I'd like to resolve first before merging.

nrc · 2017-08-11T05:43:45Z

So for inferred/default stuff, here are some thoughts: just to make sure, let's only do this where we need to (which is build_lib/bin for now, I think). I think then we want to split the config struct so we have a RawConfig which we can deserialise from LSP and then the Config we use internally. The raw one doesn't have the inferred/whatever thing. Then use a generic enum for build_bin, etc. - I think you need Specified/Inferred/None (where the last is used when we haven't inferred yet, or can't be inferred), then have an update method which takes the supplied input. This works for build_bin, but not build_lib, I think. You'll need to change the type of build_lib to Option and change the default in VSCode to null. Otherwise you don't know whether the user is using the default or not.

nrc

I think all the Cargo stuff looks fine, and generally this is a good approach, I think the only thing that needs work is the Config for Inferred vs Default and merging configs.

nrc · 2017-08-11T06:08:00Z

src/actions/mod.rs

    pub fn init<O: Output>(&self, init_options: InitializationOptions, out: O) {
        trace!("init: {:?}", init_options);

+        if let Err(err) = self.infer_config_defaults(&mut *self.config.lock().unwrap()) {


Can we spawn a thread to do this, rather than doing it on the main thread?

I've tried to catch every Err and surface it out so we won't get a panic there (updated now, got rid of the .expect() and .unwrap()). Should I still spawn a new thread for that here?

So my concern was that this might take a long time to run and block other main thread options, rather than about error handling. Given that we're doing a bunch of disk access, I think it is probably still too long running a task to be on the main thread.

Xanewok · 2017-08-13T14:35:59Z

Updated, added new test cases for inferring crate target types. Introduced Inferrable::{Specified(T), Inferred(T), None as per comment.
One minor caveat is that it'd be good to run infer_config_defaults when specifying options back to None via config, as these won't be downgraded from Specified variant with current implementation. Should I add that?

nrc

Looks good! A couple of nits inline.

nrc · 2017-08-14T00:11:06Z

src/config.rs

+}
+
+/// Deserialize as if it's `Option<T>` and use `None` variant if it's `None`,
+/// otherwise use `Specified` variant for deserialized value.


nit: I think this should be a regular comment, not a doc comment

nrc · 2017-08-14T00:11:55Z

src/config.rs

+        match self {
+            &Inferrable::Inferred(ref value) |
+            &Inferrable::Specified(ref value) => value,
+            &Inferrable::None => unreachable!(),


Should probably expect here so we get a useful message

In fact, I wonder that we might hit this in real life - either because inference has not completed yet, or the user resets to null. In which case we should probably be able to continue without crashing - either just skip the build or build with some defaults or something

Currently it's impossible to reset the config value to None, it's only a way to not get Specified value when deserializing. However if user desires to fall back to auto-detecting, we'd still have to run the infer_... again after config change, which isn't done yet.
I made an assumption, that all values (including default ones, which I set to Inferred) should not ever be None, but I have a hard time trying to enforce that at compile-time. Splitting the config into deserialized/actually used would work, but we'd lose compile-time guarantee that we copy every value and would have to duplicate a lot of code or write less straightforward macro, that's why I went with this approach. How should I tackle this?

nrc · 2017-08-14T00:30:06Z

One minor caveat is that it'd be good to run infer_config_defaults when specifying options back to None via config, as these won't be downgraded from Specified variant with current implementation. Should I add that?

Yes please.

On RLS initialize, call `infer_config_defaults` which creates a `Workspace` and queries appropriate package's targets to see which crate targets are available (including inferred `lib` and `bin` ones). Whenever received changed config has some `null` values (most probably the user wants to use inferred values again), set them to the default values and run `infer_config_defaults` later as well.

Xanewok · 2017-08-14T23:51:34Z

src/actions/mod.rs

    }
 }

+fn infer_config_defaults(project_dir: &Path, config: &mut Config) -> CargoResult<()> {


Should this be in an impl Config? This would need more Cargo-related bits moved to config.rs, not sure if it's good from in terms of the module split/architecture

Yeah, I think I would expect this to be in the cargo module and probably a method on Config. Taking a mut ref to Config is a bit strange, I guess I would expect to take a Config and return a new one? And that might make some of the referencing calling this easier? Anyway, I'm going to merge this now, we can refactor later if you like because I think this is polish.

Xanewok · 2017-08-14T23:51:51Z

Addressed every point, rebased and force-pushed now. Is spawning a thread here and here is okay?

Xanewok · 2017-08-15T06:48:34Z

I guess it'd be also good to introduce a test case to detect inference user sends a DidChangeConfiguration with a null option, I can do that a bit later.
Thanks for merging!

nrc reviewed Aug 11, 2017

View reviewed changes

Xanewok force-pushed the infer-targets branch 2 times, most recently from 8172909 to 86d1ab3 Compare August 13, 2017 14:29

Xanewok mentioned this pull request Aug 13, 2017

Allow for build_lib config value to be null rust-lang/vscode-rust#121

Merged

nrc reviewed Aug 14, 2017

View reviewed changes

Xanewok added 2 commits August 15, 2017 01:43

Add test cases for inferring crate target types

5f5e64a

Xanewok force-pushed the infer-targets branch from 86d1ab3 to 5f5e64a Compare August 14, 2017 23:47

Xanewok commented Aug 14, 2017

View reviewed changes

nrc merged commit 2e5ba48 into rust-lang:master Aug 15, 2017

Xanewok deleted the infer-targets branch August 15, 2017 06:49

Xanewok mentioned this pull request Oct 30, 2017

Don't require the build_lib flag #332

Closed

Conversation

Xanewok commented Aug 10, 2017

Uh oh!

nrc commented Aug 11, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Aug 13, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented Aug 14, 2017

Uh oh!

Xanewok Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xanewok commented Aug 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Xanewok Aug 14, 2017 •

edited

Loading

Xanewok commented Aug 14, 2017 •

edited

Loading