No notifications for FOLDING subscription in analysis server

[DanTup]

I've added FOLDING to my subscriptions, but I don't seem to get any notifications with the folding data:

[10:23:43]: Spawning /Users/dantup/Dev/dart-sdk/dart-2.0.0-dev.40.0/bin/dart with args ["/Users/dantup/Dev/dart-sdk/dart-2.0.0-dev.40.0/bin/snapshots/analysis_server.dart.snapshot","--client-id=Dart-Code.dart-code","--client-version=2.13.0-dev"]
[10:23:44]: <== {"event":"server.connected","params":{"version":"1.20.0","pid":9717,"sessionId":""}}
[10:23:44]: ==> {"id":"12","method":"analysis.setAnalysisRoots","params":{"excluded":[],"included":["/Users/dantup/Desktop/Dart Sample"]}}
[10:23:44]: ==> {"id":"13","method":"analysis.updateContent","params":{"files":{"/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart":{"edits":[{"offset":0,"length":0,"replacement":"","id":""}],"type":"change"}}}}
[10:23:44]: ==> {"id":"14","method":"analysis.setPriorityFiles","params":{"files":["/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart"]}}
[10:23:44]: ==>
  {
    "id": "15",
    "method": "analysis.setSubscriptions",
    "params": {
      "subscriptions": {
        "CLOSING_LABELS": [
          "/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart"
        ],
        "FOLDING": [
          "/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart"
        ],
        "OCCURRENCES": [
          "/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart"
        ],
        "OUTLINE": [
          "/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart"
        ]
      }
    }
  }
