Service and plugin lookups in files open at startup

docs/CHANGELOG.md

@@ -587,6 +587,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Find References in Drupal hook files.** Calls in a Drupal site's `.module`, `.install`, `.theme`, `.profile`, `.inc` and `.engine` files are now found by Find References and rename, and `analyze` checks those files, without listing the extensions under `[indexing] extensions`.
 - **WordPress's `__()` returns a string.** Outside a Laravel project, `__()` and `trans()` keep the return type their own declaration gives, instead of Laravel's `string|array`, so passing a translated string to WordPress functions is no longer reported.
 - **WordPress output constants.** In a project with the WordPress stubs installed, `OBJECT`, `OBJECT_K`, `ARRAY_A` and `ARRAY_N` are known constants, so `get_post($id)` is a `WP_Post|null` and `get_post($id, ARRAY_A)` an array, instead of every possible result at once.
+- **Service and plugin lookups in files open at startup.** A lookup through a service or a plugin manager, such as `\Drupal::service('entity_type.manager')->getStorage('node')`, in a file the editor had open before the project finished indexing is now found once indexing ends, and follows a service provider's `alter()` as it is edited. Contributed by @giuseppe-esposito-87.
 
 #### Completion, hover, and editor features
 

docs/todo/bugs.md

@@ -61,7 +61,50 @@ No outstanding items.
 
 ## Framework names in strings
 
