[labs/analyzer] Collect and log non-fatal diagnostics. #3678

bicknellr · 2023-02-16T23:58:21Z

This adds a few methods to the analyzer for collecting and logging diagnostics. The analyzer no longer crashes when it sees a LitElement extension that has a non-identifier property name and instead adds a warning diagnostic. Also, the CEM generator now logs any diagnostics that were added after it generates the manifest.

…`ts.DiagnosticCategory`.

changeset-bot · 2023-02-16T23:58:24Z

🦋 Changeset detected

Latest commit: a0f3911

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	Minor
@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-02-16T23:58:41Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -2% - +7% (-0.38ms - +1.14ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 83.73ms - 90.11ms
lit-html-kitchen-sink: unsure 🔍 -6% - +6% (-2.00ms - +1.96ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -4% - +7% (-0.47ms - +0.80ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -4% - +2% (-2.00ms - +1.06ms)
^{this-change vs tip-of-tree}
⏱ reactive-element-list: unsure 🔍 -3% - +3% (-1.65ms - +1.88ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 845.11ms - 860.13ms
lit-html-kitchen-sink: unsure 🔍 -5% - +3% (-3.99ms - +2.22ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -2% - +1% (-7.18ms - +4.18ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +2% (-2.82ms - +2.58ms)
^{this-change vs tip-of-tree}
⏱ reactive-element-list: unsure 🔍 -1% - +1% (-8.80ms - +9.83ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 836.62ms - 846.10ms
⏱ reactive-element-list: unsure 🔍 -1% - +2% (-5.59ms - +13.52ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 110.0.5481.177
Sample size: 50
Built by: Benchmarks #4486
Commit: 4615f8e

render

Version	Avg time	vs
	83.73ms - 90.11ms	-

update

Version	Avg time	vs
	845.11ms - 860.13ms	-

update-reflect

Version	Avg time	vs
	836.62ms - 846.10ms	-

lit-html-kitchen-sink

Browser: chrome-headless 110.0.5481.177
Sample size: 30
Built by: Benchmarks #4486
Commit: 4615f8e

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.11ms - 35.65ms	-	unsure 🔍 -6% - +6% -2.00ms - +1.96ms	unsure 🔍 -5% - +7% -1.76ms - +2.37ms
tip-of-tree tip-of-tree	32.88ms - 35.91ms	unsure 🔍 -6% - +6% -1.96ms - +2.00ms	-	unsure 🔍 -6% - +8% -1.90ms - +2.55ms
previous-release previous-release	32.45ms - 35.70ms	unsure 🔍 -7% - +5% -2.37ms - +1.76ms	unsure 🔍 -7% - +5% -2.55ms - +1.90ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	81.40ms - 85.18ms	-	unsure 🔍 -5% - +3% -3.99ms - +2.22ms	unsure 🔍 -4% - +2% -3.52ms - +2.05ms
tip-of-tree tip-of-tree	81.71ms - 86.64ms	unsure 🔍 -3% - +5% -2.22ms - +3.99ms	-	unsure 🔍 -4% - +4% -3.05ms - +3.35ms
previous-release previous-release	81.97ms - 86.07ms	unsure 🔍 -2% - +4% -2.05ms - +3.52ms	unsure 🔍 -4% - +4% -3.35ms - +3.05ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	16.87ms - 18.12ms	-	unsure 🔍 -2% - +7% -0.38ms - +1.14ms	slower ❌ 0% - 9% 0.07ms - 1.42ms
tip-of-tree tip-of-tree	16.69ms - 17.55ms	unsure 🔍 -6% - +2% -1.14ms - +0.38ms	-	unsure 🔍 -1% - +5% -0.14ms - +0.88ms
previous-release previous-release	16.49ms - 17.01ms	faster ✔ 1% - 8% 0.07ms - 1.42ms	unsure 🔍 -5% - +1% -0.88ms - +0.14ms	-

lit-html-repeat

Browser: chrome-headless 110.0.5481.177
Sample size: 30
Built by: Benchmarks #4486
Commit: 4615f8e

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.98ms - 12.89ms	-	unsure 🔍 -4% - +7% -0.47ms - +0.80ms	unsure 🔍 -0% - +12% +0.01ms - +1.35ms
tip-of-tree tip-of-tree	11.83ms - 12.71ms	unsure 🔍 -6% - +4% -0.80ms - +0.47ms	-	unsure 🔍 -1% - +10% -0.14ms - +1.18ms
previous-release previous-release	11.26ms - 12.25ms	faster ✔ 0% - 11% 0.01ms - 1.35ms	unsure 🔍 -9% - +1% -1.18ms - +0.14ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	314.53ms - 321.49ms	-	unsure 🔍 -2% - +1% -7.18ms - +4.18ms	unsure 🔍 -3% - +0% -10.65ms - +1.62ms
tip-of-tree tip-of-tree	315.02ms - 324.00ms	unsure 🔍 -1% - +2% -4.18ms - +7.18ms	-	unsure 🔍 -3% - +1% -9.78ms - +3.75ms
previous-release previous-release	317.47ms - 327.58ms	unsure 🔍 -1% - +3% -1.62ms - +10.65ms	unsure 🔍 -1% - +3% -3.75ms - +9.78ms	-

lit-html-template-heavy

Browser: chrome-headless 110.0.5481.177
Sample size: 50
Built by: Benchmarks #4486
Commit: 4615f8e

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	54.79ms - 56.88ms	-	unsure 🔍 -4% - +2% -2.00ms - +1.06ms	unsure 🔍 -3% - +2% -1.61ms - +1.38ms
tip-of-tree tip-of-tree	55.19ms - 57.41ms	unsure 🔍 -2% - +4% -1.06ms - +2.00ms	-	unsure 🔍 -2% - +3% -1.19ms - +1.90ms
previous-release previous-release	54.88ms - 57.02ms	unsure 🔍 -2% - +3% -1.38ms - +1.61ms	unsure 🔍 -3% - +2% -1.90ms - +1.19ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	124.09ms - 128.07ms	-	unsure 🔍 -2% - +2% -2.82ms - +2.58ms	unsure 🔍 -2% - +2% -2.55ms - +2.90ms
tip-of-tree tip-of-tree	124.37ms - 128.03ms	unsure 🔍 -2% - +2% -2.58ms - +2.82ms	-	unsure 🔍 -2% - +2% -2.32ms - +2.91ms
previous-release previous-release	124.04ms - 127.78ms	unsure 🔍 -2% - +2% -2.90ms - +2.55ms	unsure 🔍 -2% - +2% -2.91ms - +2.32ms	-

⏱ reactive-element-list

Browser: chrome-headless 110.0.5481.177
Sample size: 50
Built by: Benchmarks #4480
Commit: e655481

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.98ms - 56.51ms	-	unsure 🔍 -3% - +3% -1.65ms - +1.88ms	unsure 🔍 -3% - +4% -1.41ms - +2.08ms
tip-of-tree tip-of-tree	53.90ms - 56.36ms	unsure 🔍 -3% - +3% -1.88ms - +1.65ms	-	unsure 🔍 -3% - +4% -1.50ms - +1.93ms
previous-release previous-release	53.72ms - 56.11ms	unsure 🔍 -4% - +3% -2.08ms - +1.41ms	unsure 🔍 -4% - +3% -1.93ms - +1.50ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	844.64ms - 858.14ms	-	unsure 🔍 -1% - +1% -8.80ms - +9.83ms	unsure 🔍 -1% - +1% -9.79ms - +6.97ms
tip-of-tree tip-of-tree	844.46ms - 857.29ms	unsure 🔍 -1% - +1% -9.83ms - +8.80ms	-	unsure 🔍 -1% - +1% -10.04ms - +6.18ms
previous-release previous-release	847.84ms - 857.77ms	unsure 🔍 -1% - +1% -6.97ms - +9.79ms	unsure 🔍 -1% - +1% -6.18ms - +10.04ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	855.33ms - 870.10ms	-	unsure 🔍 -1% - +2% -5.59ms - +13.52ms	unsure 🔍 -1% - +1% -8.26ms - +11.53ms
tip-of-tree tip-of-tree	852.68ms - 864.82ms	unsure 🔍 -2% - +1% -13.52ms - +5.59ms	-	unsure 🔍 -1% - +1% -11.29ms - +6.63ms
previous-release previous-release	854.49ms - 867.67ms	unsure 🔍 -1% - +1% -11.53ms - +8.26ms	unsure 🔍 -1% - +1% -6.63ms - +11.29ms	-

_{tachometer-reporter-action v2 for Benchmarks}

I thought it might be useful to collect diagnostics for each module individually and also have a single list with all diagnostics in a program, but there's nothing that needs this right now.

This mostly reverts commit 24ba127, but leaves the part that ignores function overloads.

…ith unsupported name types.

packages/labs/analyzer/test-files/ts/functions/src/functions.ts

packages/labs/analyzer/test-files/ts/properties/src/element-a.ts

bicknellr · 2023-02-28T01:42:58Z

packages/labs/analyzer/src/test/lit-element/properties_test.ts

+    // Fields named with symbols are visible in the `fields` iterator.
+    const field = Array.from(element.fields).find(
+      (x) => x.name === '[unsupportedPropertyName]'
+    );
+    assert.ok(field);


I think this behavior is really a bug. Symbol-named properties make it into fields here:

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

Lines 89 to 100 in be18292

} else if (ts.isPropertyDeclaration(node)) {

const info = getMemberInfo(node);

(info.static ? staticFieldMap : fieldMap).set(

node.name.getText(),

new ClassField({

...info,

default: node.initializer?.getText(),

type: getTypeForNode(node, analyzer),

...parseNodeJSDocInfo(node),

})

);

}

...which doesn't seem to care what type of name a property is using, but I'm not sure if that was intentional or not. node.name is the AST node for however the name of a field is described, so its getText() ends up returning the name with brackets around it, which looks reasonable but is really just JS text. (I think getText() would return ["abc" + "def"] for a field like class { ["abc" + "def"] = 123; }.)

hmm...is that a bug? What do we want the name to be? I could see that for the text representation, that something like [mySymbol] is about as good as we can get. But to fully support Symbol-named fields we probably want to represent them as a reference in the manifest, so we'd need to keep the name AST node.

I think that symbol-named fields are rare enough that we could punt on a few things, keep issues, then come back with a push to support symbols better, including adding name reference support to ClassField and ClassMethod: https://github.com/webcomponents/custom-elements-manifest/blob/main/schema.d.ts#L557

Justin what you said makes sense if name were only used as the display name in the model (probably the best we can do), but it's also used as the key in the map for getField(name), and you wouldn't look up a class model's symbol-based field using a string like getField('[mySymbol]') -- you'd probably need to construct and pass a reference.

Let's add a TODO (there's already an issue for dealing with Symbol-based fields), and I think it'd be fine to log an error/warning here if the name is not an identifier, at least for now.

I've updated this to also log a diagnostic rather than analyzing the property.

…h non-identifiers.

…nilla JS classes.

This reverts commit 1a98ee7.

bicknellr · 2023-03-06T20:57:12Z

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

@@ -73,6 +75,18 @@ export class Analyzer implements AnalyzerInterface {
      ),
    });
  }
+
+  addDiagnostic(options: DiagnosticOptions) {


Originally I had this parameter as a Diagnostic but then I changed it to DiagnosticOptions since it made the user-side of this API a bit cleaner but I'm still on the fence about it. WDYT?

bicknellr · 2023-03-06T21:00:40Z

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

+  *getDiagnostics(): IterableIterator<ts.Diagnostic> {
+    yield* this.diagnostics;
+  }


This feels like something that someone using the analyzer's API would want but it's only used in tests as of now. Should I update this to avoid putting on the public API for now?

rictic · 2023-05-31T03:05:30Z

Superceded by #3866

bicknellr added 7 commits February 15, 2023 17:33

format

45a4864

Collect declarations synchronously.

24ba127

Add addDiagnostic to AnalyzerInterface.

7da603d

createDiagnostic now accepts an options object which can contain a …

c5efd5e

…`ts.DiagnosticCategory`.

Add DiagnosticCollector.

768d260

Collect diagnostics while analyzing modules.

9dbab2d

Log all diagnostics collected while analyzing a package.

c79815a

bicknellr added 11 commits February 16, 2023 16:04

Update the warning text.

d54c5f5

Only start a new diagnostic context if the module isn't cached.

81b5602

Add a license header to the new file.

512b4c5

Remove DiagnosticCollector and the concept of nested contexts. (...)

98c6ad6

I thought it might be useful to collect diagnostics for each module individually and also have a single list with all diagnostics in a program, but there's nothing that needs this right now.

Collect declarations async again.

f1abec1

This mostly reverts commit 24ba127, but leaves the part that ignores function overloads.

Log any diagnostics collected while running generators.

dab12dd

Revert some no-op changes.

05dad89

Merge remote-tracking branch 'origin/main' into collect-diagnostics

aedabb4

Add an enum for diagnostic codes produced by the analyzer.

fd61707

Add a test that checks that a diagnostic is produced for properties w…

436aa20

…ith unsupported name types.

Add tests for overloaded function exports and methods.

da9dc35

bicknellr commented Feb 28, 2023

View reviewed changes

packages/labs/analyzer/test-files/ts/functions/src/functions.ts Outdated Show resolved Hide resolved

packages/labs/analyzer/test-files/ts/properties/src/element-a.ts Outdated Show resolved Hide resolved

bicknellr commented Feb 28, 2023

View reviewed changes

Replace the DiagnosticCode enum with an object.

eadf60b

bicknellr mentioned this pull request Mar 1, 2023

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

Merged

bicknellr added 3 commits March 1, 2023 15:15

Merge remote-tracking branch 'origin/main' into collect-diagnostics

f3130e2

Remove code related to supporting overloaded functions.

1809cf8

addDiagnostic now directly takes a DiagnosticOptions.

b68f395

bicknellr marked this pull request as ready for review March 2, 2023 20:58

bicknellr added 2 commits March 2, 2023 13:02

Add a changeset.

b9a901f

Prevent the LitElement analyzer from analyzing properties named wit…

b70b930

…h non-identifiers.

bicknellr added 4 commits March 2, 2023 13:23

Remove logDiagnostics method from Analyzer.

1a98ee7

Add a test for the UNSUPPORTED_PROPERTY_NAME_TYPE diagnostic for va…

bd9e8cc

…nilla JS classes.

Revert "Remove logDiagnostics method from Analyzer."

191eaa4

This reverts commit 1a98ee7.

Import AbsolutePath from the main file of @lit-labs/analyzer.

272da5a

bicknellr requested review from justinfagnani and kevinpschaaf March 2, 2023 22:30

Merge remote-tracking branch 'origin/main' into collect-diagnostics

a0f3911

bicknellr commented Mar 6, 2023

View reviewed changes

rictic closed this May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[labs/analyzer] Collect and log non-fatal diagnostics. #3678

[labs/analyzer] Collect and log non-fatal diagnostics. #3678

bicknellr commented Feb 16, 2023 •

edited

changeset-bot bot commented Feb 16, 2023 •

edited

github-actions bot commented Feb 16, 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

bicknellr Feb 28, 2023 •

edited

justinfagnani Feb 28, 2023

kevinpschaaf Feb 28, 2023 •

edited

bicknellr Mar 2, 2023

bicknellr Mar 6, 2023 •

edited

bicknellr Mar 6, 2023

rictic commented May 31, 2023

	} else if (ts.isPropertyDeclaration(node)) {
	const info = getMemberInfo(node);
	(info.static ? staticFieldMap : fieldMap).set(
	node.name.getText(),
	new ClassField({
	...info,
	default: node.initializer?.getText(),
	type: getTypeForNode(node, analyzer),
	...parseNodeJSDocInfo(node),
	})
	);
	}

[labs/analyzer] Collect and log non-fatal diagnostics. #3678

[labs/analyzer] Collect and log non-fatal diagnostics. #3678

Conversation

bicknellr commented Feb 16, 2023 • edited

changeset-bot bot commented Feb 16, 2023 • edited

🦋 Changeset detected

github-actions bot commented Feb 16, 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

bicknellr Feb 28, 2023 • edited

Choose a reason for hiding this comment

justinfagnani Feb 28, 2023

Choose a reason for hiding this comment

kevinpschaaf Feb 28, 2023 • edited

Choose a reason for hiding this comment

bicknellr Mar 2, 2023

Choose a reason for hiding this comment

bicknellr Mar 6, 2023 • edited

Choose a reason for hiding this comment

bicknellr Mar 6, 2023

Choose a reason for hiding this comment

rictic commented May 31, 2023

bicknellr commented Feb 16, 2023 •

edited

changeset-bot bot commented Feb 16, 2023 •

edited

github-actions bot commented Feb 16, 2023 •

edited

bicknellr Feb 28, 2023 •

edited

kevinpschaaf Feb 28, 2023 •

edited

bicknellr Mar 6, 2023 •

edited