diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 54c13b59b..d1165bcd1 100644 --- a/docs/CHANGELOG.md +++ b/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 diff --git a/docs/todo/bugs.md b/docs/todo/bugs.md index ac3336653..cf3f579a9 100644 --- a/docs/todo/bugs.md +++ b/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 diff --git a/src/blade/state.rs b/src/blade/state.rs index 2d21d623a..c55196133 100644 --- a/src/blade/state.rs +++ b/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>, /// 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>, + /// 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, } diff --git a/src/blade/typed_receiver.rs b/src/blade/typed_receiver.rs index f58323d9e..b9363b1d5 100644 --- a/src/blade/typed_receiver.rs +++ b/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>); +/// 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>); 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> = + const { std::cell::Cell::new(None) }; +} + thread_local! { /// The files whose sites are being confirmed on this thread. static CONFIRMING: std::cell::RefCell> = @@ -668,3 +711,7 @@ impl<'ast, 'arena, 'w, 'sites> } } } + +#[cfg(test)] +#[path = "typed_receiver_tests.rs"] +mod tests; diff --git a/src/blade/typed_receiver_tests.rs b/src/blade/typed_receiver_tests.rs new file mode 100644 index 000000000..4e39b109f --- /dev/null +++ b/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 = "get('app.mailer');\n}\n"; + +const CLASS_URI: &str = "file:///phpantom-typed-receiver-mailer.php"; +const CLASS: &str = " (Backend, Arc) { + 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) + )); +} diff --git a/src/drupal/mod.rs b/src/drupal/mod.rs index 2c110a1a8..69ea2b26c 100644 --- a/src/drupal/mod.rs +++ b/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 diff --git a/src/drupal/plugins.rs b/src/drupal/plugins.rs index b5acbe5fd..fb5c3f959 100644 --- a/src/drupal/plugins.rs +++ b/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 diff --git a/src/drupal/service_providers.rs b/src/drupal/service_providers.rs index 055ba86a6..50afe78bd 100644 --- a/src/drupal/service_providers.rs +++ b/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,10 +251,18 @@ 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(); } } @@ -258,6 +270,7 @@ impl Backend { 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(); } } diff --git a/src/parser/ast_publish.rs b/src/parser/ast_publish.rs index b0bc20007..27eae9898 100644 --- a/src/parser/ast_publish.rs +++ b/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(); } diff --git a/src/symfony/container.rs b/src/symfony/container.rs index 35602020a..1f04477b9 100644 --- a/src/symfony/container.rs +++ b/src/symfony/container.rs @@ -312,6 +312,14 @@ impl Backend { }; if changed { state.generation.fetch_add(1, Ordering::AcqRel); + drop(contributions); + #[cfg(test)] + if let Some(hook) = AFTER_GENERATION_MOVES.take() { + hook(self); + } + // A receiver typed by a service, as `\Drupal::service('…')->` + // is, may have been confirmed against declarations not read yet. + self.retire_typed_receiver_spans(); } } @@ -331,6 +339,7 @@ impl Backend { return; } state.generation.fetch_add(1, Ordering::AcqRel); + self.retire_typed_receiver_spans(); } /// The container index, built if nothing current is kept. @@ -666,6 +675,15 @@ thread_local! { static BUILDS: std::cell::Cell = const { std::cell::Cell::new(0) }; } +#[cfg(test)] +thread_local! { + /// Run once, on this thread, right after a file's declarations move the + /// container's generation on: a test's stand-in for a lookup on another + /// thread landing then. + static AFTER_GENERATION_MOVES: std::cell::Cell> = + const { std::cell::Cell::new(None) }; +} + #[cfg(test)] mod tests { use super::*; @@ -799,4 +817,41 @@ mod tests { assert_eq!(declared, 3); assert_eq!(BUILDS.get(), 1); } + + #[test] + fn a_lookup_as_a_file_starts_declaring_reads_its_declarations() { + let backend = Backend::new_headless(); + let uri = "file:///config/services.yaml"; + let map = Arc::new(SymbolMap { + spans: vec![crate::symbol_map::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.files.write().insert( + uri.to_string(), + Arc::new(crate::file_state::FileState { + symbol_map: Some(Arc::clone(&map)), + ..Default::default() + }), + ); + AFTER_GENERATION_MOVES.set(Some(|backend| { + backend.container_index(); + })); + backend.note_symfony_declarations(uri, &map); + assert!(AFTER_GENERATION_MOVES.take().is_none()); + assert!( + backend + .container_index() + .services + .contains_key(&crate::atom::atom("app.mailer")) + ); + } } diff --git a/src/symfony/mod.rs b/src/symfony/mod.rs index 0ae1e7387..a6e66f78c 100644 --- a/src/symfony/mod.rs +++ b/src/symfony/mod.rs @@ -252,21 +252,22 @@ impl Backend { self.note_drupal_plugin_sources(uri, Some(map)); self.note_drupal_entity_type_test_sources(uri, Some(map)); self.note_drupal_theme_suggestions(map); - self.note_container_declarations(uri, declares.then_some(map)); let was_declaring = self.symfony.declaring_uris.read().contains(uri); if was_declaring || !map.event_constant_keys.is_empty() { self.forget_event_constant_names(); } // Only take the write lock when the answer changes. - if was_declaring == declares { - return; - } - let mut uris = self.symfony.declaring_uris.write(); - if declares { - uris.insert(uri.to_string()); - } else { - uris.remove(uri); + if was_declaring != declares { + let mut uris = self.symfony.declaring_uris.write(); + if declares { + uris.insert(uri.to_string()); + } else { + uris.remove(uri); + } } + // After the declaring files: a lookup that reads the container's + // new generation builds its index from them. + self.note_container_declarations(uri, declares.then_some(map)); } /// Forget a file that no longer exists. @@ -277,11 +278,11 @@ impl Backend { self.note_drupal_library_builder_sources(uri, None); self.note_drupal_plugin_sources(uri, None); self.note_drupal_entity_type_test_sources(uri, None); - self.note_container_declarations(uri, None); if self.symfony.declaring_uris.read().contains(uri) { self.symfony.declaring_uris.write().remove(uri); self.forget_event_constant_names(); } + self.note_container_declarations(uri, None); } /// The names of the class constants the project's subscribers key their diff --git a/tests/integration/drupal_entities.rs b/tests/integration/drupal_entities.rs index 82c7b32bd..721b57c85 100644 --- a/tests/integration/drupal_entities.rs +++ b/tests/integration/drupal_entities.rs @@ -75,6 +75,51 @@ async fn an_entity_type_id_goes_to_its_declaration_and_lists_its_readers() { ); } +#[tokio::test] +async fn a_lookup_in_a_file_opened_before_the_index_is_read_again_once_it_is_built() { + let manager_class = "getStorage('acme_note'); + } +} +"#; + let consumer_path = "web/modules/custom/acme/src/StaticNotes.php"; + let dir = tempfile::tempdir().unwrap(); + for (path, content) in [ + (MANAGER_PATH, MANAGER), + ( + "web/core/lib/Drupal/Core/Entity/EntityTypeManager.php", + manager_class, + ), + ("web/core/core.services.yml", core_services), + (NOTE_PATH, NOTE), + ] { + let full = dir.path().join(path); + std::fs::create_dir_all(full.parent().unwrap()).unwrap(); + std::fs::write(full, content).unwrap(); + } + // The editor restores the tab before the workspace index runs, and a + // request reads the file against a container nothing has filled yet. + let (backend, dir) = workspace_in(dir, &[(consumer_path, consumer)]).await; + let php = uri_for(&dir, consumer_path); + let position = position_in(consumer, "getStorage('acme_note')", 13); + let _ = definitions_at(&backend, &php, position).await; + backend.ensure_workspace_indexed(); + + let found = definitions_at(&backend, &php, position).await; + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!( + (&found[0].uri, found[0].range.start.line), + (&uri_for(&dir, NOTE_PATH), 6) + ); +} + /// Core's entity classes sit in a folder the project's `.gitignore` leaves /// out, and its test modules declare entity types no real site has. #[tokio::test] diff --git a/tests/integration/drupal_plugins.rs b/tests/integration/drupal_plugins.rs index 31260f059..b508ec557 100644 --- a/tests/integration/drupal_plugins.rs +++ b/tests/integration/drupal_plugins.rs @@ -528,3 +528,68 @@ async fn test_code_is_not_checked_for_plugins() { let (backend, dir) = site_with(UNKNOWN_PLUGIN_SITE, &[(path, LOOKUPS)]).await; assert!(unknown_plugins(&backend, &dir, path, LOOKUPS).is_empty()); } + +/// The editor restores the tab before the workspace index runs, when no +/// plugin manager has been read yet. +#[tokio::test] +async fn a_plugin_id_in_a_file_opened_before_the_index_goes_to_its_class_once_it_is_built() { + let dir = tempfile::tempdir().unwrap(); + for (path, content) in CORE.iter().chain(&[ + (".git/HEAD", "ref: refs/heads/main\n"), + (".gitignore", "/web/core/\n"), + (HELLO_PATH, HELLO), + ]) { + let full = dir.path().join(path); + std::fs::create_dir_all(full.parent().unwrap()).unwrap(); + std::fs::write(full, content).unwrap(); + } + let (backend, dir) = workspace_in(dir, &[(CONSUMER_PATH, CONSUMER)]).await; + let php = uri_for(&dir, CONSUMER_PATH); + let position = position_in(CONSUMER, "'acme_hello')", 2); + let _ = definitions_at(&backend, &php, position).await; + backend.ensure_workspace_indexed(); + + let found = definitions_at(&backend, &php, position).await; + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!( + (&found[0].uri, found[0].range.start.line), + (&uri_for(&dir, HELLO_PATH), 6) + ); +} + +/// A plugin ID goes to its class only while every class the service may +/// be is the manager, and so follows a service provider as it is edited. +#[tokio::test] +async fn a_plugin_id_follows_a_service_provider_as_it_is_edited() { + let consumer_path = "web/modules/custom/acme/src/Things.php"; + let consumer = "createInstance('acme_hello');\n }\n}\n"; + let (backend, dir) = site_with( + &[ + ( + "web/modules/custom/acme/acme.services.yml", + "services:\n acme.blocks:\n class: Drupal\\Core\\Block\\BlockManager\n", + ), + ( + "web/modules/custom/acme/src/Blocks.php", + "getDefinition('acme.blocks')->setClass(Blocks::class);\n }\n}\n"; + crate::common::open_document(&backend, &provider_uri, "php", provider).await; + assert!(definitions_at(&backend, &php, position).await.is_empty()); + + let edited = provider.replace("setClass(Blocks::class)", "setPublic(true)"); + backend.update_ast(provider_uri.as_str(), &edited); + let found = definitions_at(&backend, &php, position).await; + assert_eq!(found.len(), 1, "{found:?}"); + assert_eq!( + (&found[0].uri, found[0].range.start.line), + (&uri_for(&dir, HELLO_PATH), 6) + ); +}