-No outstanding items.
+### B743. A configurator confirmed while another is being confirmed keeps an incomplete answer
+
+**Impact: Medium · Complexity: Medium**
+
+Reproduced on `75f27a0ba` and on `d0b36afac` (before #568), so the
+per-thread index reuse did not introduce it.
+
+```php
+// config/c00.php
+function configure0(ContainerBuilder $container): void {
+    $container->register('app.s0', ContainerBuilder::class);
+}
+// config/c01.php
+function configure1(ContainerBuilder $container): void {
+    $container->get('app.s0')->register('app.s1', ContainerBuilder::class);
+}
+// config/c02.php
+function configure2(ContainerBuilder $container): void {
+    $container->get('app.s1')->register('app.s2', ContainerBuilder::class);
+}
+// config/c03.php
+function configure3(ContainerBuilder $container): void {
+    $container->get('app.s2')->register('app.s3', ContainerBuilder::class);
+}
+```
+
+The container index ends up with `app.s0` and `app.s1` only, so
+`$container->get('app.s2')` and `'app.s3'` resolve to nothing everywhere.
+Building the index confirms `c01`, which builds the index again, which
+confirms `c02` while `c01` reads as declaring nothing. `app.s1` is then
+missing, so `c02` declares nothing, and `typed_receiver_spans_for` caches
+that answer even though `partial_answers()` moved while it was computed.
+The outer build is correctly not kept, but the next one reads `c02`'s
+cached answer and is. The answer only goes once the confirmed spans are
+retired. When each file depends on one that sorts after it, as in
+`chained_configurators` in `src/symfony/container.rs`, every nested answer
+happens to be complete, which is why the existing test passes.
+
+Not caching an answer computed while the partial count moved fixes it, but
+every nested confirmation is partial in that sense, so the chain is then
+recomputed at every level. The fix that keeps the cost down is a fixed
+point: build the index from the untyped declarations, then confirm the
+typed calls (`Contribution::typed`) against it, re-confirming until no
+call is added.
 
 ## Name resolution
 

src/blade/state.rs

@@ -2,6 +2,7 @@
 //! (see [`FileState`](crate::file_state::FileState)).
 
 use std::collections::{HashMap, HashSet};
+use std::sync::atomic::AtomicU64;
 
 use parking_lot::RwLock;
 
@@ -24,9 +25,13 @@ pub(crate) struct BladeState {
     /// whose scope is unchanged.
     pub(crate) injected_vars: RwLock<HashMap<String, super::call_site_inference::BladeScope>>,
     /// Per-file string-key spans whose call only the receiver's *type*
-    /// settles, computed on first use and dropped when the file is
-    /// re-parsed (see [`crate::blade::typed_receiver`]).  Only files whose
-    /// symbol map recorded a candidate site ever get an entry.
+    /// settles, computed on first use and stale once the file is
+    /// re-parsed or [`Self::typed_receiver_epoch`] moves on.  Only files
+    /// whose symbol map recorded a candidate site ever get an entry.
     pub(crate) typed_receiver_spans_cache:
         RwLock<HashMap<String, super::typed_receiver::TypedReceiverSpans>>,
+    /// Moves on whenever a change elsewhere in the project may retype a
+    /// receiver, retiring every entry of `typed_receiver_spans_cache`
+    /// stamped before it (see `Backend::retire_typed_receiver_spans`).
+    pub(crate) typed_receiver_epoch: AtomicU64,
 }

src/blade/typed_receiver.rs

@@ -29,6 +29,7 @@
 //! [`Backend::typed_receiver_key_kind`]).
 
 use std::sync::Arc;
+use std::sync::atomic::Ordering;
 
 use mago_span::HasSpan;
 use mago_syntax::cst::literal::Literal;
@@ -40,13 +41,17 @@ use crate::parser::with_parsed_program;
 use crate::php_type::PhpType;
 use crate::symbol_map::{ReceiverClass, StringKeyKind, SymbolMap, SymbolSpan, TypedReceiverSite};
 
-/// One file's confirmed extra string-key spans, and the source length they
-/// were derived from.
+/// One file's confirmed extra string-key spans, with the epoch and the
+/// source length they were derived from.
 ///
 /// The length is the same staleness signal [`SymbolMap::matches_source`]
 /// uses: an insertion or removal since the spans were computed moves every
-/// offset in them, so they have to be recomputed rather than reused.
-pub(crate) type TypedReceiverSpans = (u32, Arc<Vec<SymbolSpan>>);
+/// offset in them, so they have to be recomputed rather than reused.  The
+/// epoch is [`BladeState::typed_receiver_epoch`] as it read before the
+/// receivers were resolved.
+///
+/// [`BladeState::typed_receiver_epoch`]: super::state::BladeState::typed_receiver_epoch
+pub(crate) type TypedReceiverSpans = (u64, u32, Arc<Vec<SymbolSpan>>);
 
 impl Backend {
     /// The string-key spans of `uri` whose call is recognised by the
@@ -68,7 +73,13 @@ impl Backend {
         if map.typed_receiver_sites.is_empty() {
             return empty_spans();
         }
-        if let Some((source_len, spans)) = self.blade.typed_receiver_spans_cache.read().get(uri)
+        // Read before resolving: a change that retypes a receiver while
+        // the sites are being confirmed then retires the answer, instead of
+        // the answer being cached as current when it may predate the change.
+        let epoch = self.blade.typed_receiver_epoch.load(Ordering::Acquire);
+        if let Some((cached_epoch, source_len, spans)) =
+            self.blade.typed_receiver_spans_cache.read().get(uri)
+            && *cached_epoch == epoch
             && *source_len == map.source_len
         {
             return Arc::clone(spans);
@@ -96,13 +107,36 @@ impl Backend {
         };
         let spans =
             Arc::new(self.confirm_typed_receiver_sites(uri, &content, &map.typed_receiver_sites));
-        self.blade
-            .typed_receiver_spans_cache
-            .write()
-            .insert(uri.to_string(), (map.source_len, Arc::clone(&spans)));
+        #[cfg(test)]
+        if let Some(hook) = BEFORE_CACHING.take() {
+            hook(self);
+        }
+        let mut cache = self.blade.typed_receiver_spans_cache.write();
+        // An answer a change has already retired would only stand in for,
+        // or replace, a newer one.
+        if self.blade.typed_receiver_epoch.load(Ordering::Acquire) == epoch {
+            cache.insert(uri.to_string(), (epoch, map.source_len, Arc::clone(&spans)));
+        }
         spans
     }
 
+    /// Retire the confirmed spans of every file, for a change that may
+    /// retype a receiver anywhere: a signature, what the service container
+    /// declares, or the plugin managers.
+    ///
+    /// Call it once a confirmation that reads the new epoch can only read
+    /// the new state: after the change is published, or while holding a
+    /// lock such a confirmation has to wait for, as the `files` lock a
+    /// batch is published under.
+    pub(crate) fn retire_typed_receiver_spans(&self) {
+        self.blade
+            .typed_receiver_epoch
+            .fetch_add(1, Ordering::Release);
+        if !self.blade.typed_receiver_spans_cache.read().is_empty() {
+            self.blade.typed_receiver_spans_cache.write().clear();
+        }
+    }
+
     /// Drop the confirmed spans of one file, so the next reader recomputes
     /// them against the file as it now reads.
     pub(crate) fn evict_typed_receiver_spans(&self, uri: &str) {
@@ -501,6 +535,15 @@ impl Drop for ForwardingGuard {
     }
 }
 
+#[cfg(test)]
+thread_local! {
+    /// Run once, on this thread, between confirming a file's sites and
+    /// caching them: a test's stand-in for a change landing on another
+    /// thread meanwhile.
+    static BEFORE_CACHING: std::cell::Cell<Option<fn(&Backend)>> =
+        const { std::cell::Cell::new(None) };
+}
+
 thread_local! {
     /// The files whose sites are being confirmed on this thread.
     static CONFIRMING: std::cell::RefCell<std::collections::HashSet<String>> =
@@ -668,3 +711,7 @@ impl<'ast, 'arena, 'w, 'sites>
         }
     }
 }
+
+#[cfg(test)]
+#[path = "typed_receiver_tests.rs"]
+mod tests;

src/blade/typed_receiver_tests.rs

@@ -0,0 +1,105 @@
+use std::sync::Arc;
+
+use super::BEFORE_CACHING;
+use crate::Backend;
+use crate::symbol_map::{StringKeyKind, SymbolKind, SymbolMap, SymbolSpan};
+
+const URI: &str = "file:///phpantom-typed-receiver-cache.php";
+const SOURCE: &str = "<?php\nfunction mailer(\\Psr\\Container\\ContainerInterface $container) {\n    $container->get('app.mailer');\n}\n";
+
+const CLASS_URI: &str = "file:///phpantom-typed-receiver-mailer.php";
+const CLASS: &str = "<?php\nclass Mailer { public function send(): int { return 1; } }\n";
+const CLASS_RETYPED: &str =
+    "<?php\nclass Mailer { public function send(): string { return ''; } }\n";
+
+const SERVICES_URI: &str = "file:///config/services.yaml";
+
+/// Note a services file that declares `app.mailer`.
+fn declare_mailer(backend: &Backend) {
+    let declaring = SymbolMap {
+        spans: vec![SymbolSpan {
+            start: 12,
+            end: 22,
+            kind: SymbolKind::StringKey {
+                kind: StringKeyKind::ServiceId,
+                key: "app.mailer".to_string(),
+                is_write: true,
+                is_optional: false,
+            },
+        }],
+        ..Default::default()
+    };
+    backend.note_container_declarations(SERVICES_URI, Some(&declaring));
+}
+
+fn open(backend: &Backend, uri: &str, source: &str) {
+    backend
+        .open_files
+        .write()
+        .insert(uri.to_string(), Arc::new(source.to_string()));
+    backend.update_ast(uri, source);
+}
+
+fn opened() -> (Backend, Arc<SymbolMap>) {
+    let backend = Backend::new_headless();
+    open(&backend, CLASS_URI, CLASS);
+    open(&backend, URI, SOURCE);
+    let map = backend.symbol_map_for(URI).unwrap();
+    assert!(!map.typed_receiver_sites.is_empty());
+    (backend, map)
+}
+
+#[test]
+fn confirmed_spans_are_reused_until_a_receiver_may_be_retyped() {
+    let (backend, map) = opened();
+    let first = backend.typed_receiver_spans_for(URI, &map);
+    assert!(Arc::ptr_eq(
+        &first,
+        &backend.typed_receiver_spans_for(URI, &map)
+    ));
+
+    open(&backend, CLASS_URI, CLASS_RETYPED);
+    let retyped = backend.typed_receiver_spans_for(URI, &map);
+    assert!(!Arc::ptr_eq(&first, &retyped));
+
+    declare_mailer(&backend);
+    assert!(!Arc::ptr_eq(
+        &retyped,
+        &backend.typed_receiver_spans_for(URI, &map)
+    ));
+}
+
+#[test]
+fn reading_the_drupal_project_again_retires_confirmed_spans() {
+    let (backend, map) = opened();
+    let confirmed = backend.typed_receiver_spans_for(URI, &map);
+    backend.clear_drupal_web_roots();
+    assert!(!Arc::ptr_eq(
+        &confirmed,
+        &backend.typed_receiver_spans_for(URI, &map)
+    ));
+}
+
+#[test]
+fn spans_confirmed_while_a_signature_changes_are_not_kept() {
+    let (backend, map) = opened();
+    BEFORE_CACHING.set(Some(|backend| open(backend, CLASS_URI, CLASS_RETYPED)));
+    let confirmed = backend.typed_receiver_spans_for(URI, &map);
+    assert!(BEFORE_CACHING.take().is_none());
+    assert!(!Arc::ptr_eq(
+        &confirmed,
+        &backend.typed_receiver_spans_for(URI, &map)
+    ));
+}
+
+#[test]
+fn spans_confirmed_while_the_container_changes_are_not_kept() {
+    let (backend, map) = opened();
+    BEFORE_CACHING.set(Some(declare_mailer));
+    let confirmed = backend.typed_receiver_spans_for(URI, &map);
+    assert!(BEFORE_CACHING.take().is_none());
+    assert!(!Arc::ptr_eq(
+        &confirmed,
+        &backend.typed_receiver_spans_for(URI, &map)
+    ));
+}

src/drupal/mod.rs

@@ -431,6 +431,9 @@ impl Backend {
         self.drupal.service_definition_files.write().clear();
         *self.drupal.php_service_ids.write() = None;
         self.drupal.config.clear();
+        // The service providers' replacements and the plugin managers type
+        // receivers, so lookups confirmed against them go with them.
+        self.retire_typed_receiver_spans();
     }
 
     /// Remember the declaration files the Drupal walk found: the YAML for

src/drupal/plugins.rs

@@ -458,7 +458,17 @@ impl Backend {
                 from_test,
             })
         });
