Skip to content

Conversation

@gabritto
Copy link
Member

@gabritto gabritto commented Dec 4, 2025

In Strada, the document symbol tests (i.e. navigationTree) were not baseline tests. I initially ported those tests as they were in Strada, but some 40 tests failed, and mostly due to intentional differences, and so I just went through every failing test, fixed the most relevant differences, and re-ported the tests as baseline tests. Long term I think baseline tests are what we want for document symbol, and I think keeping diffs was going to be too noisy anyways.

I had to add support in document symbol for expandos and some other missing things like name truncation.

Noticeable differences:

  • Strada used to mark some function and variable declarations as classes in JS whenever they had an expando property, and that meant we added a "constructor" document symbol child to those symbols. I'm not doing this in Corsa.
  • In Corsa we don't look deeply into a node to determine its kind. So if e.g. you have a property assignment with a function, its kind is property, not method.
  • In Corsa we don't include symbols for nodes with missing names, i.e. cases where you'd have a parse error anyways (e.g. import *{} from 'foo' won't include an unnamed namespace import).
  • Corsa still doesn't add symbols for default export assignments. I think we added this in Strada in order to support code lenses, I am not sure if we still need to add those now.

Still missing:

  • Support for expandos via Object.defineProperty(...) is missing because we still haven't added support for this construct yet in Corsa. See navigationBarFunctionPrototype_test.go. If we add support for it by reparsing it into something else, we'll just need to update the newDocumentSymbol to allow that reparsed node, but that should be a quick change.

If the baseline format is ok, I can add a similar baselining function for workspace symbols too, since it's likely we'll want to use that for new tests going forward.

completionsAtIncompleteObjectLiteralProperty
completionsSelfDeclaring1
completionsWithDeprecatedTag4
navigationBarFunctionPrototype
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of those tests had .js files and needed // @allowJs: true to be added. Strada didn't need this because document symbol did not need to obtain the file from a program, it just obtained the file from a syntax cache (?). In Corsa, we try to get the requested file from the program, so these tests crashed. One thing I've been wanting to do is to use the same inferred project default settings for fourslash, but this would break a few import code fix tests, so I didn't do it in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants