Analyzer: adding dartdoc causes spurious message for unused local variable

[skybrian]

(This is not reproducible but I thought I't report it anyway, mostly as an example of a bug where it's difficult to gather enough information to reproduce it.)

Using Atom 1.2.0, dartlang 0.4.15, dart sdk 1.13.0.

Sometimes converting a '//' to a '///' above a getter causes spurious messages for all local
variables inside the the getter:
Info: 'the value of the local variable 'XXX' is not used'
Deleting the final slash (so it's no longer a doc comment)
will remove the messages, as will reanalyzing sources.

I was able to reproduce the error locally in a different project, using the code below [1]. To trigger the error, type "/" for the comment on 'title'.

However, I was unable to reproduce this after copying the same Dart code to a new file (also in the second project), so I suspect you won't be able to reproduce it either. I edited the code to remove unnecessary cruft, and I don't remember which edits I made. It seems like we need a way of dumping more information about the current state of a Dart file?

I also tried looking at Analyzer diagnostics, but ran into (dart-atom/dart#588 (comment)) so I don't have any more information.

[1]

    class Card {
      final int id; // may be null if not in a Deck
      final String note;
      final String text;

      Card._raw(this.id, this.note, this.text);

      // Returns the first line of the note field.
      String get title {
        int newline = note.indexOf("\n");
        if (newline == -1) return note;
        return note.substring(0, newline);
      }
    }

[bwilkerson]

Actually, I was able to reproduce it by pasting the text above into my standard "test.dart" test file.

[skybrian]

I can reproduce it as well. I'll look into it.

[skybrian]

Another symptom: the outline disappears.

Using the diagnostics page, it looks like the AST structure disappears:

CompilationUnit [0..336]
┊ element = [redacted]/example.dart [search index]
┊ Comment [151..195]

After changing "///" to "//" it appears again.

[skybrian]

A log entry [1] indicates that the bug happens when _tryPoorMansIncrementalResolution in context.dart succeeds. I haven't been able to make that happen outside Atom yet.

[1] 1452586433119:Perf:analysis_incremental:11:success=true,context_id=0,code_length=337

[bwilkerson]

@scheglov Any ideas?

[skybrian]

There is a _resolveCommentDoc method specifically for resolving comment docs. I don't see the bug, but perhaps it's failing somehow.

[skybrian]

I figured out why my integration test didn't reproduce the same problem as I saw in Atom. It turns out that the behavior is different in checked mode. The analysis server swallows the exception and keeps going using non-incremental resolution, so I wasn't able to reproduce the bug.

Here is the stack trace:

stderr: [2016-01-12 17:00:11.626932] type 'ClassDeclaration' is not a subtype of type 'Comment' of 'oldComment'.
stderr: [2016-01-12 17:00:11.627174] #0 PoorMansIncrementalResolver._resolveCommentDoc (package:analyzer/src/generated/incremental_resolver.dart:1598:26)
stderr: #1 PoorMansIncrementalResolver.resolve (package:analyzer/src/generated/incremental_resolver.dart:1434:28)
stderr: #2 AnalysisContextImpl._tryPoorMansIncrementalResolution. (package:analyzer/src/context/context.dart:1837:31)
stderr: #3 _PerformanceTagImpl.makeCurrentWhile (package:analyzer/src/generated/utilities_general.dart:194:15)
stderr: #4 AnalysisContextImpl._tryPoorMansIncrementalResolution (package:analyzer/src/context/context.dart:1787:54)
stderr: #5 AnalysisContextImpl.handleContentsChanged (package:analyzer/src/context/context.dart:963:14)
stderr: #6 AnalysisServer.updateContent.. (package:analysis_server/src/analysis_server.dart:1235:23)
stderr: #7 List.forEach (dart:core-patch/growable_array.dart:233)
stderr: #8 AnalysisServer.updateContent. (package:analysis_server/src/analysis_server.dart:1233:17)
stderr: #9 _HashVMBase&MapMixin&&_LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:340)
stderr: #10 AnalysisServer.updateContent (package:analysis_server/src/analysis_server.dart:1186:13)
stderr: #11 AnalysisDomainHandler.updateContent (package:analysis_server/src/domain_analysis.dart:305:12)
stderr: #12 AnalysisDomainHandler.handleRequest (package:analysis_server/src/domain_analysis.dart:211:16)
stderr: #13 AnalysisServer.handleRequest.. (package:analysis_server/src/analysis_server.dart:686:45)
stderr: #14 _PerformanceTagImpl.makeCurrentWhile (package:analyzer/src/generated/utilities_general.dart:194:15)
stderr: #15 AnalysisServer.handleRequest. (package:analysis_server/src/analysis_server.dart:682:50)

We should probably audit the analyzer for places where we catch all exceptions and make sure that checked mode failures are handled properly. For some reason this exception wasn't logged until I turned on the special incremental resolution logger.

In this particular case, the culprit is the searchWithin() method in utilities.dart. the catch clause in incremental_resolver.dart line 1557.

[skybrian]

Looking at _findFirstDifferentToken, it appears that a _TokenPair will have type COMMENT_DOC if either token is preceded by a doc comment, and the doc comments are different. (The tokens themselves could be different as well, because the comments are compared first.)

In PoorMansIncrementalResolver.resolve(), it will call _resolveCommentDoc for any pair of this type.

_resolveCommentDoc appears to be written assuming that _findNodeCovering will return a Comment for both token positions but this doesn't seem to be the case.

[bwilkerson]

@scheglov Any advice?

[skybrian]

It looks like doc comments are represented in the AST, but regular comments are not. Therefore, when _resolveCommentDoc calls _findNodeCovering() with the offset of a comment that is not a doc comment, the closest match is the parent node, which in this case is a ClassDeclaration, not a Comment.

In checked mode, this throws an exception. In non-checked mode, it calls NodeReplacer.replace() to remove the ClassDeclaration and replace it with the DocComment.

So this code needs to return false when it doesn't receive a pair of doc comments.

[scheglov]

https://codereview.chromium.org/1581293002

[skybrian]

I was just about to send a patch for review, but you beat me to it.

[scheglov]

9484bfe