-        let unchanged = self.drupal.plugin_sources.read().get(uri) == sources.as_ref();
+        let (unchanged, managers_changed) = {
+            let stored = self.drupal.plugin_sources.read();
+            let old = stored.get(uri);
+            fn managers(sources: Option<&PluginSources>) -> &[PluginManagerSite] {
+                sources.map_or(&[], |sources| sources.managers.as_slice())
+            }
+            (
+                old == sources.as_ref(),
+                managers(old) != managers(sources.as_ref()),
+            )
+        };
         if !unchanged {
             let mut stored = self.drupal.plugin_sources.write();
             match sources {
@@ -484,6 +494,13 @@ impl Backend {
                 *slot = None;
             }
         }
+        // A receiver is confirmed as a plugin manager by the managers this
+        // index holds.  The plugins themselves do not type a receiver, and
+        // their offsets move with every edit above them, so a plugin class
+        // being indexed or edited leaves every file's confirmed spans be.
+        if managers_changed {
+            self.retire_typed_receiver_spans();
+        }
     }
 
     /// Read the plugin types from the project's plugin managers, then the

src/drupal/service_providers.rs

@@ -223,7 +223,11 @@ impl Backend {
                 read.push((crate::util::path_to_uri(&path), replacements));
             }
         }
+        if read.is_empty() {
+            return;
+        }
         self.drupal.service_replacements.write().extend(read);
