Skip to content

Commit 3a0f70f

Browse files
committed
Allow diagnostics for vendor files, and fix overlapping error
supression.
1 parent 2817162 commit 3a0f70f

4 files changed

Lines changed: 108 additions & 130 deletions

File tree

docs/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
4949

5050
### Fixed
5151

52+
- **Diagnostics now work for vendor files open in the editor.** Projects using `--prefer-source` or monorepo setups where libraries are edited from the `vendor/` directory no longer have diagnostics suppressed. Previously all vendor files were unconditionally skipped, hiding syntax errors, deprecation warnings, and all other diagnostics. The `analyze` command also accepts vendor paths as explicit overrides (e.g. `phpantom_lsp analyze vendor/some-package/src/`).
53+
- **Full-line diagnostic suppression no longer requires a code mapping.** Full-line diagnostics (from tools like PHPStan that only report line numbers) are now suppressed whenever any precise diagnostic exists on the same line, regardless of error codes. Previously suppression relied on a hand-maintained mapping between error codes from different tools, which was incomplete and allowed redundant full-line underlines to obscure the precise location of the issue.
54+
5255
- **Null narrowing from `!== null` checks in conditions.** When a null-initialized variable was guarded by `$var !== null` in an `if` or `while` condition, the variable still showed `null` in its type inside the condition's `&&` operands and inside the then-body. The `!== null` check (and `!is_null()`, bare truthy guards) now narrows away `null` both for subsequent `&&` operands and inside the corresponding body block. Chained conditions like `$a !== null && $b !== null && $a->method()` narrow all checked variables. Conditions wrapped inside ternary expressions and return statements are also handled.
5356
- **Variables assigned inside `if`/`while` conditions now resolve in the body.** `if ($admin = AdminUser::first())` and `while ($row = nextRow())` now register the assignment so the variable has a type inside the loop or branch body. Assignments wrapped in comparisons like `if (($conn = getConn()) !== null)` are also recognized.
5457
- **Fluent chains only flag the first broken link.** In a chain like `$m->callHome()->callMom()->callDad()` where `callHome` does not exist, only `callHome` is flagged. Previously every subsequent link received its own "cannot verify" warning, burying the root cause in noise. Separate statements on the same variable (`$m->callHome(); $m->callMom();`) still flag independently. Scalar member access chains (`$user->getAge()->value->deep`) flag only the first scalar break. Null-safe, static, and mixed-operator chains are all handled.

docs/todo/performance.md

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -437,9 +437,13 @@ split into three tiers based on how the file is used:
437437
should not fire when fqn_index is fully populated. Go-to-definition
438438
can re-parse on demand using the file path from class_index.
439439

440-
- **Vendor files are never edited, never diagnosed.** They only
441-
need ClassInfo for type resolution and class_index for
442-
go-to-definition file lookup.
440+
- **Vendor files are rarely edited but can be diagnosed.** Users
441+
working in monorepos or with `--prefer-source` packages edit
442+
vendor files directly, and diagnostics run on any file open in
443+
the editor. Tiered storage must still keep enough data to
444+
support diagnostics for open vendor files, but non-open vendor
445+
files only need ClassInfo for type resolution and class_index
446+
for go-to-definition file lookup.
443447

