[labs/analyzer] Analyze overloaded functions and methods. #3702

bicknellr · 2023-03-01T01:31:07Z

This PR adds an overloads field to FunctionLikeDeclaration and fills it with declarations for each overload signature.

… obvious.

…ion's implementation node.

…clarations.

changeset-bot · 2023-03-01T01:31:10Z

🦋 Changeset detected

Latest commit: 2feb32b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@lit-labs/analyzer	Minor
@lit-labs/cli	Patch
@lit-labs/gen-manifest	Patch
@lit-labs/gen-utils	Patch
@lit-labs/gen-wrapper-angular	Patch
@lit-labs/gen-wrapper-react	Patch
@lit-labs/gen-wrapper-vue	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-03-01T01:31:26Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -5% - +6% (-0.84ms - +0.99ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 79.03ms - 85.91ms
lit-html-kitchen-sink: unsure 🔍 -7% - +5% (-2.37ms - +1.66ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -6% - +17% (-0.69ms - +1.97ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +3% (-1.79ms - +1.55ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -5% - +2% (-2.65ms - +1.18ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 689.70ms - 703.70ms
lit-html-kitchen-sink: unsure 🔍 -2% - +6% (-1.18ms - +4.29ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -2% - +3% (-5.82ms - +7.74ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +1% (-1.18ms - +1.59ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +1% (-1.88ms - +5.31ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 694.96ms - 704.28ms
reactive-element-list: unsure 🔍 -0% - +1% (-2.29ms - +3.68ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 110.0.5481.177
Sample size: 50
Built by: Benchmarks #4485
Commit: fdb7964

render

Version	Avg time	vs
	79.03ms - 85.91ms	-

update

Version	Avg time	vs
	689.70ms - 703.70ms	-

update-reflect

Version	Avg time	vs
	694.96ms - 704.28ms	-

lit-html-kitchen-sink

Browser: chrome-headless 110.0.5481.177
Sample size: 30
Built by: Benchmarks #4485
Commit: fdb7964

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.47ms - 34.01ms	-	unsure 🔍 -7% - +5% -2.37ms - +1.66ms	unsure 🔍 -6% - +5% -1.94ms - +1.61ms
tip-of-tree tip-of-tree	31.54ms - 34.66ms	unsure 🔍 -5% - +7% -1.66ms - +2.37ms	-	unsure 🔍 -5% - +7% -1.80ms - +2.19ms
previous-release previous-release	31.67ms - 34.15ms	unsure 🔍 -5% - +6% -1.61ms - +1.94ms	unsure 🔍 -7% - +5% -2.19ms - +1.80ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	77.86ms - 81.22ms	-	unsure 🔍 -2% - +6% -1.18ms - +4.29ms	unsure 🔍 -3% - +5% -1.98ms - +4.22ms
tip-of-tree tip-of-tree	75.83ms - 80.14ms	unsure 🔍 -5% - +1% -4.29ms - +1.18ms	-	unsure 🔍 -5% - +4% -3.81ms - +2.94ms
previous-release previous-release	75.82ms - 81.02ms	unsure 🔍 -5% - +2% -4.22ms - +1.98ms	unsure 🔍 -4% - +5% -2.94ms - +3.81ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	16.15ms - 17.60ms	-	unsure 🔍 -5% - +6% -0.84ms - +0.99ms	unsure 🔍 -5% - +7% -0.81ms - +1.21ms
tip-of-tree tip-of-tree	16.25ms - 17.35ms	unsure 🔍 -6% - +5% -0.99ms - +0.84ms	-	unsure 🔍 -5% - +6% -0.77ms - +1.02ms
previous-release previous-release	15.97ms - 17.38ms	unsure 🔍 -7% - +5% -1.21ms - +0.81ms	unsure 🔍 -6% - +5% -1.02ms - +0.77ms	-

lit-html-repeat

Browser: chrome-headless 110.0.5481.177
Sample size: 30
Built by: Benchmarks #4485
Commit: fdb7964

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.28ms - 13.78ms	-	unsure 🔍 -6% - +17% -0.69ms - +1.97ms	unsure 🔍 -3% - +20% -0.28ms - +2.34ms
tip-of-tree tip-of-tree	11.44ms - 12.33ms	unsure 🔍 -15% - +5% -1.97ms - +0.69ms	-	unsure 🔍 -2% - +9% -0.21ms - +0.98ms
previous-release previous-release	11.11ms - 11.89ms	unsure 🔍 -18% - +1% -2.34ms - +0.28ms	unsure 🔍 -8% - +2% -0.98ms - +0.21ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	268.09ms - 278.24ms	-	unsure 🔍 -2% - +3% -5.82ms - +7.74ms	unsure 🔍 -2% - +4% -4.46ms - +9.49ms
tip-of-tree tip-of-tree	267.71ms - 276.71ms	unsure 🔍 -3% - +2% -7.74ms - +5.82ms	-	unsure 🔍 -2% - +3% -5.01ms - +8.12ms
previous-release previous-release	265.87ms - 275.44ms	unsure 🔍 -3% - +2% -9.49ms - +4.46ms	unsure 🔍 -3% - +2% -8.12ms - +5.01ms	-

lit-html-template-heavy

Browser: chrome-headless 110.0.5481.177
Sample size: 50
Built by: Benchmarks #4485
Commit: fdb7964

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.43ms - 55.75ms	-	unsure 🔍 -3% - +3% -1.79ms - +1.55ms	slower ❌ 1% - 6% 0.59ms - 3.38ms
tip-of-tree tip-of-tree	53.51ms - 55.92ms	unsure 🔍 -3% - +3% -1.55ms - +1.79ms	-	slower ❌ 1% - 7% 0.67ms - 3.54ms
previous-release previous-release	51.83ms - 53.39ms	faster ✔ 1% - 6% 0.59ms - 3.38ms	faster ✔ 1% - 6% 0.67ms - 3.54ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	113.19ms - 115.27ms	-	unsure 🔍 -1% - +1% -1.18ms - +1.59ms	unsure 🔍 -0% - +2% -0.01ms - +2.51ms
tip-of-tree tip-of-tree	113.12ms - 114.93ms	unsure 🔍 -1% - +1% -1.59ms - +1.18ms	-	unsure 🔍 -0% - +2% -0.11ms - +2.20ms
previous-release previous-release	112.27ms - 113.69ms	unsure 🔍 -2% - +0% -2.51ms - +0.01ms	unsure 🔍 -2% - +0% -2.20ms - +0.11ms	-

reactive-element-list

Browser: chrome-headless 110.0.5481.177
Sample size: 50
Built by: Benchmarks #4485
Commit: fdb7964

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	54.31ms - 56.82ms	-	unsure 🔍 -5% - +2% -2.65ms - +1.18ms	unsure 🔍 -3% - +4% -1.80ms - +1.98ms
tip-of-tree tip-of-tree	54.86ms - 57.74ms	unsure 🔍 -2% - +5% -1.18ms - +2.65ms	-	unsure 🔍 -2% - +5% -1.19ms - +2.84ms
previous-release previous-release	54.06ms - 56.89ms	unsure 🔍 -4% - +3% -1.98ms - +1.80ms	unsure 🔍 -5% - +2% -2.84ms - +1.19ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	698.22ms - 703.21ms	-	unsure 🔍 -0% - +1% -1.88ms - +5.31ms	unsure 🔍 -0% - +1% -2.51ms - +4.58ms
tip-of-tree tip-of-tree	696.41ms - 701.59ms	unsure 🔍 -1% - +0% -5.31ms - +1.88ms	-	unsure 🔍 -1% - +0% -4.29ms - +2.93ms
previous-release previous-release	697.16ms - 702.20ms	unsure 🔍 -1% - +0% -4.58ms - +2.51ms	unsure 🔍 -0% - +1% -2.93ms - +4.29ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	731.81ms - 736.13ms	-	unsure 🔍 -0% - +1% -2.29ms - +3.68ms	unsure 🔍 -0% - +0% -3.33ms - +3.02ms
tip-of-tree tip-of-tree	731.21ms - 735.34ms	unsure 🔍 -1% - +0% -3.68ms - +2.29ms	-	unsure 🔍 -1% - +0% -3.96ms - +2.26ms
previous-release previous-release	731.80ms - 736.45ms	unsure 🔍 -0% - +0% -3.02ms - +3.33ms	unsure 🔍 -0% - +1% -2.26ms - +3.96ms	-

_{tachometer-reporter-action v2 for Benchmarks}

packages/labs/analyzer/src/lib/javascript/classes.ts

packages/labs/analyzer/src/lib/javascript/functions.ts

justinfagnani · 2023-03-01T02:45:15Z

packages/labs/analyzer/src/lib/javascript/modules.ts

@@ -117,7 +117,9 @@ export const getModule = (
  for (const statement of sourceFile.statements) {
    if (ts.isClassDeclaration(statement)) {
      addDeclaration(getClassDeclarationInfo(statement, analyzer));
-    } else if (ts.isFunctionDeclaration(statement)) {
+      // Ignore non-implementation signatures of overloaded functions by
+      // checking for `statement.body`.


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).

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

throw new Error(

`Internal error: duplicate declaration '${name}' in ${sourceFile.fileName}`

);

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.

which would move the responsibility for matching TS's behavior or not to consumers of the manifest. That sounds good to me.

justinfagnani · 2023-03-01T02:48:21Z

packages/labs/analyzer/test-files/js/functions/functions.js

@@ -36,3 +36,46 @@ export function function2(a, b = false, ...c) {
 export default function () {
  return 'default';
 }
+
+/**
+ * This function has an overloaded signature in TS.


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.

Yes, the same tests are run twice (once for each analysis result from files in the ts and js 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.

justinfagnani · 2023-03-01T02:55:04Z

packages/labs/analyzer/src/lib/javascript/functions.ts

+
+  // Overloaded functions have mulitple declaration nodes. If there are no
+  // JSDoc tags on the provided declaration, use the first one that does have
+  // JSDoc tags for the purpose of extracting a description.


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

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.

…unctions

.changeset/early-berries-live.md

bicknellr added 10 commits February 28, 2023 13:24

Add tests for overloaded function exports and methods.

2923990

Ignore non-implementation signatures of overloaded functions.

2262994

Make the string concatenation in the overloaded example function more…

324191a

… obvious.

Add different JSDoc comment combinations for overloaded functions.

2d570d6

Add tests for overloaded function JSDoc comments.

d8e004a

Read JSDoc tags from another declaration if there are none on a funct…

d726897

…ion's implementation node.

Extract getDocNodeForFunctionLikeDeclaration.

7290b67

Add missing JSDoc tags to JS properties test fixture.

a8c92e4

Add support for retrieving JSDoc descriptions from method overload de…

666e657

…clarations.

Add a test for JSDoc descriptions on multiple overload signatures.

839d1e8

bicknellr changed the title ~~[labs/analyzer] Add support for extracting docs from overloaded functions and methods.~~ [labs/analyzer] Extract docs from overloaded functions and methods. Mar 1, 2023

bicknellr commented Mar 1, 2023

View reviewed changes

packages/labs/analyzer/src/lib/javascript/classes.ts Show resolved Hide resolved

bicknellr requested a review from kevinpschaaf March 1, 2023 01:46

bicknellr marked this pull request as ready for review March 1, 2023 01:46

bicknellr requested a review from justinfagnani as a code owner March 1, 2023 01:46

bicknellr commented Mar 1, 2023

View reviewed changes

packages/labs/analyzer/src/lib/javascript/functions.ts Outdated Show resolved Hide resolved

Add a changeset.

4bca6fa

justinfagnani reviewed Mar 1, 2023

View reviewed changes

bicknellr added 6 commits March 2, 2023 15:45

Add FunctionOverloadDeclaration.

049b531

Don't use documentation from one signature for another.

fb57847

Merge remote-tracking branch 'origin/main' into analyzer-overloaded-f…

8b5b4e9

…unctions

Collect function overloads.

4bd8d46

Fix tests for overloaded non-method functions.

7cadbac

Fix overloaded methods.

4755340

bicknellr changed the title ~~[labs/analyzer] Extract docs from overloaded functions and methods.~~ [labs/analyzer] Analyze overloaded functions and methods. Mar 3, 2023

Add a comment about checking for .body on method declarations.

591c4ea

bicknellr requested a review from justinfagnani March 3, 2023 02:39

justinfagnani approved these changes Mar 3, 2023

View reviewed changes

bicknellr commented Mar 3, 2023

View reviewed changes

.changeset/early-berries-live.md Outdated Show resolved Hide resolved

Update the changeset.

2feb32b

bicknellr merged commit 520b471 into main Mar 6, 2023

bicknellr deleted the analyzer-overloaded-functions branch March 6, 2023 18:36

lit-robot mentioned this pull request Mar 10, 2023

Version Packages #3717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[labs/analyzer] Analyze overloaded functions and methods. #3702

[labs/analyzer] Analyze overloaded functions and methods. #3702

bicknellr commented Mar 1, 2023 •

edited

changeset-bot bot commented Mar 1, 2023 •

edited

github-actions bot commented Mar 1, 2023 •

edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

justinfagnani Mar 1, 2023

bicknellr Mar 1, 2023

justinfagnani Mar 3, 2023

justinfagnani Mar 1, 2023

bicknellr Mar 1, 2023

justinfagnani Mar 1, 2023

bicknellr Mar 1, 2023

	throw new Error(
	`Internal error: duplicate declaration '${name}' in ${sourceFile.fileName}`
	);

[labs/analyzer] Analyze overloaded functions and methods. #3702

[labs/analyzer] Analyze overloaded functions and methods. #3702

Conversation

bicknellr commented Mar 1, 2023 • edited

changeset-bot bot commented Mar 1, 2023 • edited

🦋 Changeset detected

github-actions bot commented Mar 1, 2023 • edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

justinfagnani Mar 1, 2023

Choose a reason for hiding this comment

bicknellr Mar 1, 2023

Choose a reason for hiding this comment

justinfagnani Mar 3, 2023

Choose a reason for hiding this comment

justinfagnani Mar 1, 2023

Choose a reason for hiding this comment

bicknellr Mar 1, 2023

Choose a reason for hiding this comment

justinfagnani Mar 1, 2023

Choose a reason for hiding this comment

bicknellr Mar 1, 2023

Choose a reason for hiding this comment

bicknellr commented Mar 1, 2023 •

edited

changeset-bot bot commented Mar 1, 2023 •

edited

github-actions bot commented Mar 1, 2023 •

edited