Skip to content

Commit 2042850

Browse files
jensjohaCommit Queue
jensjoha
authored and
Commit Queue
committed
[analyzer] Ignore info can parse 'word' starting with underscore
Analyzer plugin warnings and lints can be ignored with "// Ignore: plugin_name/lint_name". Plugins are just packages and packages can start with an underscore, so the ignore syntax should support that too, which it did not. This CL changes that so it's supported. It furthermore changes the extraction method (`ignoredElements`) from sync* to be a regular sync method because it's so much easier to step through. It's also faster though: Below I've run this: ```dart Stopwatch stopwatch = Stopwatch()..start(); CommentToken token = CommentToken( TokenType.STRING, // One of these: // "", // "// Ignore: foobar, baz", 42, ); for (int i = 0; i < 10000000; i++) { // ignoredElements1 or ignoredElements2 -- sync* vs sync. var list = token.ignoredElements2.toList(); // One of these: // if (list.isNotEmpty) throw "Not empty."; // if (list.length != 2) throw "Not right size."; } print("Done in ${stopwatch.elapsed}"); ``` as aot-compiled, run each 10 times and put the output through `ministat`: Empty (''), sync* vs sync: ``` Difference at 95.0% confidence -0.629713 +/- 0.0755153 -69.6007% +/- 8.34653% (Student's t, pooled s = 0.08037) ``` 2 ignores ('// Ignore: foobar, baz'), sync* vs sync: ``` Difference at 95.0% confidence -1.17368 +/- 0.0544388 -46.8828% +/- 2.17457% (Student's t, pooled s = 0.0579385) ``` Change-Id: I0c1abdc8001125e32bb5bddd579a187476f2fe31 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/449700 Commit-Queue: Jens Johansen <[email protected]> Reviewed-by: Johnni Winther <[email protected]>
1 parent 659978f commit 2042850

File tree

4 files changed

+98
-47
lines changed

4 files changed

+98
-47
lines changed

‎pkg/analyzer/lib/src/ignore_comments/ignore_info.dart‎

