Better outlining spans for prototype methods

[dragomirtitian]

Fixes #31516

Hi @DanielRosenwasser, @amcasey

I know it's been a while since i said I would have a look at this, but I finally got around to finishing the implementation for this.
First the good news, this is what an ES5 class looks like now:

Now for the problems:

1. The implementation in VS code for outline view takes into account the node ranges returned when deciding how to display children. This means that the generated class node needs to encompass all the class members, otherwise the child nodes will not be displayed. This is fine if the members are contiguous, however it generates problems if there are other definitions between the members. The only ill effect I can see is that errors on these other definitions are displayed on the enclosing class.

I see some options:
a. Do nothing, the outline looks nice, although the bug report about this kind of writes itself
b. Create multiple additional spans for the class if the members are not contiguous. This is probably the best option but it can end up splitting up the class outline node:

c. Include any extra node in the class outline, maybe marking them in some distinctive way (although I am not sure what that would look like, and it would be kind of confusing)
d. Change the way errors are matched with outline nodes in VS-Code (probably a bigger change than we want to make for this feature)

2. Class static members. I tried to add to the outline view assignments that occur directly on the constructor (ie. static members) but ran into a problem.

Given assignments of the form

A.v = 1;
A.a = function() { };

It is impossible to tell without using the type checker, if the assignments will need to become static members on a class named A or they are just regular assignments. One option would be to keep all such assignments and remove them from the outline during the merging of outline nodes. This could end up creating a lot of extra nodes. My solution to limit the number of generated nodes was to track any found classes in the current scope, and only add nodes for the assignments above if a class like definition A was found in the current scope. A is considered a class like definition if a function A is declared, or if a something was already assigned to the prototype of A :

function A() { }
A.v = 1;
A.a = function() { };

I do not search the scope before-hand, but rather mark the class when I find it, this does mean that if static members are assigned before I mark the class they do not appear in the outline as part of the class:

The alternative might be to not add these members at all.

3. If two or more nodes can be merged a class will be generated, however if there is just one node there is a change in what is displayed but I think it's for the better:
   

[amcasey]

@dragomirtitian Thanks for looking into this! Regarding the problems you raised:

1. I think an analogous problem might already exist for namespaces. Superficial experimentation suggests that it creates a separate entry for each (contiguous) chunk of the namespace (your (b)). If I understand correctly that errors are propagated through the outline based on these scopes, that might be the best option (unless, for some reason, splitting up an ES5 class declaration is common practice).
2. I had independently spent some time thinking about how to fit expando objects into this model and was also unable to see a way to do it without semantic info. On the VS side, I'm pretty sure we're fine with this being a semantic operation, but VS Code might have different requirements. (To clarify, VS has no outline view, but the same code powers VS's Navigation Bars.)
3. I'm not totally sure I understand this point. I think the difference is that the parent node has the purple icon, rather than the orange icon?

[dragomirtitian]

@amcasey Thank you for your feedback.

1. I will add additional spans, this seemed like the best option to me too. (eta: End of week)

2. As far as I can tell, VS Code starts two language services instances a syntax and a semantic one. The outline runs on the syntactic instance. Also currently navigationBar.ts does not use the type checker at all, my guess this is by design to keep outline generation reasonably quick. (@DanielRosenwasser, @mjbvz can you please validate the assumptions I made here. Thank you.)

3. I was just referring to the fact that previously Something.prototype.method would show up as method->method now it shows up as Something->method. If there is a single prototype assignment Something is not promoted to a class node, it remains a method node as it did previously, just the name has changed.

[amcasey]

1. Cool
2. Interesting. VS specifically uses the semantic server, even though it could use the syntactic server, because populating the navbars isn't important enough to block whatever else the syntactic server might be doing.
3. If a solution jumps out at me, I'll let you know. Otherwise, your change seems like a net improvement and I'm fine dealing with that case later. @mjbvz may feel differently.

[amcasey]

@jessetrinity is doing work in this area and is probably a better person to sign off on the actual code change.

[dragomirtitian]

@amcasey @jessetrinity

I finished the changes for when ES5 classes are interwoven with other declarations. The outline is broken only if there is code that generates other outline nodes between class members:

If you think I should make any other changes let me know.

[jessetrinity]

Aside from the one issue I noted it's looking pretty good so far in VS!

[jessetrinity]

I finished the changes for when ES5 classes are interwoven with other declarations. The outline is broken only if there is code that generates other outline nodes between class members

I don't think we see that exact issue in VS, but it does look like our nav bar blows up when there is briefly an extra PropertyAccessExpression in there:

[jessetrinity]

I would want a sign off from @mjbvz as well for how the changes affect VSCode.

[dragomirtitian]

@jessetrinity Strange. I will check the spans returned for the outline nodes. I did not see similar behavior in VS Code 😕.

[mjbvz]

The overall presentation looks reasonable. Adding @jrieken in case he has any specific feedback on the error matching in the outline panel.

For implementation, VS Code uses navtree requests to power a few different features:

• Outline panel
• Go to symbol in document
• Breadcrumbs

These are important user facing features that should be available as soon as possible, so we run navtree against the syntax server in TS. Would that still be possible after these changes?

[jessetrinity]

FYI I want to change the presentation of NavBar items with #33040. Updating the tests between both of these might be a little rough. If you don't get these changes in before that I would be happy to resolve any conflicts.

[dragomirtitian]

@jessetrinity Thanks for the heads-up about this. It's been a hectic week and I haven't had time to finalize this. I will try to finish over the weekend.

[dragomirtitian]

@jessetrinity I fixed the issue with multi-line values in the navigation item name. The reason was that previously I would put whatever was the target of the prototype access (ie for C.prototype.member i would use C) as the name of the node. This works fine if the target for the prototype access is a simple identifier, if however we have a nested target, any spaces or newlines would make it into the name (ie for C⮒B . A.prototype.member would end up in the name being C⮒B . A) which was the cause of the issues in VS. (VSCode replaced the newline with a return arrow character so it was less noticeable, but still an issue).

My solution was to walk up the prototype access target and add nodes for each identifier in the path. This will remove any spaces and should work fine (I did think about how to do this without generating the extra nodes, but I think this is the best solution, the other solution I considered was to keeping the current names and use a regex to remove unwanted characters, but this seemed hacky. The extra number of nodes should not be an issue in most situations, I expect most such classes are not nested in deep namespaces)

[dragomirtitian]

For implementation, VS Code uses navtree requests to power a few different features:

• Outline panel
• Go to symbol in document
• Breadcrumbs

@mjbvz I don't see any reason the changes in this PR would be incompatible with these features. I did try to be mindful towards performance while improving the outline, so users should not see any performance degradation in these features. Other than that I don't see any other potential issues, so I think it will work well 😊

[mjbvz]

Change looks good as long as it can still run on the syntactic ts server

[jessetrinity]

the other solution I considered was to keeping the current names and use a regex to remove unwanted characters, but this seemed hacky.

I think we just merged something similar to keep the navbar from breaking in these situations (it wouldn't keep us from getting into them in the first place, like this PR now does).

The extra number of nodes should not be an issue in most situations, I expect most such classes are not nested in deep namespaces

You would think that, but it turns out people do this quite extensively. Either way after some discussion we have decided that seeing all of the nested nodes is probably the best way to present it.