444448
- **Go-to-implementation currently scans all ast_map entries.**
445449
A dedicated GTI index (parent FQN to list of child FQNs, built

src/analyse.rs

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -472,9 +472,29 @@ pub(crate) fn discover_user_files(
472472

473473
let vendor_dirs: Vec<PathBuf> = backend.vendor_dir_paths.lock().clone();
474474

475+
// When an explicit path filter points outside all PSR-4 source
476+
// directories (e.g. into vendor/), walk the filter path directly
477+
// instead of skipping it. This matches PHPStan behaviour: the
478+
// default scan covers only user code, but an explicit override
479+
// scans whatever you point it at.
480+
let filter_overlaps_psr4 = abs_filter.as_ref().is_none_or(|fp| {
481+
source_dirs
482+
.iter()
483+
.any(|d| d.starts_with(fp) || fp.starts_with(d))
484+
});
485+
486+
let dirs_to_walk: Vec<&Path> = if filter_overlaps_psr4 {
487+
source_dirs.iter().map(|p| p.as_path()).collect()
488+
} else {
489+
// The filter path doesn't overlap any PSR-4 dir — walk it
490+
// directly (no vendor exclusion since the user explicitly
491+
// asked for this path).
492+
vec![abs_filter.as_deref().unwrap()]
493+
};
494+
475495
let mut files: Vec<PathBuf> = Vec::new();
476496

477-
for dir in &source_dirs {
497+
for dir in &dirs_to_walk {
478498
// If a directory filter is active and doesn't overlap with
479499
// this source dir, skip entirely.
480500
if let Some(ref fp) = abs_filter
@@ -485,7 +505,13 @@ pub(crate) fn discover_user_files(
485505
continue;
486506
}
487507

488-
let skip_vendor = vendor_dirs.clone();
508+
let skip_vendor = if filter_overlaps_psr4 {
509+
vendor_dirs.clone()
510+
} else {
511+
// User explicitly targeted this path — don't skip vendor
512+
// subdirectories within it.
513+
Vec::new()
514+
};
489515
let walker = WalkBuilder::new(dir)
490516
.git_ignore(true)
491517
.git_global(true)
@@ -495,6 +521,7 @@ pub(crate) fn discover_user_files(
495521
.ignore(true)
496522
.filter_entry(move |entry| {
497523
if entry.file_type().is_some_and(|ft| ft.is_dir())
524+
&& !skip_vendor.is_empty()
498525
&& let Ok(canonical) = entry.path().canonicalize()
499526
&& skip_vendor.iter().any(|v| canonical.starts_with(v))
500527
{

src/diagnostics/mod.rs

Lines changed: 69 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -121,13 +121,12 @@ use crate::util::ranges_overlap;
121121

122122
impl Backend {
123123
/// Returns `true` if the URI should be skipped for diagnostics
124-
/// (stub files and vendor files).
124+
/// (stub files only). Vendor files are not skipped because
125+
/// diagnostics only run on files the user has open in the editor,
126+
/// and users working in monorepos or with `--prefer-source`
127+
/// packages legitimately edit vendor files.
125128
fn should_skip_diagnostics(&self, uri_str: &str) -> bool {
126-
if uri_str.starts_with("phpantom-stub://") || uri_str.starts_with("phpantom-stub-fn://") {
127-
return true;
128-
}
129-
let prefixes = self.vendor_uri_prefixes.lock();
130-
prefixes.iter().any(|p| uri_str.starts_with(p.as_str()))
129+
uri_str.starts_with("phpantom-stub://") || uri_str.starts_with("phpantom-stub-fn://")
131130
}
132131

133132
/// Collect Phase 1 (fast) diagnostics: syntax errors, unused
@@ -1067,8 +1066,23 @@ impl Backend {
10671066
/// 1. `unknown_class` trumps `unresolved_member_access`
10681067
/// 2. `unknown_member` trumps `unresolved_member_access`
10691068
/// 3. `scalar_member_access` trumps `unresolved_member_access`
1070-
/// 4. Any precise (sub-line) diagnostic suppresses full-line diagnostics
1071-
/// on the same line (PHPStan only reports line numbers).
1069+
/// 4. Full-line diagnostics are suppressed when any precise (sub-line)
1070+
/// diagnostic exists on the same line.
1071+
///
1072+
/// **Why rule 4 exists.** Diagnostics arrive from multiple independent
1073+
/// sources (Mago parser, PHPStan, native PHPantom checks) that use
1074+
/// completely different error codes and descriptions. There is no
1075+
/// reliable way to determine whether two diagnostics from different
1076+
/// sources describe the same issue. What we *can* determine is
1077+
/// precision: tools like PHPStan only report a line number, so their
1078+
/// diagnostics span the entire line (character 0 to a very large end
1079+
/// character). Native diagnostics and parser errors pinpoint the exact
1080+
/// token. A full-line underline obscures the precise location, making
1081+
/// it harder for the developer to spot the problem. Suppressing it
1082+
/// unconditionally when any precise diagnostic exists on the same line
1083+
/// keeps the pinpointed one visible without losing information. Once
1084+
/// the precise diagnostic is resolved, the full-line one reappears
1085+
/// automatically (if the underlying issue persists).
10721086
///
10731087
/// Each source's diagnostics are authoritative: if PHPStan reports five
10741088
/// issues on a line, all five are shown; if PHPantom reports two issues
@@ -1127,27 +1141,14 @@ fn deduplicate_diagnostics(diagnostics: &mut Vec<Diagnostic>) {
11271141
// diagnostic. A diagnostic is "precise" when it does not span the
11281142
// entire line, i.e. it has a meaningful character range rather than
11291143
// `0..MAX`. External tools like PHPStan only report a line number,
1130-
// so their diagnostics stretch the full line. When a native
1131-
// diagnostic already pinpoints the exact location on that line *and*
1132-
// reports a related issue, the full-line underline is redundant
1133-
// noise. We only suppress a full-line diagnostic when a precise
1134-
// diagnostic on the same line has a related code.
1135-
let mut precise_diags_by_line: std::collections::HashMap<u32, Vec<String>> =
1136-
std::collections::HashMap::new();
1144+
// so their diagnostics stretch the full line. A full-line underline
1145+
// obscures the precise location and makes it harder for the
1146+
// developer to spot the problem, so we suppress it unconditionally
1147+
// when any precise diagnostic exists on the same line.
1148+
let mut lines_with_precise: std::collections::HashSet<u32> = std::collections::HashSet::new();
11371149
for d in diagnostics.iter() {
11381150
if !is_full_line_range(&d.range) {
1139-
let code = d
1140-
.code
1141-
.as_ref()
1142-
.map(|c| match c {
1143-
NumberOrString::String(s) => s.clone(),
1144-
NumberOrString::Number(n) => n.to_string(),
1145-
})
1146-
.unwrap_or_default();
1147-
precise_diags_by_line
1148-
.entry(d.range.start.line)
1149-
.or_default()
1150-
.push(code);
1151+
lines_with_precise.insert(d.range.start.line);
11511152
}
11521153
}
11531154

@@ -1168,33 +1169,11 @@ fn deduplicate_diagnostics(diagnostics: &mut Vec<Diagnostic>) {
11681169
.any(|pr| ranges_overlap(pr, &d.range));
11691170
}
11701171

1171-
// Suppress full-line diagnostics on lines where a more precise
1172-
// diagnostic already covers the same issue. This avoids the
1173-
// visual clutter of a line-wide PHPStan underline next to a
1174-
// pinpointed native error. The suppressed diagnostic will
1175-
// reappear once the user resolves the precise one.
1176-
//
1177-
// Only suppress when the precise diagnostic is *related* to
1178-
// the full-line one. Unrelated diagnostics (e.g. PHPStan's
1179-
// `class.prefixed` alongside a native `unknown_class`) must
1180-
// both be shown so their respective code actions are available.
1181-
if is_full_line_range(&d.range) {
1182-
let full_line_code = d
1183-
.code
1184-
.as_ref()
1185-
.map(|c| match c {
1186-
NumberOrString::String(s) => s.as_str(),
1187-
_ => "",
1188-
})
1189-
.unwrap_or("");
1190-
1191-
if let Some(precise_codes) = precise_diags_by_line.get(&d.range.start.line)
1192-
&& precise_codes
1193-
.iter()
1194-
.any(|pc| are_related_diagnostics(full_line_code, pc))
1195-
{
1196-
return false;
1197-
}
1172+
// Suppress full-line diagnostics when any precise diagnostic
1173+
// exists on the same line. See the doc comment on this
1174+
// function for the rationale.
1175+
if is_full_line_range(&d.range) && lines_with_precise.contains(&d.range.start.line) {
1176+
return false;
11981177
}
11991178

12001179
true
@@ -1219,76 +1198,6 @@ fn is_full_line_range(range: &Range) -> bool {
12191198
range.start.line == range.end.line && range.start.character == 0 && range.end.character >= 1000
12201199
}
12211200

1222-
/// Check whether two diagnostic codes report related issues such that
1223-
/// the full-line diagnostic is redundant when the precise one is
1224-
/// present.
1225-
///
1226-
/// For example, PHPStan's `argument.type` and a native `unknown_class`
1227-
/// on the same line are related (the class error causes the argument
1228-
/// mismatch). But PHPStan's `class.prefixed` and a native
1229-
/// `unknown_class` are unrelated issues that need separate fixes.
1230-
fn are_related_diagnostics(full_line_code: &str, precise_code: &str) -> bool {
1231-
// A native diagnostic that already covers the exact same concept
1232-
// makes the full-line PHPStan diagnostic redundant.
1233-
//
1234-
// Group 1: class existence / resolution errors.
1235-
const CLASS_RELATED: &[&str] = &["unknown_class", "class.notFound", "class.nameCase"];
1236-
// Group 2: member access errors.
1237-
const MEMBER_RELATED: &[&str] = &[
1238-
"unknown_member",
1239-
"unresolved_member_access",
1240-
"scalar_member_access",
1241-
"method.notFound",
1242-
"staticMethod.notFound",
1243-
"property.notFound",
1244-
"constant.notFound",
1245-
"access.property",
1246-
"access.staticProperty",
1247-
];
1248-
// Group 3: function call errors.
1249-
const FUNCTION_RELATED: &[&str] = &["unknown_function", "function.notFound"];
1250-
1251-
// If both codes belong to the same group, they are related.
1252-
for group in &[CLASS_RELATED as &[&str], MEMBER_RELATED, FUNCTION_RELATED] {
1253-
let full_in = group.contains(&full_line_code);
1254-
let precise_in = group.contains(&precise_code);
1255-
if full_in && precise_in {
1256-
return true;
1257-
}
1258-
}
1259-
1260-
// PHPStan diagnostics that are consequences of a class/member not
1261-
// being found are redundant when the native diagnostic already
1262-
// flags the root cause. For example, `argument.type` or
1263-
// `return.type` caused by an unknown class.
1264-
const CONSEQUENCE_CODES: &[&str] = &[
1265-
"argument.type",
1266-
"return.type",
1267-
"assign.propertyType",
1268-
"isset.property",
1269-
"deadCode.unreachable",
1270-
];
1271-
const ROOT_CAUSE_CODES: &[&str] = &[
1272-
"unknown_class",
1273-
"unknown_member",
1274-
"unknown_function",
1275-
"scalar_member_access",
1276-
"unresolved_member_access",
1277-
];
1278-
1279-
if CONSEQUENCE_CODES.contains(&full_line_code) && ROOT_CAUSE_CODES.contains(&precise_code) {
1280-
return true;
1281-
}
1282-
1283-
// Same code — always related (e.g. PHPStan `class.notFound` and
1284-
// native `class.notFound` on the same line).
1285-
if full_line_code == precise_code {
1286-
return true;
1287-
}
1288-
1289-
false
1290-
}
1291-
12921201
// ── Helpers ─────────────────────────────────────────────────────────────────
12931202

12941203
/// Build a diagnostic range from byte offsets, returning `None` if either
@@ -1543,6 +1452,11 @@ mod tests {
15431452

15441453
#[test]
15451454
fn suppresses_full_line_phpstan_when_precise_diagnostic_on_same_line() {
1455+
// A full-line diagnostic (from a tool that only reports line
1456+
// numbers) is suppressed when any precise diagnostic exists on
1457+
// the same line, regardless of error codes. The precise
1458+
// diagnostic pinpoints the exact location; the full-line
1459+
// underline just adds noise.
15461460
let phpstan = Diagnostic {
15471461
range: make_range(5, 0, 5, u32::MAX),
15481462
severity: Some(DiagnosticSeverity::ERROR),
@@ -1565,6 +1479,35 @@ mod tests {
15651479
assert_eq!(diags[0].message, precise.message);
15661480
}
15671481

1482+
#[test]
1483+
fn suppresses_full_line_regardless_of_code() {
1484+
// Suppression is unconditional — we cannot reliably determine
1485+
// whether diagnostics from different tools (Mago parser,
1486+
// PHPStan, native PHPantom) describe the same issue because
1487+
// they use completely different error codes and descriptions.
1488+
// Any precise diagnostic on the same line is enough.
1489+
let phpstan = Diagnostic {
1490+
range: make_range(5, 0, 5, u32::MAX),
1491+
severity: Some(DiagnosticSeverity::ERROR),
1492+
code: Some(NumberOrString::String("class.prefixed".to_string())),
1493+
source: Some("phpstan".to_string()),
1494+
message: "Class prefixed with vendor namespace.".to_string(),
1495+
..Default::default()
1496+
};
1497+
let syntax_error = Diagnostic {
1498+
range: make_range(5, 3, 5, 10),
1499+
severity: Some(DiagnosticSeverity::ERROR),
1500+
code: Some(NumberOrString::String("syntax_error".to_string())),
1501+
source: Some("phpantom".to_string()),
1502+
message: "Syntax error: unexpected token `->`".to_string(),
1503+
..Default::default()
1504+
};
1505+
let mut diags = vec![phpstan, syntax_error.clone()];
1506+
deduplicate_diagnostics(&mut diags);
1507+
assert_eq!(diags.len(), 1);
1508+
assert_eq!(diags[0].message, syntax_error.message);
1509+
}
1510+
15681511
#[test]
15691512
fn keeps_full_line_phpstan_when_no_precise_diagnostic_on_line() {
15701513
let phpstan = Diagnostic {
@@ -1668,7 +1611,8 @@ mod tests {
16681611

16691612
#[test]
16701613
fn keeps_multiple_phpstan_diagnostics_on_same_line() {
1671-
// If PHPStan reports five issues on a line, all five survive.
1614+
// If PHPStan reports five issues on a line and no precise
1615+
// diagnostic exists, all five survive.
16721616
let make_phpstan = |code: &str, msg: &str| Diagnostic {
16731617
range: make_range(10, 0, 10, u32::MAX),
16741618
severity: Some(DiagnosticSeverity::ERROR),

0 commit comments

Comments
 (0)