Lines changed: 61 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -224,10 +224,8 @@ class IgnoreInfo {
224224
extension CommentTokenExtension on CommentToken {
225225
/// The elements ([IgnoredDiagnosticName]s and [IgnoredDiagnosticType]s) cited
226226
/// by this comment, if it is a correctly formatted ignore comment.
227-
// Use of `sync*` should not be non-performant; the vast majority of ignore
228-
// comments cite a single diagnostic name. Ignore comments that cite multiple
229-
// diagnostic names typically cite only a handful.
230-
Iterable<IgnoredElement> get ignoredElements sync* {
227+
List<IgnoredElement> get ignoredElements {
228+
List<IgnoredElement> result = [];
231229
var offset = lexeme.indexOf(':') + 1;
232230

233231
void skipPastWhitespace() {
@@ -240,8 +238,9 @@ extension CommentTokenExtension on CommentToken {
240238
}
241239

242240
void readWord() {
243-
if (!lexeme.codeUnitAt(offset).isLetter) {
244-
// Must start with a letter.
241+
if (!lexeme.codeUnitAt(offset).isLetter &&
242+
!lexeme.codeUnitAt(offset).isUnderscore) {
243+
// Must start with a letter or underscore.
245244
return;
246245
}
247246
offset++;
@@ -261,63 +260,71 @@ extension CommentTokenExtension on CommentToken {
261260
skipPastWhitespace();
262261
if (offset == lexeme.length) {
263262
// Reached the end without finding any ignored elements.
264-
return;
263+
return result;
265264
}
266265
var wordOffset = offset;
267266
// Parse each comma-separated diagnostic code, and diagnostic type.
268267
readWord();
269268
if (wordOffset == offset) {
270269
// There is a non-word (other characters) at `offset`.
271270
if (hasIgnoredElements) {
272-
yield IgnoredDiagnosticComment(
273-
lexeme.substring(offset),
274-
this.offset + wordOffset,
271+
result.add(
272+
IgnoredDiagnosticComment(
273+
lexeme.substring(offset),
274+
this.offset + wordOffset,
275+
),
275276
);
276277
}
277-
return;
278+
return result;
278279
}
279280
var word = lexeme.substring(wordOffset, offset);
280281
if (word.toLowerCase() == 'type') {
281282
// Parse diagnostic type.
282283
skipPastWhitespace();
283-
if (offset == lexeme.length) return;
284+
if (offset == lexeme.length) return result;
284285
var nextChar = lexeme.codeUnitAt(offset);
285-
if (!nextChar.isEqual) return;
286+
if (!nextChar.isEqual) return result;
286287
offset++;
287288
skipPastWhitespace();
288-
if (offset == lexeme.length) return;
289+
if (offset == lexeme.length) return result;
289290
var typeOffset = offset;
290291
readWord();
291292
if (typeOffset == offset) {
292293
// There is a non-word (other characters) at `offset`.
293294
if (hasIgnoredElements) {
294-
yield IgnoredDiagnosticComment(
295-
lexeme.substring(offset),
296-
this.offset + wordOffset,
295+
result.add(
296+
IgnoredDiagnosticComment(
297+
lexeme.substring(offset),
298+
this.offset + wordOffset,
299+
),
297300
);
298301
}
299-
return;
302+
return result;
300303
}
301304
if (offset < lexeme.length) {
302305
var nextChar = lexeme.codeUnitAt(offset);
303306
if (!nextChar.isSpace && !nextChar.isComma) {
304307
// There are non-identifier characters at the end of this word,
305308
// like `ignore: http://google.com`. This is not a diagnostic name.
306309
if (hasIgnoredElements) {
307-
yield IgnoredDiagnosticComment(
308-
lexeme.substring(wordOffset),
309-
this.offset + wordOffset,
310+
result.add(
311+
IgnoredDiagnosticComment(
312+
lexeme.substring(wordOffset),
313+
this.offset + wordOffset,
314+
),
310315
);
311316
}
312-
return;
317+
return result;
313318
}
314319
}
315320
var type = lexeme.substring(typeOffset, offset);
316321
hasIgnoredElements = true;
317-
yield IgnoredDiagnosticType(
318-
type,
319-
this.offset + wordOffset,
320-
offset - wordOffset,
322+
result.add(
323+
IgnoredDiagnosticType(
324+
type,
325+
this.offset + wordOffset,
326+
offset - wordOffset,
327+
),
321328
);
322329
} else {
323330
String? pluginName;
@@ -328,19 +335,21 @@ extension CommentTokenExtension on CommentToken {
328335
// 'plugin_one/foo'.
329336
pluginName = word;
330337
offset++;
331-
if (offset == lexeme.length) return;
338+
if (offset == lexeme.length) return result;
332339
var nameOffset = offset;
333340
readWord();
334341
word = lexeme.substring(nameOffset, offset);
335342
if (nameOffset == offset) {
336343
// There is a non-word (other characters) at `offset`.
337344
if (hasIgnoredElements) {
338-
yield IgnoredDiagnosticComment(
339-
lexeme.substring(offset),
340-
this.offset + nameOffset,
345+
result.add(
346+
IgnoredDiagnosticComment(
347+
lexeme.substring(offset),
348+
this.offset + nameOffset,
349+
),
341350
);
342351
}
343-
return;
352+
return result;
344353
}
345354
}
346355
}
@@ -350,40 +359,46 @@ extension CommentTokenExtension on CommentToken {
350359
// There are non-identifier characters at the end of this word,
351360
// like `ignore: http://google.com`. This is not a diagnostic name.
352361
if (hasIgnoredElements) {
353-
yield IgnoredDiagnosticComment(
354-
lexeme.substring(wordOffset),
355-
this.offset + wordOffset,
362+
result.add(
363+
IgnoredDiagnosticComment(
364+
lexeme.substring(wordOffset),
365+
this.offset + wordOffset,
366+
),
356367
);
357368
}
358-
return;
369+
return result;
359370
}
360371
}
361372
hasIgnoredElements = true;
362-
yield IgnoredDiagnosticName(
363-
word,
364-
this.offset + wordOffset,
365-
pluginName: pluginName,
373+
result.add(
374+
IgnoredDiagnosticName(
375+
word,
376+
this.offset + wordOffset,
377+
pluginName: pluginName,
378+
),
366379
);
367380
}
368381

369-
if (offset == lexeme.length) return;
382+
if (offset == lexeme.length) return result;
370383
skipPastWhitespace();
371-
if (offset == lexeme.length) return;
384+
if (offset == lexeme.length) return result;
372385

373386
var nextChar = lexeme.codeUnitAt(offset);
374387
if (!nextChar.isComma) {
375388
// We've reached the end of the comma-separated codes and types. What
376389
// follows is unstructured comment text.
377390
if (hasIgnoredElements) {
378-
yield IgnoredDiagnosticComment(
379-
lexeme.substring(offset),
380-
this.offset + wordOffset,
391+
result.add(
392+
IgnoredDiagnosticComment(
393+
lexeme.substring(offset),
394+
this.offset + wordOffset,
395+
),
381396
);
382397
}
383-
return;
398+
return result;
384399
}
385400
offset++;
386-
if (offset == lexeme.length) return;
401+
if (offset == lexeme.length) return result;
387402
}
388403
}
389404
}

‎pkg/analyzer/lib/src/utilities/extensions/string.dart‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ extension IntExtension on int {
1818

1919
bool get isLetterOrDigit => isLetter || isDigit;
2020

21-
bool get isLetterOrDigitOrUnderscore => isLetter || isDigit || this == 0x5F;
21+
bool get isLetterOrDigitOrUnderscore => isLetter || isDigit || isUnderscore;
2222

2323
/// Whether this, as an ASCII character, is a newline (not a carriage
2424
/// return) character.
@@ -29,6 +29,8 @@ extension IntExtension on int {
2929
/// Whether this, as an ASCII character, is a space or tab character.
3030
bool get isSpace => this == 0x20 || this == 0x09;
3131

32+
bool get isUnderscore => this == 0x5F;
33+
3234
/// Whether this, as an ASCII character, is a space (as per [isSpace]) or EOL
3335
/// character (as per [isEOL]).
3436
bool get isWhitespace => isSpace || isEOL;

‎pkg/analyzer/test/src/ignore_comments/ignore_info_test.dart‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,19 @@ class IgnoreInfoTest extends PubPackageResolutionTest {
9797
);
9898
}
9999

100+
test_pluginName_starts_with_underscore() async {
101+
var ignoredElements = await _parseIgnoredElements(
102+
'// ignore: _plugin_one/foo',
103+
);
104+
expect(ignoredElements, hasLength(1));
105+
_expectIgnoredName(
106+
ignoredElements[0],
107+
name: 'foo',
108+
offset: 11,
109+
pluginName: '_plugin_one',
110+
);
111+
}
112+
100113
test_trailingComma() async {
101114
var ignoredElements = await _parseIgnoredElements('// ignore: foo,');
102115
expect(ignoredElements, hasLength(1));

‎pkg/analyzer/test/src/utilities/extensions/string_test.dart‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,21 @@ class StringExtensionTest {
170170
expect('.'.codeUnitAt(0).isLetterOrDigit, isFalse);
171171
}
172172

173+
void test_isLetterOrDigitOrUnderscore() {
174+
for (var c in 'abcdefghijklmnopqrstuvwxyz'.codeUnits) {
175+
expect(c.isLetterOrDigitOrUnderscore, isTrue);
176+
}
177+
for (var c in 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'.codeUnits) {
178+
expect(c.isLetterOrDigitOrUnderscore, isTrue);
179+
}
180+
for (var c in '0123456789'.codeUnits) {
181+
expect(c.isLetterOrDigitOrUnderscore, isTrue);
182+
}
183+
expect('_'.codeUnitAt(0).isLetterOrDigitOrUnderscore, isTrue);
184+
expect(' '.codeUnitAt(0).isLetterOrDigitOrUnderscore, isFalse);
185+
expect('.'.codeUnitAt(0).isLetterOrDigitOrUnderscore, isFalse);
186+
}
187+
173188
void test_isSpace() {
174189
expect(' '.codeUnitAt(0).isSpace, isTrue);
175190
expect('\t'.codeUnitAt(0).isSpace, isTrue);
@@ -179,6 +194,12 @@ class StringExtensionTest {
179194
expect('A'.codeUnitAt(0).isSpace, isFalse);
180195
}
181196

197+
void test_isUnderscore() {
198+
expect('_'.codeUnitAt(0).isUnderscore, isTrue);
199+
expect(' '.codeUnitAt(0).isUnderscore, isFalse);
200+
expect('A'.codeUnitAt(0).isUnderscore, isFalse);
201+
}
202+
182203
void test_isWhitespace() {
183204
expect(' '.codeUnitAt(0).isWhitespace, isTrue);
184205
expect('\t'.codeUnitAt(0).isWhitespace, isTrue);

0 commit comments

Comments
 (0)