+        self.retire_typed_receiver_spans();
     }
 
     /// Read a service provider again after its file changed.  Cheap for
@@ -247,17 +251,26 @@ impl Backend {
         }
         let replacements = service_class_replacements(content);
         let mut all = self.drupal.service_replacements.write();
-        if replacements.is_empty() {
-            all.remove(uri);
+        let changed = if replacements.is_empty() {
+            all.remove(uri).is_some()
+        } else if all.get(uri) == Some(&replacements) {
+            false
         } else {
             all.insert(uri.to_string(), replacements);
+            true
+        };
+        drop(all);
+        // A replacement types the service, and so its lookups' receivers.
+        if changed {
+            self.retire_typed_receiver_spans();
         }
     }
 
     /// Forget what a deleted service provider replaced.
     pub(crate) fn forget_drupal_service_provider(&self, uri: &str) {
         if self.drupal.service_replacements.read().contains_key(uri) {
             self.drupal.service_replacements.write().remove(uri);
+            self.retire_typed_receiver_spans();
         }
     }
 

src/parser/ast_publish.rs

@@ -974,7 +974,10 @@ impl Backend {
         // A receiver's type is settled against the classes of the whole
         // workspace, so a signature change anywhere can turn a call that
         // was not a render into one, or the other way round.
-        self.blade.typed_receiver_spans_cache.write().clear();
+        // Retired here because `invalidate_container_index` only retires
+        // them when a configurator's typed calls make the index depend on
+        // signatures.
+        self.retire_typed_receiver_spans();
         self.invalidate_container_index(true);
         self.laravel.morph_column_spans_cache.write().clear();
     }