Improvements to analysis server closing labels

[DanTup]

@bwilkerson Here's some tweaks to the closing labels as a result of testing on a larger codebase, inc:

1. Fix misleading comment after moving the _compareLabels method to unit tests (it takes arguments now instead of using class variables)
2. Track the ending line of closing labels and include labels that would normally be filtered (only spanning 1-2 lines) if they end on the same line as another label (> 2 lines)
3. Revert line checking to use argument lists node instead of invocation node to avoid showing labels on expressions that span multiple lines when the arguments are only on 1-2 (and apply same to constructors)
4. Don't show closing labels for code inside interpolated strings

Depending on feedback, we might want to tweak some of these (eg. I think 2 is a good improvement, but haven't had much feedback yet) but I think these generally improve what's there so far.

I don't know whether the way I'm handling the grouping of lines (using a map with a new private class to add the line number) or interpolated strings (counting nest levels) are best; I'm open to suggestions to improve them.

I don't know what the release schedule is, but it might be useful to bump the server version and get this in a dev SDK release and then let people play around with it properly (I'll need to push a Dart Code update, but that's mostly complete). WDYT?

[bwilkerson]

Let me know when you're ready for this to be merged.

[bwilkerson]

If you created a constructor you could make these fields final. Your call.

[DanTup]

Seems like a good idea; final always sounds better to me!

[bwilkerson]

Not necessary, but we often use a code pattern like this:

computer._closingLabelsByEndLine.putIfAbsent(end.lineNumber, () => <_ClosingLabelWithLineCount>[]).add(labelWithSpan);

which has less duplication of code. (The explicit type on the list literal is there because I don't think inference will be able to figure it out.)

[DanTup]

Cool; I was wondering if there was a nicer way to do that, didn't spot putIfAbsent, ta!

[bwilkerson]

... handling the grouping of lines ...

Seems reasonable to me. We should do some performance testing at some point, but I think the structure you have now is easy enough to maintain.

I don't know what the release schedule is ...

We'll release it as soon as we think it's ready, so it's pretty much up to us. The code will be in the next dev build, but that doesn't necessarily correspond to the update of the spec version.

I think you and I are reasonably happy with the API, but it would be worthwhile looping Alex in before we finalize it (to ensure that IntelliJ doesn't need something different). I'll send out an e-mail this morning.

[DanTup]

@bwilkerson I've pushed changes for the things you commented on. Merging is fine by me if you're happy with it. Let me know if anything comes out of your discussion with Alex.

[bwilkerson]

I'll note that in addition to running dartfmt, we use the "sort members" feature for all of our files. It helps reduce discussions about non-semantic coding issues (like where to put field definitions).

The build system requires formatting but not sorting, and I'll happily merge without sorting, but it anyone on our team touches these files they will get sorted.

[bwilkerson]

You could write the (whole) constructor as

_ClosingLabelWithLineCount(this.label, this.spannedLines);

This is semantically the same as what you wrote, but a little more compact.

[DanTup]

Thanks! I actually tried something like this (probably using typescript syntax, where you don't need to declare the fields if you put public/private in front of the constructor arg) but it failed. I should've looked it up!

[DanTup]

@bwilkerson I've run Sort members on the two test files plus the computer file (apologies - you had mentioned this, I just forgot it.. I should have an option to run during formats!).

I believe everything should've been formatted correctly (I do format-on-save) but lmk if you think anything is still wonky and I'll fix. Don't worry about refusing to merge over things some might picky; I'm more than happy to ensure my code is up to scratch (it's totally reasonable to expect you don't end up with some huge diff after making a trivial change because someone didn't format/sort correctly).

[DanTup]

(Oops, I'll sort the merge, seems Devon made an edit because I left an unused var).

[DanTup]

I'm a bit of a Git noob.. I merged this (since I presumed rebase would confuse discussions above, since the commits would be replaced with new IDs), but not sure if that complicates anything (it looks good on GH because it says it shows a condensed view of the merge commit). LMK if rebasing would be better and I'll redo.

[bwilkerson]

Just FYI, you don't need the type annotations; the type of the parameter is the same as the type of the field if it's omitted.

[DanTup]

Oops, I didn't copy your example very well! Updated now; much better 👍

[DanTup]

@bwilkerson I had an email from a buildbot saying a build failed and I'm the only name on the blamelist...

https://build.chromium.org/p/client.dart/builders/vm-precomp-win-simarm64-1-4-be/builds/2313

I'm not totally sure I understand why it's failed.. I'm assuming it's just flakey, but if you think it's something of mine, please let me know!

[bwilkerson]

There's nothing to worry about.

For future reference, I arrived at that conclusion by the following steps.

1. Open the build result page and look in the section titled Steps and Logfiles:.
2. Ignore steps and Failure reason because they rarely (never?) have information that isn't in the other steps.
3. The only other step that's red is test vm. It's unlikely that your code could have caused the VM to fail, but I'll look a little deeper just to be sure.
4. Click on stdio (under test vm) to open the log file and scroll to the end.
5. Look for a section labeled === Failure summary:.
6. Under that there are two sections that start like this:

FAILED: precompiler-dart_precompiled release_simarm64 language/regress_26543_2_test
Expected: Pass 
Actual: Timeout

That tells me that the test took longer than normal to run for some reason. I don't believe there's anything you wrote that could have caused the VM to run slower when executing code in the set of language tests, so you're off the hook.

[DanTup]

Great, thanks for the info 🙂