Skip to content
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

fix: run flycheck without rev_deps when target is specified #17912

Merged
merged 2 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions crates/rust-analyzer/src/flycheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ pub(crate) struct CargoOptions {
pub(crate) target_dir: Option<Utf8PathBuf>,
}

#[derive(Clone)]
pub(crate) enum Target {
Bin(String),
Example(String),
Benchmark(String),
Test(String),
}

impl CargoOptions {
pub(crate) fn apply_on_command(&self, cmd: &mut Command) {
for target in &self.target_triples {
Expand Down Expand Up @@ -119,13 +127,13 @@ impl FlycheckHandle {

/// Schedule a re-start of the cargo check worker to do a workspace wide check.
pub(crate) fn restart_workspace(&self, saved_file: Option<AbsPathBuf>) {
self.sender.send(StateChange::Restart { package: None, saved_file }).unwrap();
self.sender.send(StateChange::Restart { package: None, saved_file, target: None }).unwrap();
}

/// Schedule a re-start of the cargo check worker to do a package wide check.
pub(crate) fn restart_for_package(&self, package: String) {
pub(crate) fn restart_for_package(&self, package: String, target: Option<Target>) {
self.sender
.send(StateChange::Restart { package: Some(package), saved_file: None })
.send(StateChange::Restart { package: Some(package), saved_file: None, target })
.unwrap();
}

Expand Down Expand Up @@ -183,7 +191,7 @@ pub(crate) enum Progress {
}

enum StateChange {
Restart { package: Option<String>, saved_file: Option<AbsPathBuf> },
Restart { package: Option<String>, saved_file: Option<AbsPathBuf>, target: Option<Target> },
Cancel,
}

Expand Down Expand Up @@ -271,7 +279,7 @@ impl FlycheckActor {
tracing::debug!(flycheck_id = self.id, "flycheck cancelled");
self.cancel_check_process();
}
Event::RequestStateChange(StateChange::Restart { package, saved_file }) => {
Event::RequestStateChange(StateChange::Restart { package, saved_file, target }) => {
// Cancel the previously spawned process
self.cancel_check_process();
while let Ok(restart) = inbox.recv_timeout(Duration::from_millis(50)) {
Expand All @@ -281,11 +289,12 @@ impl FlycheckActor {
}
}

let command =
match self.check_command(package.as_deref(), saved_file.as_deref()) {
Some(c) => c,
None => continue,
};
let Some(command) =
self.check_command(package.as_deref(), saved_file.as_deref(), target)
else {
continue;
};

let formatted_command = format!("{command:?}");

tracing::debug!(?command, "will restart flycheck");
Expand Down Expand Up @@ -381,6 +390,7 @@ impl FlycheckActor {
&self,
package: Option<&str>,
saved_file: Option<&AbsPath>,
target: Option<Target>,
) -> Option<Command> {
match &self.config {
FlycheckConfig::CargoCommand { command, options, ansi_color_output } => {
Expand All @@ -396,6 +406,15 @@ impl FlycheckActor {
None => cmd.arg("--workspace"),
};

if let Some(tgt) = target {
match tgt {
Target::Bin(tgt) => cmd.arg("--bin").arg(tgt),
Target::Example(tgt) => cmd.arg("--example").arg(tgt),
Target::Test(tgt) => cmd.arg("--test").arg(tgt),
Target::Benchmark(tgt) => cmd.arg("--bench").arg(tgt),
};
}

cmd.arg(if *ansi_color_output {
"--message-format=json-diagnostic-rendered-ansi"
} else {
Expand Down
52 changes: 40 additions & 12 deletions crates/rust-analyzer/src/handlers/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ use vfs::{AbsPathBuf, ChangeKind, VfsPath};

use crate::{
config::{Config, ConfigChange},
flycheck::Target,
global_state::{FetchWorkspaceRequest, GlobalState},
lsp::{from_proto, utils::apply_document_changes},
lsp_ext::{self, RunFlycheckParams},
mem_docs::DocumentData,
reload,
target_spec::TargetSpec,
};

pub(crate) fn handle_cancel(state: &mut GlobalState, params: CancelParams) -> anyhow::Result<()> {
Expand Down Expand Up @@ -287,16 +289,40 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
let world = state.snapshot();
let mut updated = false;
let task = move || -> std::result::Result<(), ide::Cancelled> {
// Trigger flychecks for all workspaces that depend on the saved file
// Crates containing or depending on the saved file
let crate_ids: Vec<_> = world
.analysis
.crates_for(file_id)?
.into_iter()
.flat_map(|id| world.analysis.transitive_rev_deps(id))
.flatten()
.unique()
.collect();
// Is the target binary? If so we let flycheck run only for the workspace that contains the crate.
let target = TargetSpec::for_file(&world, file_id)?.and_then(|x| {
let tgt_kind = x.target_kind();
let tgt_name = match x {
TargetSpec::Cargo(c) => c.target,
TargetSpec::ProjectJson(p) => p.label,
};

let tgt = match tgt_kind {
project_model::TargetKind::Bin => Target::Bin(tgt_name),
project_model::TargetKind::Example => Target::Example(tgt_name),
project_model::TargetKind::Test => Target::Test(tgt_name),
project_model::TargetKind::Bench => Target::Benchmark(tgt_name),
_ => return None,
};

Some(tgt)
});

let crate_ids = if target.is_some() {
// Trigger flychecks for the only workspace which the binary crate belongs to
world.analysis.crates_for(file_id)?.into_iter().unique().collect::<Vec<_>>()
Comment on lines +312 to +313
Copy link
Member

Choose a reason for hiding this comment

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

We should already have that information in the TargetSpec, it should contain the relevant crate id

Copy link
Member Author

@alibektas alibektas Aug 22, 2024

Choose a reason for hiding this comment

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

this is the only bit I couldn't really come up with a solution to because the package information for RustProjectJson is not really clear.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, #17946 the data was there but not exposed that's all

} else {
// Trigger flychecks for all workspaces that depend on the saved file
// Crates containing or depending on the saved file
world
.analysis
.crates_for(file_id)?
.into_iter()
.flat_map(|id| world.analysis.transitive_rev_deps(id))
.flatten()
.unique()
.collect::<Vec<_>>()
};

let crate_root_paths: Vec<_> = crate_ids
.iter()
Expand Down Expand Up @@ -346,8 +372,10 @@ fn run_flycheck(state: &mut GlobalState, vfs_path: VfsPath) -> bool {
for (id, package) in workspace_ids.clone() {
if id == flycheck.id() {
updated = true;
match package.filter(|_| !world.config.flycheck_workspace()) {
Some(package) => flycheck.restart_for_package(package),
match package
.filter(|_| !world.config.flycheck_workspace() || target.is_some())
{
Some(package) => flycheck.restart_for_package(package, target.clone()),
None => flycheck.restart_workspace(saved_file.clone()),
}
continue;
Expand Down