Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[labs/analyzer] Analyze overloaded functions and methods. #3702
[labs/analyzer] Analyze overloaded functions and methods. #3702
Changes from 10 commits
2923990
2262994
324191a
2d570d6
d8e004a
d726897
7290b67
a8c92e4
666e657
839d1e8
4bca6fa
049b531
fb57847
8b5b4e9
4bd8d46
7cadbac
4755340
591c4ea
2feb32b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript's behavior seems to be to choose the matched overload if there is one, or the first overload with jsdocs, but, surprisingly, never show the impl docs. The impl basically doesn't exist.
https://www.typescriptlang.org/play?ts=5.1.0-dev.20230227#code/PQKhCgAIUh5A3ApgJwDYHsCGATSBGKEYcAMwFcA7AYwBcBLdCyTACgA8AuSAZxuTooBzAJRd46OtgDc4UBGhwkaLLgBMhYuWr1GzdlwpkAtgCMUoyOMky5hSAEkjAB1QbSlWgyatOPPgMFIAB9IQ1NzSABvKEgqRm50VEQAOgxBdmEZAF9wWWBIAAl0JQBaYpQeAAt0AHduSAAiBBQMHEhVBvBWPEzc2wVm5TaCaE0PHSYTfT9+IQsraXdtL0gp3zCzZHmJRf6YRxc3LU9dNa5eWcCQjYjoyFj4xJS0jOy+-KLS8uQq2vqmpStXB4TpTHoyJYnJhUaYXALbayQiaxaY3LZiHY2MB2A6uUZIlYw3xwoTBULGTbCKIxOIUBJJVLodJsXo5PKFb5lJS-OqhdCQABW3Gw6Co3HAMPB4CAA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently having any overload signatures hides the implementation signature w.r.t. type checking call sites, so I guess from that perspective it makes sense that the implementation signature's docs aren't ever shown for a given call site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you maybe expand on this a bit? Like why do we want to ignore overloads? What do we do with different docs on different overloads? I don't the the CEM format explicitly handles overloads, so if we generated multiple declarations with the same name but different parameters, that would basically work by default - with the exception that tools might expect names to be unique.
I could see us documenting in the CEM that names aren't unique and duplicates indicate overloads (though we might want a way to mark and/or hide the implementation declaration because its signature might not be pretty).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, I went down this path because there are few places in the analyzer that currently require unique names (and I just assumed this was a requirement). Kevin's comment on my other PR about
getField()
is related. Another example is here, where declarations in modules are collected:lit/packages/labs/analyzer/src/lib/javascript/modules.ts
Lines 105 to 107 in be18292
I think your suggestion of explicitly documenting that names of potentially overloadable items in collections are not necessarily unique sounds like the best way to go. That way I could remove the weird documentation merging stuff here and we would avoid losing information in the output. Conveniently, the CEM schema already seems prepared for this since most (all?) collections of declarations are arrays of items with
name
properties and not an object / map that requires a unique key per item. This would also resolve the issue you pointed out in your other comment about this PR not matching TS's behavior around docs for overloads.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which would move the responsibility for matching TS's behavior or not to consumers of the manifest. That sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that mean exactly? Is this file associated with
ts/functions/src/functions.ts
so that those overloads apply here? I would have thought that these were separate modules.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the same tests are run twice (once for each analysis result from files in the
ts
andjs
directories) so the doc comments that those tests check for need to match. For overloaded functions in particular, the comments for the single declaration in JS need to match those from the particular overload that they're extracted from in TS. These could be rewritten to make what's happening here clearer though.However, it also sounds like I need to update this PR to handle overloads by producing multiple entries rather than trying to merge into a single one and I'm not sure how this is going to affect what's in the JS version of these fixtures yet. I'll try to make these more self-explanatory once I know what's happening with them.