Allow fold when function declaration is in multiple lines Dart

[airherna]

Type: Bug

I'm filing this issue in the Dart extension because I have not been able to reproduce this exact behavior in languages like Python or C, even with the same parameter formatting. It's relevant because this type of formatting is provided by dartfmt and makes the fold feature useless.

Environment:
OS: Windows
Version: 1.73.1
Reproducibility rate: 100%

Steps to reproduce:

1. Create a Dart function such as:
   Future function(String param1,
   String param2, String param3) async {
   // content
   // content
   // content
   }
2. Try to fold the function

Expected Result:
The function folds and all the content and parameters are hidden.

Actual Result:
Only the function parameters are folded and the content stays visible.

Extension version: 3.52.1
VS Code version: Code 1.73.1 (6261075646f055b99068d3688932416f2346dd3b, 2022-11-09T04:27:29.066Z)
OS version: Windows_NT x64 10.0.22000
Modes:
Sandboxed: No

System Info

Item	Value
CPUs	11th Gen Intel(R) Core(TM) i7-11850H @ 2.50GHz (16 x 2496)
GPU Status	2d_canvas: enabled canvas_oop_rasterization: disabled_off direct_rendering_display_compositor: disabled_off_ok gpu_compositing: enabled multiple_raster_threads: enabled_on opengl: enabled_on rasterization: enabled raw_draw: disabled_off_ok skia_renderer: enabled_on video_decode: enabled video_encode: enabled vulkan: disabled_off webgl: enabled webgl2: enabled webgpu: disabled_off
Load (avg)	undefined
Memory (System)	31.67GB (16.43GB free)
Process Argv	--crash-reporter-id 923bf38f-184f-4b44-8523-aba43b319e5f
Screen Reader	no
VM	0%

A/B Experiments

vsliv368cf:30146710
vsreu685:30147344
python383cf:30185419
vspor879:30202332
vspor708:30202333
vspor363:30204092
vslsvsres303:30308271
pythonvspyl392:30443607
vserr242:30382549
pythontb:30283811
vsjup518:30340749
pythonptprofiler:30281270
vshan820:30294714
vstes263:30335439
vscoreces:30445986
pythondataviewer:30285071
vscod805:30301674
binariesv615:30325510
bridge0708:30335490
bridge0723:30353136
cmake_vspar411:30581797
vsaa593:30376534
pythonvs932:30410667
cppdebug:30492333
vsclangdc:30486549
c4g48928:30535728
dsvsc012cf:30540253
azure-dev_surveyone:30548225
pyindex848:30577860
nodejswelcome1cf:30587006
fc301958:30595537
3biah626:30602489
gswce1:30612156
3d0df643:30608728
dbltrim-noruby:30604474
89544117:30607850

[DanTup]

I agree it's inconvenient not being able to fold an entire function declaration. Although there are some questions about the best way to do this. We can't start multiple folding regions on the same line (because editors will generally only show a single folding icon), so we need to decide what to do with the current parameter list region.

Given the code:

/// Foo
@deprecated
void myMethod(String a,
    String b,
    String c,
) {
  // ...
}

There's currently a range for String a, String b, String c, and one for the body between { and }. Some choices that need to be made (note: I'm assuming the declarations range would start at the end of the name, so when collapsed you can still see the name in clients that support character-offset folding):

1. Do we remove the folding range for the parameters, or push the start of if to the next line (so it now covers only String b, String c)?
   If we keep it and push it to the next line, do we start at the beginning of the line or first non-whitespace character? (for VS Code this doesn't make any difference because it only supports line folding, but we still need to pick)
2. Do we remove the existing FUNCTION_BODY range (that goes from { to }) to replace with this, or keep it (I'm questioning how valuable it is to fold the body without the parameters?)

@bwilkerson any opinions? My feeling is that we drop/replace the existing BODY range (that just covers the block) and push the parameters range to the first non-whitespace character of the following line (although the way I found to get at the indentation for that is CorrectionUtils.getLinePrefix() which requires a resolved unit that folding doesn't have.. maybe we can use the Session to get the File, but I'm not sure any editors would even render a difference (or even which ones do more than just line folding)).

[bwilkerson]

No, I don't have any opinion here. I don't use code folding, so I have no experience to draw on, and I haven't really paid any attention to what our current support looks like.

I do have to wonder, though, what use case prompted us to support folding for parameter lists and why we chose to fold function bodies rather than the whole function (maybe because we fold parameter lists and couldn't have overlapping regions?). If we know why we support what we support we might at least know if changing it will break an important use case.

[DanTup]

I do have to wonder, though, what use case prompted us to support folding for parameter lists and why we chose to fold function bodies rather than the whole function (maybe because we fold parameter lists and couldn't have overlapping regions?)

I did some digging earlier to see if I could find if there was a reason for going this route, and could not find anything. I think it's likely it was somewhat accidental - in dart-lang/sdk@4604300 I used FunctionBody to fold functions/methods/constructors and none of my tests had multi-line parameter lists (and parameter folding was added much later in dart-lang/sdk@d3082bc).

I'm also not a heavy user of it so don't have too strong an opinion, but this issue did remind me that I had tried to fold methods before and it didn't feel quite right (but being just an extra click I probably didn't think too much about it).

[airherna]

Following this discussion I want to add that I've not being able to fold the function body at all. It's not that folding the body requires an extra click after folding the parameters, it's that the arrow to fold the body does not appear at all when the declaration spans multiple lines.

[DanTup]

@airherna that issue has been fixed (via #4121) and will ship in an upcoming SDK release. There was a change in VS Code that changed how it handled ranges where one started on the same line that another started (which is the case for the parameter list and the function body in your example). If used to allow them, but now it drops one of them.

So using the latest (bleeding edge) code your example above has a fold for the function body:

The reason the parameter folding is gone now, is that we "shrunk" it to not overlap with the function body, and it was left as only a single line so it was removed. In the case where it was still multiple lines, it would still get a folding region:

However, I still think it's a little clumsy to have to fold two regions and it's left looking like this:

[DanTup]

I have some changes that I think make this work as well as we can. We'll add "function body" folding ranges for the whole declaration instead of just from the { and we'll start parameter list folding from the start of the first parameter (instead of from the ().

The result is that whole functions can be folded, and if the parameters are more than two lines (which dartfmt seems to always then push the start onto the next line) then they'll be foldable within that:

Nov-17-2022.15-34-02.mp4

Since VS Code only supports line folding, you'll always see the entire line when folding (eg. int foo( and int foo2(String ...), but for clients that support character offsets for the ranges, the fold would start immediately after the name.

[bwilkerson]

That sounds good to me.

Another data point would be to see how other language servers handle this case (assuming that any do).

[DanTup]

TypeScript (which I generally use as a template because it's maintained by the VS Code team) does fold the whole declaration, but doesn't allow folding of parameter lists:

Nov-17-2022.16-34-20.mp4

I think there may be some value in folding parameter lists (I suspect on average Flutter code has longer parameter lists and/or more likely has them split over a larger number of lines) but I'm not against removing them if they feel redundant.

Review on the way shortly.

[DanTup]

With dart-lang/sdk@35d3015, whole function declarations (instead of just bodies) can be folded. There are also some other minor tweaks to ranges to improve handling of parameter lists when they start on the same line as the declaration (so they can still be folded).

This change is in the Dart language server so the fixes will show up with a future SDK update (rather than a Dart VS Code extension update).