From 9dd21e74a4a18850af0cbeead6089c4637b011ee Mon Sep 17 00:00:00 2001 From: Marlon Mata-Madriz Date: Fri, 20 Dec 2024 13:52:22 -0800 Subject: [PATCH 1/3] Suppress request after analysis done --- example_files/example_lint_cfg.README | 5 ++-- example_files/example_lint_cfg.json | 2 +- src/actions/analysis_storage.rs | 5 +++- src/actions/mod.rs | 19 +++++++++++---- src/dfa/client.rs | 15 +++--------- src/lint/mod.rs | 4 ++-- src/server/mod.rs | 34 +++++++++++++++++++-------- 7 files changed, 52 insertions(+), 32 deletions(-) diff --git a/example_files/example_lint_cfg.README b/example_files/example_lint_cfg.README index 2289e2b..c1bbf15 100644 --- a/example_files/example_lint_cfg.README +++ b/example_files/example_lint_cfg.README @@ -9,8 +9,9 @@ // your modifications { - // if set to 'false', will lint indirectly imported files as well - "direct_only": true, + // if set to 'false', will parse and lint imported files as well + // inteded for use with dfa CLI client to lint single files + "cli_mode": true, // linting rules with no arguments are enabled by assigning an empty struct "sp_brace": {}, // linting rules with arguments are straightforward diff --git a/example_files/example_lint_cfg.json b/example_files/example_lint_cfg.json index cfb5fee..51f367d 100644 --- a/example_files/example_lint_cfg.json +++ b/example_files/example_lint_cfg.json @@ -1,5 +1,5 @@ { - "direct_only": true, + "cli_mode": false, "sp_brace": {}, "sp_punct": {}, "nsp_funpar": {}, diff --git a/src/actions/analysis_storage.rs b/src/actions/analysis_storage.rs index 636e84c..080961d 100644 --- a/src/actions/analysis_storage.rs +++ b/src/actions/analysis_storage.rs @@ -627,7 +627,10 @@ impl AnalysisStorage { false } } - + // TODO: Might need to separate responsibilities here among the 3 types of analysis + // we want to suppress the client waiting for the DeviceAnalyses + // https://github.com/intel-innersource/applications.simulators.simics.base.dml-lang-server/pull/195/files#diff-4372f99a827132ceb7a1e60370d638c6c86bf3f885255373c9d5c8060659c5ed + pub fn report_errors(&mut self, path: &CanonPath, output: &O) { debug!("Reporting all errors for {:?}", path); // By this being a hashset, we will not double-report any errors diff --git a/src/actions/mod.rs b/src/actions/mod.rs index d6a4a27..268b738 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -12,7 +12,7 @@ use serde_json::json; use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::fs; -use std::sync::atomic::AtomicBool; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use crate::actions::analysis_storage::AnalysisStorage; @@ -177,7 +177,7 @@ pub struct InitActionContext { // directly opened files pub direct_opens: Arc>>, - + pub pending_direct_results: Arc, pub compilation_info: Arc>, prev_changes: Arc>>, @@ -234,6 +234,7 @@ impl InitActionContext { lint_config, jobs: Arc::default(), direct_opens: Arc::default(), + pending_direct_results: Arc::new(AtomicBool::new(false)), quiescent: Arc::new(AtomicBool::new(false)), prev_changes: Arc::default(), client_capabilities: Arc::new(client_capabilities), @@ -435,10 +436,12 @@ impl InitActionContext { "Analysing".to_string(), out.clone()); *notifier = Some(new_notifier.id()); new_notifier.notify_begin_progress(); + self.pending_direct_results.store(true, Ordering::SeqCst); } } pub fn maybe_end_progress(&mut self, out: &O) { - if !self.analysis_queue.has_work() { + if !self.analysis_queue.has_work() + && self.has_no_pending_direct_diagnostics() { // Need the scope here to succesfully drop the guard lock before // going into maybe_warn_missing_builtins below let lock_id = { self.current_notifier.lock().unwrap().clone() }; @@ -532,10 +535,11 @@ impl InitActionContext { file: &Path, out: &O) { if !self.config.lock().unwrap().linting_enabled { + self.pending_direct_results.store(true, Ordering::SeqCst); return; } let lint_config = self.lint_config.lock().unwrap().to_owned(); - if lint_config.direct_only { + if lint_config.cli_mode { let canon_path: CanonPath = file.to_path_buf().into(); if !self.direct_opens.lock().unwrap().contains(&canon_path) { return; @@ -548,6 +552,13 @@ impl InitActionContext { out); } + fn has_no_pending_direct_diagnostics(&self) -> bool { + if self.lint_config.lock().unwrap().direct_only { + return !self.pending_direct_results.load(Ordering::SeqCst) + } + true + } + fn lint_analyze(&mut self, file: &Path, context: Option, diff --git a/src/dfa/client.rs b/src/dfa/client.rs index 7b8f8ce..06eb448 100644 --- a/src/dfa/client.rs +++ b/src/dfa/client.rs @@ -231,7 +231,6 @@ impl ClientInterface { } else { self.waiting_for_received_lint.remove(&file); } - Ok(ServerMessage::Diagnostics( file, diagnostic_params.diagnostics .iter().cloned().map(Diagnostic::from) @@ -356,17 +355,9 @@ impl ClientInterface { }{} // Gather the results while 'condition: { - if self.waiting_for_received_diag.is_empty() - && self.waiting_for_received_lint.is_empty() { - if self.has_received_ended_progress { - break 'condition false; - } - debug!("Waiting for progress end"); - } else { - debug!("Waiting for outstanding analysises {:?} or lints {:?}", - self.waiting_for_received_diag, - self.waiting_for_received_lint.is_empty()); - } + if self.has_received_ended_progress { + break 'condition false; + } match self.receive() { ServerMessage::Error(e) => { trace!("server unexpectedly closed while waiting for analysis"); diff --git a/src/lint/mod.rs b/src/lint/mod.rs index 5d1ef7d..ffae159 100644 --- a/src/lint/mod.rs +++ b/src/lint/mod.rs @@ -52,7 +52,7 @@ pub fn maybe_parse_lint_cfg(path: PathBuf) -> Option { #[serde(default)] #[serde(deny_unknown_fields)] pub struct LintCfg { - pub direct_only: bool, + pub cli_mode: bool, #[serde(default)] pub sp_brace: Option, #[serde(default)] @@ -72,7 +72,7 @@ pub struct LintCfg { impl Default for LintCfg { fn default() -> LintCfg { LintCfg { - direct_only: true, + cli_mode: false, sp_brace: Some(SpBraceOptions{}), sp_punct: Some(SpPunctOptions{}), nsp_funpar: Some(NspFunparOptions{}), diff --git a/src/server/mod.rs b/src/server/mod.rs index 3a06023..991c895 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -366,9 +366,13 @@ impl LsService { requests) => { debug!("Received isolated analysis of {:?}", path); if let ActionContext::Init(ctx) = &mut self.ctx { + let lint_config = ctx.lint_config.lock().unwrap().to_owned(); ctx.update_analysis(); ctx.analysis.lock().unwrap().report_errors( &path, &self.output); + // TODO: suppress import resolver, or better yet, + // have `requests` be empty here when using direct only + // mode for file in requests { // A little bit of redundancy here, we need to // pre-resolve this import into an absolute path @@ -376,17 +380,24 @@ impl LsService { if let Some(file) = ctx.construct_resolver() .resolve_with_maybe_context(&file, context.as_ref()) { - trace!("Analysing imported file {}", - file.to_str().unwrap()); - ctx.isolated_analyze(&file, - context.clone(), - &self.output); + if lint_config.cli_mode { + trace!("Should not be here"); + } + else { + trace!("Analysing imported file {}", + file.to_str().unwrap()); + ctx.isolated_analyze(&file, + context.clone(), + &self.output); + } } else { trace!("Imported file {:?} did not resolve", file); } } - ctx.trigger_device_analysis(&path, &self.output); + if !ctx.lint_config.lock().unwrap().cli_mode { + ctx.trigger_device_analysis(&path, &self.output); + } ctx.maybe_trigger_lint_analysis(&path, &self.output); } }, @@ -404,14 +415,17 @@ impl LsService { ctx.update_analysis(); ctx.analysis.try_lock().unwrap().report_errors( &path, &self.output); + ctx.pending_direct_results.store(false, Ordering::SeqCst); } - }, // WIP lint + }, ServerToHandle::AnalysisRequest(importpath, context) => { if let ActionContext::Init(ctx) = &mut self.ctx { - debug!("Analysing imported file {}", + if !ctx.lint_config.lock().unwrap().to_owned().cli_mode { + debug!("Analysing imported file {}", &importpath.to_str().unwrap()); - ctx.isolated_analyze( - &importpath, context, &self.output); + ctx.isolated_analyze( + &importpath, context, &self.output); + } } } } From 908ddf468f6167ba80d2cb013f52fdb48f08c569 Mon Sep 17 00:00:00 2001 From: "Calvo Porras, Alejandro" Date: Mon, 27 Jan 2025 15:31:35 -0800 Subject: [PATCH 2/3] Make isolated_result first before sending IsolatedAnalysisDone --- src/actions/analysis_queue.rs | 6 +++--- src/actions/mod.rs | 16 ++++++++-------- src/server/mod.rs | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/actions/analysis_queue.rs b/src/actions/analysis_queue.rs index 23716b5..4584782 100644 --- a/src/actions/analysis_queue.rs +++ b/src/actions/analysis_queue.rs @@ -335,14 +335,14 @@ impl IsolatedAnalysisJob { } else { self.context.clone() }; + self.report.send(TimestampedStorage::make_isolated_result( + self.timestamp, + analysis.clone())).ok(); self.notify.send(ServerToHandle::IsolatedAnalysisDone( self.path.clone(), new_context, analysis.get_import_names() )).ok(); - self.report.send(TimestampedStorage::make_isolated_result( - self.timestamp, - analysis)).ok(); }, Err(e) => { trace!("Failed to create isolated analysis: {}", e); diff --git a/src/actions/mod.rs b/src/actions/mod.rs index 268b738..1fe1ce9 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -177,7 +177,7 @@ pub struct InitActionContext { // directly opened files pub direct_opens: Arc>>, - pub pending_direct_results: Arc, + pub wait_for_pending_results: Arc, pub compilation_info: Arc>, prev_changes: Arc>>, @@ -234,7 +234,7 @@ impl InitActionContext { lint_config, jobs: Arc::default(), direct_opens: Arc::default(), - pending_direct_results: Arc::new(AtomicBool::new(false)), + wait_for_pending_results: Arc::new(AtomicBool::new(false)), quiescent: Arc::new(AtomicBool::new(false)), prev_changes: Arc::default(), client_capabilities: Arc::new(client_capabilities), @@ -436,12 +436,12 @@ impl InitActionContext { "Analysing".to_string(), out.clone()); *notifier = Some(new_notifier.id()); new_notifier.notify_begin_progress(); - self.pending_direct_results.store(true, Ordering::SeqCst); + self.wait_for_pending_results.store(true, Ordering::SeqCst); } } pub fn maybe_end_progress(&mut self, out: &O) { if !self.analysis_queue.has_work() - && self.has_no_pending_direct_diagnostics() { + && self.has_no_pending_diagnostics() { // Need the scope here to succesfully drop the guard lock before // going into maybe_warn_missing_builtins below let lock_id = { self.current_notifier.lock().unwrap().clone() }; @@ -535,7 +535,7 @@ impl InitActionContext { file: &Path, out: &O) { if !self.config.lock().unwrap().linting_enabled { - self.pending_direct_results.store(true, Ordering::SeqCst); + self.wait_for_pending_results.store(false, Ordering::SeqCst); return; } let lint_config = self.lint_config.lock().unwrap().to_owned(); @@ -552,9 +552,9 @@ impl InitActionContext { out); } - fn has_no_pending_direct_diagnostics(&self) -> bool { - if self.lint_config.lock().unwrap().direct_only { - return !self.pending_direct_results.load(Ordering::SeqCst) + fn has_no_pending_diagnostics(&self) -> bool { + if self.lint_config.lock().unwrap().cli_mode { + return !self.wait_for_pending_results.load(Ordering::SeqCst) } true } diff --git a/src/server/mod.rs b/src/server/mod.rs index 991c895..81b9420 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -415,7 +415,7 @@ impl LsService { ctx.update_analysis(); ctx.analysis.try_lock().unwrap().report_errors( &path, &self.output); - ctx.pending_direct_results.store(false, Ordering::SeqCst); + ctx.wait_for_pending_results.store(false, Ordering::SeqCst); } }, ServerToHandle::AnalysisRequest(importpath, context) => { From dba60059f11a5d66864fac63ca37252b687e206d Mon Sep 17 00:00:00 2001 From: Juan Vasquez Date: Mon, 27 Jan 2025 17:31:39 -0800 Subject: [PATCH 3/3] Replace cli_mode lint_config field for suppress_imports CLI flag --- example_files/example_lint_cfg.README | 3 --- example_files/example_lint_cfg.json | 1 - src/actions/analysis_queue.rs | 5 +++-- src/actions/analysis_storage.rs | 5 +---- src/actions/mod.rs | 21 +++++---------------- src/config.rs | 2 ++ src/dfa/main.rs | 9 +++++++++ src/lint/mod.rs | 2 -- src/server/mod.rs | 15 ++++----------- 9 files changed, 24 insertions(+), 39 deletions(-) diff --git a/example_files/example_lint_cfg.README b/example_files/example_lint_cfg.README index c1bbf15..b7f64e1 100644 --- a/example_files/example_lint_cfg.README +++ b/example_files/example_lint_cfg.README @@ -9,9 +9,6 @@ // your modifications { - // if set to 'false', will parse and lint imported files as well - // inteded for use with dfa CLI client to lint single files - "cli_mode": true, // linting rules with no arguments are enabled by assigning an empty struct "sp_brace": {}, // linting rules with arguments are straightforward diff --git a/example_files/example_lint_cfg.json b/example_files/example_lint_cfg.json index 51f367d..f92abc9 100644 --- a/example_files/example_lint_cfg.json +++ b/example_files/example_lint_cfg.json @@ -1,5 +1,4 @@ { - "cli_mode": false, "sp_brace": {}, "sp_punct": {}, "nsp_funpar": {}, diff --git a/src/actions/analysis_queue.rs b/src/actions/analysis_queue.rs index 4584782..e70944b 100644 --- a/src/actions/analysis_queue.rs +++ b/src/actions/analysis_queue.rs @@ -335,13 +335,14 @@ impl IsolatedAnalysisJob { } else { self.context.clone() }; + let import_paths = analysis.get_import_names(); self.report.send(TimestampedStorage::make_isolated_result( self.timestamp, - analysis.clone())).ok(); + analysis)).ok(); self.notify.send(ServerToHandle::IsolatedAnalysisDone( self.path.clone(), new_context, - analysis.get_import_names() + import_paths )).ok(); }, Err(e) => { diff --git a/src/actions/analysis_storage.rs b/src/actions/analysis_storage.rs index 080961d..636e84c 100644 --- a/src/actions/analysis_storage.rs +++ b/src/actions/analysis_storage.rs @@ -627,10 +627,7 @@ impl AnalysisStorage { false } } - // TODO: Might need to separate responsibilities here among the 3 types of analysis - // we want to suppress the client waiting for the DeviceAnalyses - // https://github.com/intel-innersource/applications.simulators.simics.base.dml-lang-server/pull/195/files#diff-4372f99a827132ceb7a1e60370d638c6c86bf3f885255373c9d5c8060659c5ed - + pub fn report_errors(&mut self, path: &CanonPath, output: &O) { debug!("Reporting all errors for {:?}", path); // By this being a hashset, we will not double-report any errors diff --git a/src/actions/mod.rs b/src/actions/mod.rs index 1fe1ce9..ad27aa8 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -12,7 +12,7 @@ use serde_json::json; use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::fs; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::AtomicBool; use std::sync::{Arc, Mutex}; use crate::actions::analysis_storage::AnalysisStorage; @@ -177,7 +177,6 @@ pub struct InitActionContext { // directly opened files pub direct_opens: Arc>>, - pub wait_for_pending_results: Arc, pub compilation_info: Arc>, prev_changes: Arc>>, @@ -234,7 +233,6 @@ impl InitActionContext { lint_config, jobs: Arc::default(), direct_opens: Arc::default(), - wait_for_pending_results: Arc::new(AtomicBool::new(false)), quiescent: Arc::new(AtomicBool::new(false)), prev_changes: Arc::default(), client_capabilities: Arc::new(client_capabilities), @@ -436,12 +434,10 @@ impl InitActionContext { "Analysing".to_string(), out.clone()); *notifier = Some(new_notifier.id()); new_notifier.notify_begin_progress(); - self.wait_for_pending_results.store(true, Ordering::SeqCst); } } pub fn maybe_end_progress(&mut self, out: &O) { - if !self.analysis_queue.has_work() - && self.has_no_pending_diagnostics() { + if !self.analysis_queue.has_work() { // Need the scope here to succesfully drop the guard lock before // going into maybe_warn_missing_builtins below let lock_id = { self.current_notifier.lock().unwrap().clone() }; @@ -535,16 +531,16 @@ impl InitActionContext { file: &Path, out: &O) { if !self.config.lock().unwrap().linting_enabled { - self.wait_for_pending_results.store(false, Ordering::SeqCst); return; } - let lint_config = self.lint_config.lock().unwrap().to_owned(); - if lint_config.cli_mode { + let config = self.config.lock().unwrap().to_owned(); + if config.suppress_imports { let canon_path: CanonPath = file.to_path_buf().into(); if !self.direct_opens.lock().unwrap().contains(&canon_path) { return; } } + let lint_config = self.lint_config.lock().unwrap().to_owned(); debug!("Triggering linting analysis of {:?}", file); self.lint_analyze(file, None, @@ -552,13 +548,6 @@ impl InitActionContext { out); } - fn has_no_pending_diagnostics(&self) -> bool { - if self.lint_config.lock().unwrap().cli_mode { - return !self.wait_for_pending_results.load(Ordering::SeqCst) - } - true - } - fn lint_analyze(&mut self, file: &Path, context: Option, diff --git a/src/config.rs b/src/config.rs index a82377a..7d30547 100644 --- a/src/config.rs +++ b/src/config.rs @@ -104,6 +104,7 @@ pub struct Config { pub analyse_on_save: bool, pub features: Vec, pub all_features: bool, + pub suppress_imports: bool, pub linting_enabled: bool, pub lint_cfg_path: Option, pub no_default_features: bool, @@ -119,6 +120,7 @@ impl Default for Config { analyse_on_save: false, features: vec![], all_features: false, + suppress_imports: false, linting_enabled: true, lint_cfg_path: None, no_default_features: false, diff --git a/src/dfa/main.rs b/src/dfa/main.rs index 3b83849..91ba473 100644 --- a/src/dfa/main.rs +++ b/src/dfa/main.rs @@ -33,6 +33,7 @@ struct Args { files: Vec, workspaces: Vec, compile_info: Option, + suppress_imports: Option, linting_enabled: Option, lint_cfg_path: Option, test: bool, @@ -72,6 +73,11 @@ fn parse_args() -> Args { include paths") .value_parser(clap::value_parser!(PathBuf)) .required(false)) + .arg(Arg::new("suppress-imports").short('s').long("suppress-imports") + .action(ArgAction::Set) + .help("Analyses specified files only, without also analyzing files they import") + .value_parser(clap::value_parser!(bool)) + .required(false)) .arg(Arg::new("linting-enabled").short('l').long("linting-enabled") .action(ArgAction::Set) .help("Turns linting on/off (defaults to true)") @@ -99,6 +105,8 @@ fn parse_args() -> Args { test: args.contains_id("test"), compile_info: args.get_one::("compile-info") .cloned(), + suppress_imports: args.get_one::("suppress-imports") + .cloned(), linting_enabled: args.get_one::("linting-enabled") .cloned(), lint_cfg_path: args.get_one::("lint-cfg-path") @@ -134,6 +142,7 @@ fn main_inner() -> Result<(), i32> { dlsclient.add_workspaces(workspace_rest.cloned().collect()).or(Err(1))?; let config = Config { compile_info_path: arg.compile_info.clone(), + suppress_imports: arg.suppress_imports.unwrap_or(false), linting_enabled, lint_cfg_path: arg.lint_cfg_path.clone(), .. Default::default() diff --git a/src/lint/mod.rs b/src/lint/mod.rs index ffae159..6b1fa2c 100644 --- a/src/lint/mod.rs +++ b/src/lint/mod.rs @@ -52,7 +52,6 @@ pub fn maybe_parse_lint_cfg(path: PathBuf) -> Option { #[serde(default)] #[serde(deny_unknown_fields)] pub struct LintCfg { - pub cli_mode: bool, #[serde(default)] pub sp_brace: Option, #[serde(default)] @@ -72,7 +71,6 @@ pub struct LintCfg { impl Default for LintCfg { fn default() -> LintCfg { LintCfg { - cli_mode: false, sp_brace: Some(SpBraceOptions{}), sp_punct: Some(SpPunctOptions{}), nsp_funpar: Some(NspFunparOptions{}), diff --git a/src/server/mod.rs b/src/server/mod.rs index 81b9420..5e8e728 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -366,13 +366,10 @@ impl LsService { requests) => { debug!("Received isolated analysis of {:?}", path); if let ActionContext::Init(ctx) = &mut self.ctx { - let lint_config = ctx.lint_config.lock().unwrap().to_owned(); + let config = ctx.config.lock().unwrap().to_owned(); ctx.update_analysis(); ctx.analysis.lock().unwrap().report_errors( &path, &self.output); - // TODO: suppress import resolver, or better yet, - // have `requests` be empty here when using direct only - // mode for file in requests { // A little bit of redundancy here, we need to // pre-resolve this import into an absolute path @@ -380,10 +377,7 @@ impl LsService { if let Some(file) = ctx.construct_resolver() .resolve_with_maybe_context(&file, context.as_ref()) { - if lint_config.cli_mode { - trace!("Should not be here"); - } - else { + if !config.suppress_imports { trace!("Analysing imported file {}", file.to_str().unwrap()); ctx.isolated_analyze(&file, @@ -395,7 +389,7 @@ impl LsService { file); } } - if !ctx.lint_config.lock().unwrap().cli_mode { + if !ctx.config.lock().unwrap().suppress_imports { ctx.trigger_device_analysis(&path, &self.output); } ctx.maybe_trigger_lint_analysis(&path, &self.output); @@ -415,12 +409,11 @@ impl LsService { ctx.update_analysis(); ctx.analysis.try_lock().unwrap().report_errors( &path, &self.output); - ctx.wait_for_pending_results.store(false, Ordering::SeqCst); } }, ServerToHandle::AnalysisRequest(importpath, context) => { if let ActionContext::Init(ctx) = &mut self.ctx { - if !ctx.lint_config.lock().unwrap().to_owned().cli_mode { + if !ctx.config.lock().unwrap().to_owned().suppress_imports { debug!("Analysing imported file {}", &importpath.to_str().unwrap()); ctx.isolated_analyze(