diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index dad4adb9..7af4fa13 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Drupal project support.** Drupal projects are detected via `composer.json` (`drupal/core`, `drupal/core-recommended`, or `drupal/core-dev`). The web root is resolved from `extra.drupal-scaffold.locations.web-root` with a filesystem fallback. Drupal-specific directories (`core`, `modules/contrib`, `modules/custom`, `themes/contrib`, `themes/custom`, `profiles`, `sites`) are scanned with `.gitignore` bypassed so that Composer-managed code is always indexed. Drupal PHP extensions (`.module`, `.install`, `.theme`, `.profile`, `.inc`, `.engine`) are recognized as PHP source. Contributed by @syntlyx in https://github.com/AJenbo/phpantom_lsp/pull/52. +- **`@phpstan-assert-if-true $this` narrowing.** Instance methods annotated with `@phpstan-assert-if-true` or `@phpstan-assert-if-false` targeting `$this` now narrow the receiver variable in the corresponding branch. For example, `if ($app->isTestApp())` narrows `$app` to the asserted subtype inside the then-body. Contributed by @syntlyx in https://github.com/AJenbo/phpantom_lsp/pull/52. - **Fix unsafe `new static()` code action.** When PHPStan reports `new.static`, three quickfixes are offered: add `@phpstan-consistent-constructor` to the class docblock (preferred), add `final` to the class, or add `final` to the constructor. The diagnostic is eagerly cleared after applying any of the fixes. - **Keyword completions.** Context-aware PHP keyword suggestions in statement positions. Keywords are filtered by scope: `return` and `yield` only inside functions, `break` only inside loops and `switch`, `continue` only inside loops, `case` and `default` only inside `switch`, `namespace` only at the top level, and `extends`/`implements` only in declaration headers. Class-like bodies restrict completions to member keywords (`public`, `function`, `const`, etc.) appropriate for the specific kind (class, interface, trait, or enum). Enum backing types (`int`, `string`) are suggested after `enum Name:`. Modifier chains (`public static `) trigger member-keyword completions. Contributed by @ryangjchandler in https://github.com/AJenbo/phpantom_lsp/pull/43. - **Attribute completion.** Typing inside `#[…]` now only offers classes decorated with `#[\Attribute]`, filtered by the target of the declaration the attribute applies to. An attribute targeting only methods will not appear when writing `#[…]` above a class, and vice versa. Multi-attribute lists (`#[A, B]`) and namespace-qualified prefixes are supported. diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 64164fa0..52ba7375 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -39,6 +39,10 @@ Note that clippy runs twice, once for library code and once including test code. See [BUILDING.md](BUILDING.md) for more on running tests and manual LSP testing. +## Changelog + +Update [CHANGELOG.md](CHANGELOG.md) when your PR adds, changes, or fixes something a user would notice. Add entries under `## [Unreleased]` in the appropriate subsection (`### Added`, `### Fixed`, `### Changed`, or `### Removed`). Write for end users, not developers: describe what changed in the editor, not which internal modules were touched. See the existing entries for the style and level of detail expected. + ## Reporting Issues Open an issue on GitHub with: diff --git a/src/classmap_scanner.rs b/src/classmap_scanner.rs index 21abe1e4..143ff5da 100644 --- a/src/classmap_scanner.rs +++ b/src/classmap_scanner.rs @@ -680,6 +680,83 @@ pub fn scan_workspace_fallback_full( scan_files_parallel_full(&php_files) } +/// Scan Drupal-specific directories for PHP symbols, bypassing `.gitignore`. +/// +/// Drupal projects typically exclude their web root directories +/// (`web/core`, `web/modules/contrib`, etc.) from version control via +/// `.gitignore` because those files are managed by Composer. The normal +/// gitignore-aware walkers would therefore silently skip the most important +/// parts of the codebase. This function walks with gitignore **disabled** +/// so that those directories are always indexed. +/// +/// In addition to `.php` files, Drupal uses several other file extensions +/// for valid PHP source: `.module`, `.install`, `.theme`, `.profile`, +/// `.inc`, and `.engine`. All are included by this scanner. +/// +/// Test directories (`tests/` and `Tests/`) are excluded by name to avoid +/// indexing duplicate class definitions from unit-test fixtures. +pub fn scan_drupal_directories(web_root: &Path) -> WorkspaceScanResult { + use ignore::WalkBuilder; + + let drupal_dirs = [ + "core", + "modules/contrib", + "modules/custom", + "themes/contrib", + "themes/custom", + "profiles", + "sites", + ]; + + let mut php_files: Vec = Vec::new(); + + for rel in &drupal_dirs { + let dir = web_root.join(rel); + if !dir.exists() { + continue; + } + + let walker = WalkBuilder::new(&dir) + // Gitignore is intentionally disabled — Drupal's .gitignore + // excludes web/core and web/modules/contrib which are the + // most critical directories to index. + .git_ignore(false) + .git_global(false) + .git_exclude(false) + .hidden(true) // still skip .git, .idea, etc. + .parents(false) + .ignore(false) + .filter_entry(|entry| { + if entry.file_type().is_some_and(|ft| ft.is_dir()) { + let name = entry.file_name().to_str().unwrap_or(""); + // Exclude test directories (both conventional casings) + if name == "tests" || name == "Tests" { + return false; + } + } + true + }) + .build(); + + for entry in walker.flatten() { + let path = entry.path(); + if path.is_file() && is_drupal_php_file(path) { + php_files.push(path.to_path_buf()); + } + } + } + + scan_files_parallel_full(&php_files) +} + +/// Return `true` for file extensions that Drupal treats as PHP source. +fn is_drupal_php_file(path: &Path) -> bool { + matches!( + path.extension().and_then(|e| e.to_str()), + Some("php" | "module" | "install" | "theme" | "profile" | "inc" | "engine") + ) +} + // ─── Core scanner ─────────────────────────────────────────────────────────── /// The **full-scan**: a single-pass byte-level scanner that extracts diff --git a/src/completion/types/narrowing.rs b/src/completion/types/narrowing.rs index 17876c0f..6a215ef4 100644 --- a/src/completion/types/narrowing.rs +++ b/src/completion/types/narrowing.rs @@ -881,10 +881,6 @@ pub(in crate::completion) fn try_apply_assert_condition_narrowing( Expression::Call(c) => c, _ => return, }; - let info = match extract_call_assertions(call, ctx) { - Some(info) => info, - None => return, - }; // Determine whether the function returned true in this branch. // @@ -894,33 +890,125 @@ pub(in crate::completion) fn try_apply_assert_condition_narrowing( // - else-body (inverted=true), negated → function returned true let function_returned_true = !(inverted ^ condition_negated); - for assertion in info.assertions { - // Determine if this assertion's condition is satisfied in this - // branch. IfTrue assertions apply positively when the function - // returned true; IfFalse assertions apply positively when the - // function returned false. In the opposite branch, we apply - // the *inverse* (exclude instead of include, and vice-versa). - let applies_positively = match assertion.kind { - AssertionKind::IfTrue => function_returned_true, - AssertionKind::IfFalse => !function_returned_true, - AssertionKind::Always => continue, // handled elsewhere - }; + if let Some(info) = extract_call_assertions(call, ctx) { + for assertion in info.assertions { + // Determine if this assertion's condition is satisfied in this + // branch. IfTrue assertions apply positively when the function + // returned true; IfFalse assertions apply positively when the + // function returned false. In the opposite branch, we apply + // the *inverse* (exclude instead of include, and vice-versa). + let applies_positively = match assertion.kind { + AssertionKind::IfTrue => function_returned_true, + AssertionKind::IfFalse => !function_returned_true, + AssertionKind::Always => continue, // handled elsewhere + }; - if let Some(arg_var) = - find_assertion_arg_variable(info.argument_list, &assertion.param_name, info.parameters) - && arg_var == ctx.var_name - { - // XOR the assertion's own negation with whether we're in the - // opposite branch: positive + non-negated → include, - // positive + negated → exclude, opposite + non-negated → exclude, - // opposite + negated → include. - let should_exclude = assertion.negated ^ !applies_positively; - if should_exclude { - apply_instanceof_exclusion(&assertion.asserted_type, ctx, results); - } else { - apply_instanceof_inclusion(&assertion.asserted_type, false, ctx, results); + if let Some(arg_var) = find_assertion_arg_variable( + info.argument_list, + &assertion.param_name, + info.parameters, + ) && arg_var == ctx.var_name + { + // XOR the assertion's own negation with whether we're in the + // opposite branch: positive + non-negated → include, + // positive + negated → exclude, opposite + non-negated → exclude, + // opposite + negated → include. + let should_exclude = assertion.negated ^ !applies_positively; + if should_exclude { + apply_instanceof_exclusion(&assertion.asserted_type, ctx, results); + } else { + apply_instanceof_inclusion(&assertion.asserted_type, false, ctx, results); + } } } + } else { + // Handle instance method calls: `$var->method()` where the method + // carries `@phpstan-assert-if-true $this` (or the -if-false / psalm + // variants). The receiver variable itself is the assertion subject. + apply_this_assert_condition_narrowing(call, ctx, results, function_returned_true); + } +} + +/// Apply `@phpstan-assert-if-true $this` / `@phpstan-assert-if-false $this` +/// narrowing for instance method calls used as an `if` condition. +/// +/// Handles the pattern: +/// ```php +/// if ($app->isTestApp()) { +/// $app->testMethod(); // $app narrowed to TestApplication +/// } +/// ``` +/// where `isTestApp()` is annotated with +/// `@phpstan-assert-if-true \TestApplication $this`. +/// +/// The receiver's current known types (from `results`) are searched for a +/// matching method with `$this` assertions. Assertions are collected first +/// and applied after the iteration to avoid borrow-check conflicts. +fn apply_this_assert_condition_narrowing( + call: &Call<'_>, + ctx: &VarResolutionCtx<'_>, + results: &mut Vec, + function_returned_true: bool, +) { + // Extract receiver expression and method name from the call. + let (receiver, method_name) = match call { + Call::Method(mc) => ( + mc.object, + match &mc.method { + ClassLikeMemberSelector::Identifier(ident) => ident.value, + _ => return, + }, + ), + Call::NullSafeMethod(mc) => ( + mc.object, + match &mc.method { + ClassLikeMemberSelector::Identifier(ident) => ident.value, + _ => return, + }, + ), + _ => return, + }; + + // The receiver must be the variable we are currently narrowing. + let receiver_key = match expr_to_subject_key(receiver) { + Some(k) => k, + None => return, + }; + if receiver_key != ctx.var_name { + return; + } + + // Collect (asserted_type, should_exclude) pairs from every current + // candidate class that declares the method with a $this assertion. + // We collect before mutating `results` to avoid borrow conflicts. + let mut to_apply: Vec<(String, bool)> = Vec::new(); + for class_info in results.iter() { + let method = class_info + .methods + .iter() + .find(|m| m.name == method_name && !m.is_static); + if let Some(method) = method { + for assertion in &method.type_assertions { + if assertion.param_name != "$this" { + continue; + } + let applies_positively = match assertion.kind { + AssertionKind::IfTrue => function_returned_true, + AssertionKind::IfFalse => !function_returned_true, + AssertionKind::Always => continue, + }; + let should_exclude = assertion.negated ^ !applies_positively; + to_apply.push((assertion.asserted_type.clone(), should_exclude)); + } + } + } + + for (asserted_type, should_exclude) in to_apply { + if should_exclude { + apply_instanceof_exclusion(&asserted_type, ctx, results); + } else { + apply_instanceof_inclusion(&asserted_type, false, ctx, results); + } } } diff --git a/src/composer.rs b/src/composer.rs index 2be0490c..cc9fc4cc 100644 --- a/src/composer.rs +++ b/src/composer.rs @@ -26,6 +26,7 @@ use std::fs; use std::path::{Path, PathBuf}; pub use mago_composer::ComposerPackage; +use mago_composer::ComposerPackageExtra; use crate::types::PhpVersion; @@ -839,6 +840,52 @@ pub(crate) fn has_require_dev(package: &ComposerPackage, dep: &str) -> bool { package.require_dev.contains_key(dep) } +/// Detect whether the project is a Drupal project and resolve the web root. +/// +/// Returns `Some(web_root)` if one of the canonical Drupal core packages is +/// listed in `require` or `require-dev`, and we can determine the web root. +/// +/// **Web-root resolution order:** +/// 1. `extra.drupal-scaffold.locations.web-root` in `composer.json`. +/// 2. Filesystem fallback: the first of `web/`, `docroot/`, `public/`, +/// `html/` that contains `core/lib/Drupal.php`. +pub(crate) fn detect_drupal_web_root( + workspace_root: &Path, + package: &ComposerPackage, +) -> Option { + const DRUPAL_PACKAGES: &[&str] = &["drupal/core", "drupal/core-recommended", "drupal/core-dev"]; + + let is_drupal = DRUPAL_PACKAGES + .iter() + .any(|pkg| package.require.contains_key(*pkg) || package.require_dev.contains_key(*pkg)); + + if !is_drupal { + return None; + } + + // 1. Read from extra.drupal-scaffold.locations.web-root + if let Some(ComposerPackageExtra::Object(extra_map)) = &package.extra + && let Some(scaffold) = extra_map.get("drupal-scaffold") + && let Some(web_root_str) = scaffold + .get("locations") + .and_then(|l| l.get("web-root")) + .and_then(|v| v.as_str()) + { + let path = workspace_root.join(web_root_str.trim_end_matches('/')); + return Some(path); + } + + // 2. Filesystem fallback: find which common dir contains core/lib/Drupal.php + for candidate in &["web", "docroot", "public", "html"] { + let path = workspace_root.join(candidate); + if path.join("core/lib/Drupal.php").exists() { + return Some(path); + } + } + + None +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/server.rs b/src/server.rs index ba183b12..808f7c2d 100644 --- a/src/server.rs +++ b/src/server.rs @@ -1120,6 +1120,44 @@ impl Backend { let symbol_count = classmap.len(); *self.classmap.write() = classmap; + // ── Drupal: scan web-root directories (gitignore bypassed) ── + // Drupal's .gitignore excludes web/core, web/modules/contrib, + // etc. because they are managed by Composer — but those paths + // contain every base interface and hook definition that modules + // depend on. detect_drupal_web_root() returns None for + // non-Drupal projects so this block is a no-op in that case. + if let Some(ref pkg) = composer_json + && let Some(drupal_web_root) = composer::detect_drupal_web_root(root, pkg) + { + let drupal_result = classmap_scanner::scan_drupal_directories(&drupal_web_root); + let drupal_count = drupal_result.classmap.len() + + drupal_result.function_index.len() + + drupal_result.constant_index.len(); + { + let mut cm = self.classmap.write(); + for (fqn, path) in drupal_result.classmap { + cm.entry(fqn).or_insert(path); + } + } + { + let mut fi = self.autoload_function_index.write(); + for (fqn, path) in drupal_result.function_index { + fi.entry(fqn).or_insert(path); + } + } + { + let mut ci = self.autoload_constant_index.write(); + for (name, path) in drupal_result.constant_index { + ci.entry(name).or_insert(path); + } + } + tracing::info!( + "PHPantom: Drupal web root {:?}, {} symbols indexed", + drupal_web_root, + drupal_count + ); + } + // ── PSR-0 (legacy) classmap ───────────────────────────────── // Packages that declare `autoload.psr-0` in their composer.json // (e.g. HTMLPurifier) are listed in `autoload_namespaces.php`. diff --git a/tests/integration/completion_variables.rs b/tests/integration/completion_variables.rs index 4815bfb6..924375ab 100644 --- a/tests/integration/completion_variables.rs +++ b/tests/integration/completion_variables.rs @@ -7578,6 +7578,84 @@ async fn test_completion_phpstan_assert_second_parameter() { } } +/// `@phpstan-assert-if-true $this` on an instance method narrows the +/// receiver variable in the then-body. +/// +/// Pattern (common in Drupal and other frameworks): +/// ```php +/// if ($app->isTestApp()) { +/// $app->testAppMethod(); // $app is TestApplication here +/// } +/// ``` +#[tokio::test] +async fn test_completion_phpstan_assert_if_true_this_narrows_receiver() { + let backend = create_test_backend(); + + let uri = Url::parse("file:///assert_if_true_this.php").unwrap(); + let text = concat!( + "isTestApp()) {\n", // 11 + " $app->\n", // 12 + " }\n", // 13 + "}\n", // 14 + ); + + backend + .did_open(DidOpenTextDocumentParams { + text_document: TextDocumentItem { + uri: uri.clone(), + language_id: "php".to_string(), + version: 1, + text: text.to_string(), + }, + }) + .await; + + let result = backend + .completion(CompletionParams { + text_document_position: TextDocumentPositionParams { + text_document: TextDocumentIdentifier { uri }, + position: Position { + line: 12, + character: 14, + }, + }, + work_done_progress_params: WorkDoneProgressParams::default(), + partial_result_params: PartialResultParams::default(), + context: None, + }) + .await + .unwrap(); + + assert!(result.is_some(), "Should return completions"); + match result.unwrap() { + CompletionResponse::Array(items) => { + let method_names: Vec<&str> = items + .iter() + .filter(|i| i.kind == Some(CompletionItemKind::METHOD)) + .map(|i| i.filter_text.as_deref().unwrap()) + .collect(); + + assert!( + method_names.contains(&"testAppMethod"), + "Should include TestApplication's 'testAppMethod' after @phpstan-assert-if-true $this narrowing, got: {:?}", + method_names + ); + } + _ => panic!("Expected CompletionResponse::Array"), + } +} + /// `@phpstan-assert-if-true` + `@phpstan-assert-if-false` on the same /// function: each applies in the correct branch. #[tokio::test]