Analyzing progress notifications #44

Arcticae · 2024-12-18T13:14:14Z

Based off this PR we can start implementing tests (like currently we can wait for project update + the diagnostics finish and it should work fine)

src/lang/diagnostics/mod.rs

Draggu · 2024-12-18T13:34:36Z

src/server/client.rs

@@ -26,7 +26,7 @@ pub struct Client<'s> {
    pub(super) requester: Requester<'s>,
 }

-#[derive(Clone)]
+#[derive(Clone, Debug)]


Why?

Suggested change

#[derive(Clone, Debug)]

#[derive(Clone)]

It's included in the new Controller which needs to be Debug as well because of ProcMacroClient being Debug

Maybe custom impl on ProcMacroClient would be better in this case? This does not make sense to debug this struct

Draggu · 2024-12-18T13:39:25Z

src/state.rs

+        );
+
+        let diagnostics_controller =
+            DiagnosticsController::new(notifier.clone(), analysis_progress_controller.clone());


Please dont pass AnalysisProgressController everywhere, split it into 2 parts where one lives on the main thread/special thread and second can be used in other places, this will also help splitting its interface.

Done - pls let me know if that's what you meant

src/state.rs

Draggu · 2024-12-18T13:40:26Z

src/ide/analysis_progress.rs

+/// Uses information provided from other controllers (diagnostics controller, procmacro controller)
+/// to assess if diagnostics are in fact calculated.
+#[derive(Debug, Clone)]
+pub struct AnalysisProgressController {


Can we get flow explanation here? like in ProcMacroController

Can do although i think it's nicely explained here in the docstrings

TBD Still - i would like to finalize business logic first if that's OK so i don't have to update this flowchart

Co-authored-by: Piotr Figiela <77412592+Draggu@users.noreply.github.com> Signed-off-by: Tomasz Rejowski <34059895+Arcticae@users.noreply.github.com>

src/state.rs

piotmag769 · 2024-12-19T20:00:54Z

src/server/client.rs

@@ -26,7 +26,7 @@ pub struct Client<'s> {
    pub(super) requester: Requester<'s>,
 }

-#[derive(Clone)]
+#[derive(Clone, Debug)]


piotmag769 · 2024-12-19T20:01:57Z

src/state.rs

+    }
+}
+/// Struct which allows setting a callback - which can be triggered afterward
+/// by the function which has the reference.


Reference to what?

Reference to the thing that i'm talking about - the struct

Can we state that explicitly in the comment? It can be figured out, but it is not immediately obvious on the (English) language level

piotmag769 · 2024-12-19T20:02:11Z

src/state.rs

