Skip to content

Commit 7a572df

Browse files
committed
fix(analyzer): Handle multiple docblocks before a statements
Currently we're only handling docblocks that are tied to a statement, and only consider a single docblock to be tied to any given statement. This means that having multiple consecutive docblocks to declare variables, for example in a classic PHP-only template file, will have us only considering the last of these docblocks. We can improve this by treating all consecutive docblocks that preceed a given statement, only separated by whitespace or comments, to apply to that statement. This uncovered a bug in the docblock_var_on_non_assignment testcase which actually didn't test what it was supposed to test because the extra docblock was ignored. With the new behaviour, that test actually complains that the docblock is redundant because it doesn't narrow the known type. We can widen the original type and add a call that actually requires the type-narrowing docblock to be processed. We can also adjust the test case for issue #801 because we now fully support the original code.
1 parent eeb4090 commit 7a572df

File tree

4 files changed

+44
-47
lines changed

4 files changed

+44
-47
lines changed

crates/analyzer/src/context/mod.rs

Lines changed: 32 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use mago_codex::metadata::CodebaseMetadata;
55
use mago_codex::ttype::resolution::TypeResolutionContext;
66
use mago_collector::Collector;
77
use mago_database::file::File;
8-
use mago_docblock::document::Document;
8+
use mago_docblock::document::Element;
99
use mago_names::ResolvedNames;
1010
use mago_names::scope::NamespaceScope;
1111
use mago_reporting::Annotation;
@@ -137,46 +137,44 @@ impl<'ctx, 'arena> Context<'ctx, 'arena> {
137137
}
138138
}
139139

140-
pub fn get_docblock(&self) -> Option<&'arena Trivia<'arena>> {
141-
comments::docblock::get_docblock_before_position(
142-
self.source_file,
143-
self.comments,
144-
self.statement_span.start.offset,
145-
)
146-
}
147-
148-
pub fn get_parsed_docblock(&mut self) -> Option<Document<'arena>> {
149-
let trivia = self.get_docblock()?;
150-
151-
match mago_docblock::parse_trivia(self.arena, trivia) {
152-
Ok(document) => Some(document),
153-
Err(error) => {
154-
let error_span = error.span();
140+
pub fn get_parsed_docblocks(&mut self) -> Vec<Element<'arena>> {
141+
let mut elements = vec![];
142+
let mut start = self.statement_span.start.offset;
143+
while let Some(trivia) =
144+
comments::docblock::get_docblock_before_position(self.source_file, self.comments, start)
145+
{
146+
match mago_docblock::parse_trivia(self.arena, trivia) {
147+
Ok(document) => elements.extend(document.elements),
148+
Err(error) => {
149+
let error_span = error.span();
150+
151+
let mut issue = Issue::error(error.to_string())
152+
.with_annotation(
153+
Annotation::primary(error_span)
154+
.with_message("This part of the docblock has a syntax error"),
155+
)
156+
.with_note(error.note());
157+
158+
if trivia.span != error_span {
159+
issue = issue.with_annotation(
160+
Annotation::secondary(trivia.span).with_message("The error is within this docblock"),
161+
);
162+
}
155163

156-
let mut issue = Issue::error(error.to_string())
157-
.with_annotation(
158-
Annotation::primary(error_span).with_message("This part of the docblock has a syntax error"),
159-
)
160-
.with_note(error.note());
161-
162-
if trivia.span != error_span {
163164
issue = issue.with_annotation(
164-
Annotation::secondary(trivia.span).with_message("The error is within this docblock"),
165+
Annotation::secondary(self.statement_span)
166+
.with_message("This docblock is associated with the following statement"),
165167
);
166-
}
167168

168-
issue = issue.with_annotation(
169-
Annotation::secondary(self.statement_span)
170-
.with_message("This docblock is associated with the following statement"),
171-
);
169+
issue = issue.with_help(error.help());
172170

173-
issue = issue.with_help(error.help());
174-
175-
self.collector.report_with_code(IssueCode::InvalidDocblock, issue);
176-
177-
None
171+
self.collector.report_with_code(IssueCode::InvalidDocblock, issue);
172+
}
178173
}
174+
start = trivia.span.start.offset;
179175
}
176+
177+
elements
180178
}
181179

182180
pub fn record<T>(&mut self, callback: impl FnOnce(&mut Context<'ctx, 'arena>) -> T) -> (T, IssueCollection) {

crates/analyzer/src/utils/docblock.rs

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,10 @@ pub fn populate_docblock_variables_excluding<'ctx>(
7575
}
7676
}
7777

78-
/// Retrieves all `@var`, `@psalm-var`, and `@phpstan-var` tags from the docblock of the
78+
/// Retrieves all `@var`, `@psalm-var`, and `@phpstan-var` tags from the docblocks preceeding the
7979
/// current statement in the context, parsing their variable types.
8080
///
81-
/// This function scans the docblock associated with the current statement in the context,
81+
/// This function scans the docblocks associated with the current statement in the context,
8282
/// extracting all variable type declarations. It returns a vector of tuples, each containing:
8383
///
8484
/// - An optional variable name (if specified in the tag)
@@ -104,11 +104,7 @@ pub fn get_docblock_variables<'ctx>(
104104
artifacts: &mut AnalysisArtifacts,
105105
allow_tracing: bool,
106106
) -> Vec<(Option<mago_atom::Atom>, TUnion, Span)> {
107-
let Some(elements) = context.get_parsed_docblock().map(|document| document.elements) else {
108-
return vec![];
109-
};
110-
111-
elements
107+
context.get_parsed_docblocks()
112108
.into_iter()
113109
// Filter out non-tag elements
114110
.filter_map(|element| match element {

crates/analyzer/tests/cases/docblock_var_on_non_assignment.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,16 @@
1818
}
1919

2020
// Also test with multiple @var annotations
21-
/** @var array<string, object> $map */
21+
/** @var array<int|string, object> $map */
2222
$map = ['key' => new DateTime()];
2323

24+
function want_string(string $x): string {
25+
return $x;
26+
}
27+
2428
foreach ($map as $key => $value) {
2529
/** @var string $key */
2630
/** @var DateTime $value */
2731
$result = $value->format('Y-m-d');
32+
want_string($key);
2833
}

crates/analyzer/tests/cases/issue_801.php

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,9 @@ public function __construct()
2222
}
2323
}
2424

25-
/**
26-
* @var Config $config
27-
* @var Session $session
28-
*/
25+
/** @var Config $config */
26+
/** @var Session $session */
2927
if (!$session->isLoggedIn()) {
3028
}
3129

32-
echo $config->site_name;
30+
echo $config->site_name;

0 commit comments

Comments
 (0)