Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

Project model #965

Closed
wants to merge 1 commit into from
Closed

Project model #965

wants to merge 1 commit into from

Conversation

matklad
Copy link
Member

@matklad matklad commented Jul 27, 2018

This is a super-WIP not ready for the review in any capacity.

EDIT: now this is in the minimal almost-mergable state, so that we can get rid of two Cargos and update RLS at rust-lang/rust

closes #956

Currently, this is basically a copy-paste from the super-simplified project model of Racer, but we really need something more full-featured. In partucular, we need a better understanding of targets and packages to correctly supply -p arguments to our code lenses when running tests.

@matklad
Copy link
Member Author

matklad commented Jul 27, 2018

The racer side of things is racer-rust/racer#910

@@ -904,6 +908,23 @@ impl RequestAction for CodeLensRequest {
}
}

fn racer_cache(vfs: Arc<Vfs>) -> racer::FileCache {
struct RacerVfs(Arc<Vfs>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you use wrapper struct here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can’t implement he trait directly on Vfs due to coherence.

This was previously in rls_vfs without wrapper, but
#964 changes that, so we can avoid bumping racer version in rls_vfs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks.

@kngwyu
Copy link
Contributor

kngwyu commented Jul 29, 2018

Released racer 2.1.3 with racer-rust/racer#910

@Xanewok
Copy link
Member

Xanewok commented Jul 29, 2018

@matklad could you rebase, please?

So I'm not entirely sure why we need to keep 2 copies of the code here and in Racer and also how this specifically allows us to not depend on Cargo twice. Mind elaborating on this a bit more? Does the Racer 2.1.3 make it possible for it not to require Cargo but instead have its project information injected (possibly by RLS in this case) or how will that work?

@matklad
Copy link
Member Author

matklad commented Jul 29, 2018

Sure, sorry for not writing a reasonable description of the PR in the first place: I initially planed to work on it quite a bit more before writing a proper motivation, but then that update of RLS in rust-lang/rust happed, and I tried to rush it because I thought that rust-lang's CI won't handle two copies of Cargo. But since that PR landed, here's a more composed description of what this PR does, and how future work might look like.

First of all, how does this help with two Cargo's? Since the latest release, racer-the-library does not depend on Cargo at all, it only needs a racer::ProjectModelProvider instance. Racer-the-binary however depends on Cargo, and uses it to implement ProjectModelProvider.

When using racer from RLS, we should feed it our own idea of the project model, thus ensuring that RLS is a single source of truth for racer. Now, we don't have a project model per se in RLS, but I argue that we need it already and that we'll certainly need it in the future.

We have a build plan, but that's not exactly a project model (according to matklad's definition of it :) ). Build plan is dynamic, and depends on the particular set of features and subsets of packages and targets you are using. Project model is static, and only depends on Cargo.toml and Cargo.lock (modulo autodiscovered crate roots), and is basically what cargo metadata gives you.

So, if we had a project model in the form of set of packages and dependencies between them in RLS, we would be able to correctly answer the questions like "which package does this file belong to" (which we need in Code lens and build plan) and "which packages depend on this package" (which we also need in the build plan).

Now, this PR does not actually gives the full project model, and instead uses the minimal amount of code to make the Racer work, which naturally coincides with what is written the the racer's binary case.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to land now (modulo the extra comment). However, longer term we should cache the project model and update it when things change (we already watch Cargo.toml and .lock).

/// This module should represent the RLS view of the Cargo project model.
/// At the time of writing though it is very limited, does not contain
/// the project model per se and implements the minimal amount of functionality
/// required to make the racer work
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what a project model is and why we/racer needs one please?

@matklad
Copy link
Member Author

matklad commented Aug 6, 2018

Closed in favor of #981

@matklad matklad closed this Aug 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RLS links Cargo twice
4 participants