+/// Struct which allows setting a callback - which can be triggered afterward
+/// by the function which has the reference.
+#[derive(Default)]
+pub struct Beacon {


Is this a standard name for this pattern? Isn't Callback with set and execute methods better?

piotmag769 · 2024-12-19T20:03:19Z

src/state.rs

+
+    pub fn signal(&mut self) {
+        if let Some(hook) = self.signal_hook.take() {
+            hook(); // call the hook


This comment is useless

piotmag769 · 2024-12-19T20:03:45Z

src/state.rs

+        self.signal_hook = Some(Box::new(drop_hook));
+    }
+
+    pub fn signal(&mut self) {


signal_and_clear_hook?

piotmag769 · 2024-12-19T20:12:17Z

src/ide/analysis_progress.rs

+/// Notifies about diagnostics generation which is beginning to calculate
+#[derive(Debug)]
+pub struct DiagnosticsCalculationStart;
+
+impl Notification for DiagnosticsCalculationStart {
+    type Params = ();
+    const METHOD: &'static str = "cairo/diagnosticsCalculationStart";
+}
+
+/// Notifies about diagnostics generation which ended calculating
+#[derive(Debug)]
+pub struct DiagnosticsCalculationFinish;
+
+impl Notification for DiagnosticsCalculationFinish {
+    type Params = ();
+    const METHOD: &'static str = "cairo/diagnosticsCalculationFinish";
+}


Idk if it shouldn't go to lsp/ext.rs for consistency

I don't know why not place it near the usage tbh

Both are fine for me tbh, I guess we should get rid of lsp/ext.rs altogether #130

let's place it in lsp/ext.rs as discussed on meeting

src/state.rs

piotmag769 · 2024-12-20T15:33:59Z

src/state.rs

+}
+
+impl Beacon {
+    // Set the drop hook


dot.

Btw it doesn't set the drop hook, it is not done on Drop.

src/ide/analysis_progress.rs

Draggu · 2024-12-20T15:54:28Z

src/lang/proc_macros/client/mod.rs

+            .field("id_generator", &self.id_generator)
+            .field("requests_params", &self.requests_params)
+            .field("error_channel", &self.error_channel)
+            .finish()


Suggested change

.finish()

.finish_non_exhaustive()

Draggu · 2024-12-20T15:56:03Z

src/state.rs

+}
+
+impl Beacon {
+    // Set the drop hook


Maybe note here that it is disposable?

src/state.rs

Draggu · 2024-12-20T16:12:12Z

src/ide/analysis_progress.rs

+    controller: AnalysisProgressController,
+}
+
+impl AnalysisProgressTracker {


We can split it for proc macros / diagnostics so interface is clear there, also it does not have to store whole controller (and should not if it possible, if not then maybe controller itself is ok).

Draggu · 2024-12-20T16:37:42Z

src/ide/analysis_progress.rs

+    }
+
+    /// Allows to set the procmacro configuration to whatever is in the config, upon loading it.
+    pub fn set_procmacros_enabled(&self, value: bool) {


Suggested change

pub fn set_procmacros_enabled(&self, value: bool) {

pub fn on_config_change(&self, config: &Config) {

More future proof and it is clear where this info is coming from.

piotmag769 · 2024-12-20T16:23:30Z

src/ide/analysis_progress.rs

+        let id_generator = Arc::new(IdGenerator::default());
+        Self {
+            notifier,
+            id_generator: id_generator.clone(),


Why clone here, just generate unique_id before the struct constructor expr

piotmag769 · 2024-12-20T16:37:58Z

src/ide/analysis_progress.rs

+    pub fn get_generation_id(&self) -> u64 {
+        self.generation_id.load(Ordering::SeqCst)
+    }


Used in one place, just inline it imo

piotmag769 · 2024-12-20T16:42:42Z

src/ide/analysis_progress.rs

+    }
+
+    pub fn clear_active_snapshots(&self) {
+        let active_snapshots_ref = self.active_snapshots.clone();


Do you need to clone here?

piotmag769 · 2024-12-20T17:05:37Z

src/lang/proc_macros/client/mod.rs

@@ -142,6 +150,7 @@ impl ProcMacroClient {
        match self.send_request_untracked::<M>(id, &params) {
            Ok(()) => {
                requests_params.insert(id, map(params));
+                self.analysis_progress_tracker.register_procmacro_request();


I believe that there is a small chance that a yet alive snapshot from previous diagnostics calculation was already in db.get_attribute_expansion before its cancellation. It will still call this method and possibly register proc macro request in analysis tracker AFTER track_analysis is called to start tracking the new diagnostics batch. It can cause us to never finish waiting for diagnostics in tests, since:

the next batch won't be scheduled

we will always think there is some proc macro yet to resolve while it may not be

piotmag769

Logic wise seems good besides what I wrote before

piotmag769 · 2024-12-21T21:03:09Z

src/ide/analysis_progress.rs

+
+    pub fn remove_active_snapshot(&self, snapshot_id: usize) {
+        let mut active_snapshots = self.active_snapshots.lock().unwrap();
+        active_snapshots.remove(&snapshot_id);


An assertion could be made here to make sure sth was removed

piotmag769 · 2024-12-21T22:10:29Z

src/ide/analysis_progress.rs

A lot of pubs are unnecessary here, some methods could be inlined

mkaput · 2025-01-08T09:21:57Z

src/state.rs

+}
+/// Struct which allows setting a callback - which can be triggered afterward


Suggested change

}

/// Struct which allows setting a callback - which can be triggered afterward

}

/// Struct which allows setting a callback - which can be triggered afterward

mkaput · 2025-01-08T09:30:08Z

src/state.rs

+/// Struct which allows setting a callback - which can be triggered afterward
+/// by the function which has the reference.
+#[derive(Default)]
+pub struct Beacon {


TBH you have simplified this so much that I would probably drop this type entirely and just stick that Option<Box<dyn FnOnce() + Send>>.

mkaput · 2025-01-08T09:32:45Z

src/ide/analysis_progress.rs

+    /// The beacons are wrapping snapshots, which are signalling when diagnostics finished
+    /// calculating for a given snapshot (used for calculating files diagnostics or removing
+    /// stale ones)
+    pub fn track_analysis<'a>(&self, beacons: impl Iterator<Item = &'a mut Beacon>) {


Can't this just take a mutable reference to StateSnapshots?

mkaput · 2025-01-08T16:00:21Z

src/ide/analysis_progress.rs

+    /// Sequential IDs of state snapshots from the current generation, used to track their status
+    /// (present meaning it's still being used)
+    active_snapshots: Arc<Mutex<HashSet<usize>>>,
+    id_generator: Arc<IdGenerator>,


TBH you'd be better off not touching the IdGenerator here and just peeing that AtomicU64 in generation_id and incrementing it directly. you're just doing this in a fancy way now

mkaput · 2025-01-08T16:06:05Z

src/ide/analysis_progress.rs

+#[derive(Debug, Clone)]
+pub struct AnalysisProgressController {
+    notifier: Notifier,
+    /// ID of the diagnostics "generation" - the scheduled diagnostics jobs set.
+    /// Used to filter out stale threads finishing when new ones (from newer "generation")
+    /// are already in progress and being tracked by the controller.
+    generation_id: Arc<AtomicU64>,
+    /// Sequential IDs of state snapshots from the current generation, used to track their status
+    /// (present meaning it's still being used)
+    active_snapshots: Arc<Mutex<HashSet<usize>>>,
+    id_generator: Arc<IdGenerator>,
+    /// If `true` - a request to procmacro server was submitted, meaning that analysis will extend
+    /// beyond the current generation of diagnostics.
+    did_submit_procmacro_request: Arc<AtomicBool>,
+    /// Indicates that a notification was sent and analysis (i.e. macro expansion) is taking place.
+    analysis_in_progress: Arc<AtomicBool>,
+    /// Loaded asynchronously from config - unset if config was not loaded yet.
+    /// Has to be set in order for analysis to finish.
+    procmacros_enabled: Arc<Mutex<Option<bool>>>,
+}


consider just using a mutex for everything. I don't believe you benefit from atomics here over mutexes, and I'm not sure all methods here guarantee consistency in case of races.

struct AnalysisProgressController(Arc<Mutex<AnalysisProgressControllerState>>); struct AnalysisProgressControllerState { generation_id: u64, // ... }

mkaput · 2025-01-08T16:13:12Z

src/ide/analysis_progress.rs

+/// Notifies about diagnostics generation which is beginning to calculate
+#[derive(Debug)]
+pub struct DiagnosticsCalculationStart;
+
+impl Notification for DiagnosticsCalculationStart {
+    type Params = ();
+    const METHOD: &'static str = "cairo/diagnosticsCalculationStart";
+}
+
+/// Notifies about diagnostics generation which ended calculating
+#[derive(Debug)]
+pub struct DiagnosticsCalculationFinish;
+
+impl Notification for DiagnosticsCalculationFinish {
+    type Params = ();
+    const METHOD: &'static str = "cairo/diagnosticsCalculationFinish";
+}


let's place it in lsp/ext.rs as discussed on meeting

Track diagnostics worker threads with beacons

d3e5afd

Arcticae force-pushed the feature/analyzing-progress-notifications branch 2 times, most recently from ae728b8 to bcd6a77 Compare December 18, 2024 13:33

Draggu reviewed Dec 18, 2024

View reviewed changes

Analysis Progress Controller

2a2bc04

Arcticae force-pushed the feature/analyzing-progress-notifications branch from bcd6a77 to 2a2bc04 Compare December 18, 2024 13:49

Update src/lang/diagnostics/mod.rs

1606099

Co-authored-by: Piotr Figiela <77412592+Draggu@users.noreply.github.com> Signed-off-by: Tomasz Rejowski <34059895+Arcticae@users.noreply.github.com>

mkaput requested changes Dec 19, 2024

View reviewed changes

src/state.rs Outdated Show resolved Hide resolved

Review suggestions

935e0ad

piotmag769 reviewed Dec 20, 2024

View reviewed changes

Draggu reviewed Dec 20, 2024

View reviewed changes

piotmag769 requested changes Dec 20, 2024

View reviewed changes

piotmag769 reviewed Dec 21, 2024

View reviewed changes

mkaput requested changes Jan 8, 2025

View reviewed changes

	pub fn set_procmacros_enabled(&self, value: bool) {
	pub fn on_config_change(&self, config: &Config) {

		}
		/// Struct which allows setting a callback - which can be triggered afterward

Analyzing progress notifications #44

Are you sure you want to change the base?

Analyzing progress notifications #44

Conversation

Arcticae commented Dec 18, 2024

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

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

piotmag769 Dec 21, 2024 • edited Loading

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

Choose a reason for hiding this comment

piotmag769 left a 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

piotmag769 Dec 21, 2024 •

edited

Loading