[labs/analyzer] add mixin support

[43081j]

Replaces #4120.

This is actually kevin's branch but caught up from main, and various tests added.

[changeset-bot]

🦋 Changeset detected

Latest commit: 0f47969

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

This PR includes changesets to release 2 packages

Name	Type
@lit-labs/analyzer	Patch
@lit-labs/gen-manifest	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

[43081j]

i've also tried to update all occurrences of throw new DiagnosticsError or throw new Error to instead create/add a diagnostic and return undefined.

seems to be what we do elsewhere in the codebase, but do correct me if im wrong

edit:

i've also dropped a test we had for static methods since it seems like it belongs in its own PR and was failing for unrelated reasons (think i lost some of the fixture code in merge).

[43081j]

fyi i've marked for review but i have some tests to write still

there's also a comment or two i left myself which would be good to resolve first

[43081j]

FYI i've also now updated gen-manifest to support mixins.

i still need to update the goldens at some point, though.

[43081j]

just did the minor changes @augustjk suggested (some package name updates) FYI

[AndrewJakubowicz]

Documentation on this function would be helpful since it's quite impressive what this function is doing, and it took me a while to understand it.

If I understand correctly, this function is finding which argument is the superClass. And it also is able to walk through the arguments.

Could this function be tricked to return the wrong super class with the following example:

const myMixin = (superClass, someArg) => {
  // This other mixin takes `someArg` as its first argument.
  class MyMixin extends someOtherMixin(someArg, superClass) {
    ...
  }
}

In this case I think someArg would be detected as the super class.

[AndrewJakubowicz]

I may be completely misunderstanding this function as well, but it may be worth adding an additional test case where a mixin applies some other mixin.

[43081j]

you understood correctly i think

it'll basically recursively resolve your example:

• initially it'll have the expression someOtherMixin(someArg, superClass) and some possible names of ['superClass', 'someArg']
• it'll then check if someArg is one of the possible names
• it is so it'll return index 1 (because its [1] of the possible names)

so you're right, it'll pick up someArg as the class arg rather than superClass

not sure what we can do about that though 🤔

[kevinpschaaf]

Good catch here, this does seem wrong. I think in the isCallExpression branch, rather than recursing into findSuperClassArgIndexFromHeritage(), this function likely needs to try to look up / dereference the mixin that's being called and return the index of its own argument passed into the mixin's superclass argument; that will recurse until a mixin extends one of its arguments directly (iow, taking the isIdentifier branch) rather than calling another mixin.

So in the example, we'd look up someOtherMixin, it would have a superClassArgIdx of 1, and so we'd find the index of the superClass argument, and search the current mixin's arg list for that name and return 0.

As Justin said in the other comment, the name searches could be likely be done with symbol declaration identity, but that could be a separate change.

[justinfagnani]

I think we might need to add something to the custom elements manifest here to identify which parameter of a mixin is the superclass.

We can do something internally to the analyzer in the mean time by just adding that to the model object.

[AndrewJakubowicz]

Some minor comments, but overall this is awesome! pre-approving!

Only blocking comments from me are:

1. The license date that was updated in an old file.
2. Clarifying that findSuperClassArgIndexFromHeritage works correctly.

Thank you so much!

[justinfagnani]

No action needed now (unless you want!), but this is enough parameters, especially with booleans and other literals, that I would generally go for named arguments.

[justinfagnani]

Is this finding the index in the mixin function's parameter list or the heritage call expression's argument list?

Note that in general, the superclass arg could be a named property too. I'm not sure what TypeScript's rules are.

[43081j]

it is the index in the function's parameter list, based on the heritage of the class

i think if we want to change this, we should probably do it in a follow-up since it could be a bit of a refactor to allow an object to be used as a parameter

[justinfagnani]

I'm not sure we should fix this, especially if TypeScript is strict about recognizing mixins. We can keep an eye on it.

[justinfagnani]

I think we should merge this now and open an issue to address known problems in the mixin superclass lookup.

Kevin said it’s ok in chat