Skip to content

Commit 844bcee

Browse files
dantleechdantleech
committed
Refactor diagnostics provider
Only process one document at a time and move grace period to BEFORE we start linting.
1 parent 18c6336 commit 844bcee

File tree

3 files changed

+67
-81
lines changed

3 files changed

+67
-81
lines changed

‎lib/Core/Diagnostics/DiagnosticsEngine.php

+43-44
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,11 @@
1919
class DiagnosticsEngine
2020
{
2121
/**
22-
* @var Deferred<TextDocumentItem>
22+
* @var Deferred<null>
2323
*/
2424
private Deferred $deferred;
2525

26-
private bool $running = false;
27-
28-
private ?TextDocumentItem $next = null;
26+
private ?TextDocumentItem $waiting = null;
2927

3028
/**
3129
* @var array<int|string,list<Diagnostic>>
@@ -42,6 +40,8 @@ class DiagnosticsEngine
4240
*/
4341
private array $locks = [];
4442

43+
private float $lastUpdatedAt;
44+
4545
/**
4646
* @param DiagnosticsProvider[] $providers
4747
*/
@@ -51,6 +51,7 @@ public function __construct(private ClientApi $clientApi, private LoggerInterfac
5151
$this->providers = $providers;
5252
$this->clientApi = $clientApi;
5353
$this->sleepTime = $sleepTime;
54+
$this->lastUpdatedAt = 0.0;
5455
}
5556

5657
public function clear(TextDocumentItem $textDocument): void
@@ -67,8 +68,6 @@ public function clear(TextDocumentItem $textDocument): void
6768
*/
6869
public function run(CancellationToken $token): Promise
6970
{
70-
71-
7271
return \Amp\call(function () use ($token) {
7372
while (true) {
7473
try {
@@ -77,30 +76,29 @@ public function run(CancellationToken $token): Promise
7776
return;
7877
}
7978

80-
$textDocument = yield $this->nextDocument();
81-
$lastKnownVersion = ($this->versions[$textDocument->uri] ?? -1);
79+
yield $this->awaitNextDocument();
8280

83-
if ($lastKnownVersion <= $textDocument->version && isset($this->diagnostics[$textDocument->uri])) {
84-
// reset diagnostics for this document
85-
$this->clientApi->diagnostics()->publishDiagnostics(
86-
$textDocument->uri,
87-
$textDocument->version,
88-
[],
89-
);
90-
}
81+
$gracePeriod = abs($this->sleepTime - ((microtime(true) - $this->lastUpdatedAt) * 1000));
82+
yield delay(intval($gracePeriod));
9183

92-
$this->diagnostics[$textDocument->uri] = [];
84+
$textDocument = $this->waiting;
85+
$this->waiting = null;
86+
// allow the next document update to resolve
9387
$this->deferred = new Deferred();
94-
// after we have reset deferred, we can safely set linting to
95-
// `false` and let another resolve happen
96-
$this->running = false;
9788

98-
// if the last processed version of the document is more recent
99-
// than the last then continue.
100-
if ($lastKnownVersion >= $textDocument->version) {
89+
// should never happen
90+
if ($textDocument === null) {
10191
continue;
10292
}
10393

94+
// reset diagnostics for this document
95+
$this->clientApi->diagnostics()->publishDiagnostics(
96+
$textDocument->uri,
97+
$textDocument->version,
98+
[],
99+
);
100+
$this->diagnostics[$textDocument->uri] = [];
101+
104102
$this->versions[$textDocument->uri] = $textDocument->version;
105103

106104
$crashedProviders = [];
@@ -113,7 +111,7 @@ public function run(CancellationToken $token): Promise
113111
asyncCall(function () use ($providerId, $provider, $token, $textDocument, &$crashedProviders) {
114112
$start = microtime(true);
115113

116-
yield $this->await($providerId);
114+
yield $this->awaitProviderLock($providerId);
117115

118116
if (!$this->isDocumentCurrent($textDocument)) {
119117
return;
@@ -151,16 +149,11 @@ public function run(CancellationToken $token): Promise
151149
$diagnostics
152150
);
153151

154-
$timeToSleep = $this->sleepTime - $elapsed;
155-
156-
if ($timeToSleep > 0) {
157-
yield delay($timeToSleep);
158-
}
159-
160152
if (!$this->isDocumentCurrent($textDocument)) {
161153
return;
162154
}
163155

156+
164157
$this->clientApi->diagnostics()->publishDiagnostics(
165158
$textDocument->uri,
166159
$textDocument->version,
@@ -174,27 +167,35 @@ public function run(CancellationToken $token): Promise
174167

175168
public function enqueue(TextDocumentItem $textDocument): void
176169
{
177-
// if we are already linting then store whatever comes afterwards in
178-
// next, overwriting the redundant update
179-
if ($this->running === true) {
180-
$this->next = $textDocument;
170+
// set the last updated at timestamp - this will be used as the basis of
171+
// the grace period before linting
172+
$this->lastUpdatedAt = microtime(true);
173+
174+
$waiting = $this->waiting;
175+
176+
// set the next document
177+
$this->waiting = $textDocument;
178+
179+
// if we already had a waiting document then do nothing
180+
// it will get resolved next time
181+
if ($waiting !== null) {
181182
return;
182183
}
183184

184-
// resolving the promise will start the diagnostc resolving
185-
$this->running = true;
186-
$this->deferred->resolve($textDocument);
185+
// otherwise trigger the lint process
186+
$this->deferred->resolve();
187187
}
188188

189189
private function isDocumentCurrent(TextDocumentItem $textDocument): bool
190190
{
191191
return $textDocument->version === ($this->versions[$textDocument->uri] ?? -1);
192192
}
193+
193194
/**
194195
* @param string|int $providerId
195196
* @return Promise<bool>
196197
*/
197-
private function await($providerId): Promise
198+
private function awaitProviderLock($providerId): Promise
198199
{
199200
if (!array_key_exists($providerId, $this->locks)) {
200201
return new Success(true);
@@ -205,14 +206,12 @@ private function await($providerId): Promise
205206
}
206207

207208
/**
208-
* @return Promise<TextDocumentItem>
209+
* @return Promise<null>
209210
*/
210-
private function nextDocument(): Promise
211+
private function awaitNextDocument(): Promise
211212
{
212-
if ($this->next) {
213-
$textDocument = $this->next;
214-
$this->next = null;
215-
return new Success($textDocument);
213+
if ($this->waiting) {
214+
return new Success();
216215
}
217216

218217
return $this->deferred->promise();

‎tests/Unit/Core/Diagnostics/DiagnosticsEngineTest.php

+22-35
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use Phpactor\LanguageServerProtocol\TextDocumentItem;
1111
use Phpactor\LanguageServer\Core\Diagnostics\ClosureDiagnosticsProvider;
1212
use Phpactor\LanguageServer\Core\Diagnostics\DiagnosticsEngine;
13+
use Phpactor\LanguageServer\Core\Rpc\NotificationMessage;
1314
use Phpactor\LanguageServer\LanguageServerTesterBuilder;
1415
use Phpactor\LanguageServer\Test\ProtocolFactory;
1516
use Psr\Log\NullLogger;
@@ -39,16 +40,22 @@ public function testPublishesDiagnostics(): Generator
3940

4041
$token->cancel();
4142

42-
$notification = $tester->transmitter()->shiftnotification();
43+
$notification = $tester->transmitter()->shiftNotification();
44+
self::assertNotNull($notification, 'Notification sent');
45+
self::assertEquals('textDocument/publishDiagnostics', $notification->method);
46+
self::assertEquals([], $notification->params['diagnostics'] ?? null);
4347

48+
$notification = $tester->transmitter()->shiftNotification();
4449
self::assertNotNull($notification, 'Notification sent');
4550
self::assertEquals('textDocument/publishDiagnostics', $notification->method);
51+
/** @phpstan-ignore-next-line */
52+
self::assertEquals('Foobar is broken', $notification->params['diagnostics'][0]->message ?? null);
4653
}
4754

4855
/**
4956
* @return Generator<mixed>
5057
*/
51-
public function testPublishesForManyFiles(): Generator
58+
public function testOnlyPublishesForMostRecentFile(): Generator
5259
{
5360
$tester = LanguageServerTesterBuilder::create();
5461
$engine = $this->createEngine($tester, 0, 0);
@@ -64,16 +71,17 @@ public function testPublishesForManyFiles(): Generator
6471

6572
$token->cancel();
6673

67-
self::assertEquals(3, $tester->transmitter()->count());
74+
// includes reset diagnostic
75+
self::assertEquals(2, $tester->transmitter()->count());
6876
}
6977

7078
/**
7179
* @return Generator<mixed>
7280
*/
73-
public function testDeduplicatesSuccessiveChangesToSameFile(): Generator
81+
public function testDoesNotProcessMoreThanOneDocument(): Generator
7482
{
7583
$tester = LanguageServerTesterBuilder::create();
76-
$engine = $this->createEngine($tester, 10, 0);
84+
$engine = $this->createEngine($tester, 1, 0);
7785

7886
$token = new CancellationTokenSource();
7987
$promise = $engine->run($token->getToken());
@@ -82,34 +90,11 @@ public function testDeduplicatesSuccessiveChangesToSameFile(): Generator
8290
$engine->enqueue(ProtocolFactory::textDocumentItem('file:///foobar', 'foobar'));
8391
$engine->enqueue(ProtocolFactory::textDocumentItem('file:///foobar', 'foobar'));
8492

85-
yield new Delayed(1);
86-
87-
$token->cancel();
88-
89-
// clear three times
90-
self::assertEquals(2, $tester->transmitter()->count());
91-
}
92-
93-
/**
94-
* @return Generator<mixed>
95-
*/
96-
public function testIgnoresTextDocumentsWithInferiorVersions(): Generator
97-
{
98-
$tester = LanguageServerTesterBuilder::create();
99-
$engine = $this->createEngine($tester, 10, 0);
100-
101-
$token = new CancellationTokenSource();
102-
$promise = $engine->run($token->getToken());
103-
104-
$engine->enqueue(ProtocolFactory::textDocumentItem('file:///foobar', 'foobar', version: 20));
105-
$engine->enqueue(ProtocolFactory::textDocumentItem('file:///foobar', 'foobar', version: 1));
106-
$engine->enqueue(ProtocolFactory::textDocumentItem('file:///foobar', 'foobar', version: 20));
107-
$engine->enqueue(ProtocolFactory::textDocumentItem('file:///foobar', 'foobar', version: 30));
108-
109-
yield new Delayed(1);
93+
yield new Delayed(10);
11094

11195
$token->cancel();
11296

97+
// clear + publish
11398
self::assertEquals(2, $tester->transmitter()->count());
11499
}
115100

@@ -132,7 +117,7 @@ public function testSleepPreventsSeige(): Generator
132117

133118
$token->cancel();
134119

135-
self::assertEquals(3, $tester->transmitter()->count());
120+
self::assertEquals(2, $tester->transmitter()->count());
136121
}
137122

138123
public function testAggregatesResultsFromMultipleProviders(): Generator
@@ -158,7 +143,7 @@ public function testAggregatesResultsFromMultipleProviders(): Generator
158143

159144
yield new Delayed(100);
160145

161-
self::assertEquals(2, $tester->transmitter()->count());
146+
self::assertEquals(3, $tester->transmitter()->count());
162147

163148
}
164149

@@ -178,10 +163,12 @@ public function testHandlesLinterExceptions(): Generator
178163

179164
yield new Delayed(100);
180165

181-
self::assertEquals(1, $tester->transmitter()->count());
182-
$notification = $tester->transmitter()->shiftnotification();
166+
self::assertEquals(2, $tester->transmitter()->count());
167+
$notification = $tester->transmitter()->shiftNotification();
168+
$notification = $tester->transmitter()->shiftNotification();
169+
assert($notification instanceof NotificationMessage);
183170
self::assertEquals('window/showMessage', $notification->method);
184-
self::assertStringContainsString('oh dear', $notification->params['message']);
171+
self::assertStringContainsString('oh dear', ((string)($notification->params['message'] ?? '')));
185172

186173
}
187174

‎tests/Unit/Diagnostics/CodeActionDiagnosticsProviderTest.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,12 @@ public function testProvidesDiagnostics(): void
3232

3333
wait(new Delayed(10));
3434

35-
self::assertEquals(1, $tester->transmitter()->count());
35+
self::assertEquals(2, $tester->transmitter()->count());
3636

3737
$tester->textDocument()->update('file:///foobar', 'bar');
3838
wait(new Delayed(10));
3939

40-
self::assertEquals(4, $tester->transmitter()->count());
40+
self::assertEquals(5, $tester->transmitter()->count());
4141
self::assertEquals('textDocument/publishDiagnostics', $tester->transmitter()->shiftNotification()->method);
4242
}
4343
}

0 commit comments

Comments
 (0)