[10:23:44]: <== {"id":"12"}
[10:23:44]: <== {"id":"13","result":{}}
[10:23:44]: <== {"id":"14"}
[10:23:44]: <== {"id":"15"}
[10:23:44]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
[10:23:44]: <== {"event":"analysis.errors","params":{"file":"/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart","errors":[{"severity":"INFO","type":"HINT","location":{"file":"/Users/dantup/Desktop/Dart Sample/bin/folding_test.dart","offset":21,"length":6,"startLine":2,"startColumn":7},"message":"The value of the local variable 'unused' isn't used.","correction":"Try removing the variable, or using it.","code":"unused_local_variable","hasFix":false}]}}
[10:23:44]: <== {"event":"analysis.occurrences", // ...
[10:23:44]: <== {"event":"analysis.closingLabels" // ...
[10:23:44]: <== {"event":"analysis.outline", // ...
[10:23:45]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}

[DanTup]

Removing/re-adding the file to priority files doesn't help, nor does making edits the file.

[DanTup]

(cc @bwilkerson @scheglov)

[scheglov]

There are no FoldingContributor implementations for Dart in analysis server.

[DanTup]

Well that explains that! =)

Am I right in thinking this would be very similar to the Closing Labels implementation I did? If so I might have a stab at it. People have complained about VS Code's poor folding many times, and also asked for the ability to collapse directives. I'd been telling people once VS Code supports custom folding (which it does in vNext) it'll be easy. Whoops!

[scheglov]

Yes, I think it should be very similar.

[DanTup]

I've made a start on this:

https://dart-review.googlesource.com/c/sdk/+/54232

[DanTup]

A more complete implementation: https://dart-review.googlesource.com/c/sdk/+/54402

[DanTup]

Some outstanding things:

• Tweaks to FoldingKind (eg. we don't currently have a good category for nested functions); pending comments on https://dart-review.googlesource.com/c/sdk/+/54402
• Update version number for AS (especially if we're changing FoldingKind)
• Annotations (look for Angular example)

@bwilkerson @scheglov gonna land that other review so probably easiest to discuss FoldingKind here. I think we have options of either adding a LOCAL_DECLARATION or similar, or changing the existing kinds (probably not breaking if nobody can be using this yet) to be more like this list (that list seems to make more sense to me that categories top-level and class-level things differently).

[DanTup]

Annotations are done at https://dart-review.googlesource.com/c/sdk/+/54580

I need some opinions on FoldingKinds to tidy them up though. My suggestion would maybe be to change the TOP_LEVEL_DECLARATION to CLASS and METHOD (though I'm not sure about METHOD vs FUNCTION?). Since things can be nested, I think grouping based on being top-level or nested might be a bit weird.

[bwilkerson]

If it's an opinion you want ... :-)

The first question is: what do clients want to use FoldingKind for?

Based on the image, I'm guessing that at least one use case is to enable or disable the use of certain kinds of regions. If that's the only use case, then we only need to consider what users are likely to want to fold. Again, based on the list from IntelliJ, it seems reasonable to assume that users will want to fold

• file header comments
• documentation comments
• directives
• class bodies
• function bodies (block or expression, whether at the top-level, in classes, or local to other functions)
• annotations (presumably sequences of)

I don't know what "Dart parts" refers to, but I'm guessing it means part directives, which I would include in "directives" (that is, I would fold all directives unless we get feedback requesting that we fold different kinds of directives separately). I'm also not sure what "Dart generic parameters" does.

Another case suggested by the IntelliJ list is non-documentation comments (block comments and sequences of contiguous end of line comments). I don't know how useful that would be, but it's something to consider.

Is there any reason to subdivide any of those groups?

Is there any other use case for the kind?

If not, I'd be fine having the list of kinds updated to match the above.

[DanTup]

The list you gave seems good to me. We do currently have a kind for (non-doc) Comments, do you think we should remove it?

I presume we're putting class methods in with functions?

annotations (presumably sequences of)

Do you mean for us to collapse them all into one region? Currently I'm just collapsing the bit inside the parens:

@something(/*...*/)
@something(/*...*/)
main() {}

How should we decide what's a "file header comment"? Should it be any comment that starts at offset 0 and has a blank line after it, so we don't capture comments on a method like this?

// this is
// my main method
main() {}

?

[bwilkerson]

We do currently have a kind for (non-doc) Comments, do you think we should remove it?

Unless we're going to support folding of non-documentation comments, yes. I'd remove anything we're not currently using.

I presume we're putting class methods in with functions?

I would. I can't think why users would want to differentiate between different kinds of functions, but if you can think of a reason then we can split them out.

Do you mean for us to collapse them all into one region?

I can think of two possible reasons for wanting to fold annotations. The first is long annotations, which covers the Angular use case. The second is a code base with a prolific use of annotations. Consider something like

@experimental
@mustCallSuper
@nonNullable
@protected
@visibleForOverriding
@visibleForTesting
String getSomeData(...) ...

I could imagine users wanting to fold a list like that.

How should we decide what's a "file header comment"? Should it be any comment that starts at offset 0 and has a blank line after it, so we don't capture comments on a method like this?

The only thing I would allow before the comment is a script tag ("#! /usr/bin/dart").

I suspect that nearly everyone uses a sequence of end of line comments rather than a block comment, so we could limit it in that way.

I'm not sure whether we want to prevent blank lines before the file header, but I suspect that it's always at offset 0 unless there's a script tag.

[DanTup]

We do currently have code for non-doc comments, but I don't know if it triggers (if each line of the comments if considered its own comment, probably not). I'll remove for now and we can revisit if required.

I think functions+methods together makes sense, I was just confirming in case some think it weird to put a method inside a category called functions.

Collapsing multiple annotations makes sense too, so I'll add that; plus the file header stuff (probably be next week now though).

[bwilkerson]

... if each line of the comments if considered its own comment ...

Every end of line comment is a separate token, and there are only AST nodes created for documentation comments.

I think functions+methods together makes sense, I was just confirming in case some think it weird to put a method inside a category called functions.

Yeah, I tend to think in terms of the language grammar, in which case methods, constructors, top-level and local functions all have a function body. (And I'm assuming we're folding the body and not the whole method / function declaration.)

[DanTup]

methods, constructors, top-level and local functions all have a function body

Aha, then it fits perfectly :-)

I'm assuming we're folding the body and not the whole method / function declaration

Yep! In the first cut I incorrectly included more than I should. The later pushes only included the bit you'd want to disappear when you collapse (eg. inside of the braces for a block).

[DanTup]

Any opinions on the range we should return for collapsing multiple annontations? Normally we collapse the bit between braces/parents, for example:

@myAnnotation(/*START*/arg1, arg2, arg3/*END*/)

This means if the editor was to replace the region with ... it'd look good (sadly that's not how Code works, but I think it's logical to return the data that way).

I'm not sure where we should start/end the region for multiple lines of annotations, like:

@myAnnotation1
@myAnnotation2(arg1, arg2, arg3)

I'm tempted to say we start after the name of the first annotation (eg. after the 1) and end at the end of the last annotation (after the closing )). WDYT?

[DanTup]

Also; do we want to replace the existing annotation folds (eg. just the args of a single annotation if it spans multiple lines)? If so, we may end up with nested regions here, and should they be a single FoldingKind or two (Annotations vs Annotation)?

[bwilkerson]

Any opinions on the range we should return for collapsing multiple annontations?

What do we do for directives? This seems fairly analogous.

... do we want to replace the existing annotation folds ...

I'm not sure. That might depend on how we answer the question above.

I don't think it would be good if we have both kinds and the visual presentation is the same under some circumstances. My initial guess is that we only want one kind of fold, so folding multiple annotations would replace the existing folds.

If we do keep both, then I think they should have different kinds. (And nested regions isn't a problem. We already do that with classes and methods within classes.)

[DanTup]

What do we do for directives? This seems fairly analogous.

Currently we collapse after the first import keyword, so for:

import "x";

import "x";

export "x";

You'd get:

import[...]

My initial guess is that we only want one kind of fold, so folding multiple annotations would replace the existing folds

I think this feels most sensible to me. Although we nest inside methods it seems like that's more useful (you might want to collapse a nested method while looking at the containing method) it doesn't seem so useful for annotations (collapse one while looking at others).

So for now, I'll replace the old behaviour with a single fold around the edges unless anybody objects.

The FoldingKinds changes are at https://dart-review.googlesource.com/c/sdk/+/54920, I'll make the remaining changes (grouping annotations and implementing the file header) until that ones landed in case there are any changes.

[bwilkerson]

Currently we collapse after the first import keyword ...

Not as helpful as I'd hoped :-). If we followed that pattern we'd get @..., but I think that would be hard to read. Probably after the name of the first annotation, as you suggested, is best.

[DanTup]

Yeah, it's unfortunate that for imports showing the first keyword is less specific than annotations. We could always tweak the directives code to come before the first semi-colon if you want it to feel a bit more consistent; that way it'd look like:

import "first_file.dart"[...]

I'm not sure if it's any more correct, but maybe feels a little more consistent. LMK if you'd like me to change.

[bwilkerson]

I don't think we want to change the directives behavior. There's a much bigger chance of a URI being long enough to require a line break between the import and the URI than there is for there being a line break between the @ and the annotation name. And I think we want a fold to start on the same line as the construct being folded.

[DanTup]

@bwilkerson

Non-documentation comments are available only in the token stream. You'll need to find the first directive or declaration (so that you skip the script tag if there is one), ask it for the beginToken, and ask that for precedingComments.

Got it; however I'm having trouble finding when there's an empty line for the end of the header. For example, given this file:

// Copyright blah...
// See Licence

// Main method comment
main() {}

I can't find anything obvious in the tokens that identifiers there was a blank line there so I can end the file header. I guess I need to know the number of newlines between the .end of the current token and the .offset of the next (the character count is easy, but knowing whether it's \n\n or \r\n doesn't seem to be; as far as I can tell the newline characters aren't in here?)

Edit: See https://dart-review.googlesource.com/c/sdk/+/55284

[bwilkerson]

Correct. For that you need the LineInfo for the file. Then you can ask it for the line numbers of the offsets of the comment tokens and see whether the difference is greater than 1.

[DanTup]

Oh of course, I'm already doing that to decide if to show ranges. I thought Tokens were a different type, but I see it works fine! Fix incoming, thanks! 👍

[DanTup]

This is done :-)