JS: cleanup in API-graphs

[erik-krogh]

Changes based on some late feedback from #5419

I deleted the remaining references to CanonicalName, as they were no longer needed.
And added any(Type t).hasUnderlyingType(m, _) to get make sure all module names are recognized.

[max-schaefer]

Could you add a regression test that demonstrates the need for this addition?

[erik-krogh]

Hmmm. I can't find a situation where that line is needed. The imports(_, m) seems to cover all the cases I can come up with.
I still think it's a good idea to have, to have consistency with the MkHasUnderlyingType implementation.

[max-schaefer]

👍

[max-schaefer]

@erik-krogh, I made a PR against this branch (erik-krogh#2) to add back something that's roughly equivalent to the (useful parts of) MkCanonicalNameDef, but more along the lines of your MkHasUnderlyingType. What do you think?

(Happy to put it up as a separate PR if you prefer.)

[erik-krogh]

@erik-krogh, I made a PR against this branch (erik-krogh#2) to add back something that's roughly equivalent to the (useful parts of) MkCanonicalNameDef, but more along the lines of your MkHasUnderlyingType. What do you think?

It looks OK to me, and I like the renaming.
So I'm going to merge it into this PR.

But to me it looks like the new def node doesn't have an rhs, and it doesn't have any successors.
So the only thing I see it being used for is to check for the existence of a type.
Is that intended?

[max-schaefer]

So the only thing I see it being used for is to check for the existence of a type.
Is that intended?

Currently yes. The reason these nodes don't have an rhs is that their rhs would be a type definition, which isn't a data-flow node. For much the same reason, MkTypeUse nodes don't have any uses, since their uses would be type annotations, which again are not data-flow nodes.

I think it would be useful to consider giving MkTypeDefs an outgoing instance edge similar to what MkTypeUses have, for instance to look up methods and so on, but that's a somewhat more involved change that we can do separately.

[max-schaefer]

Whoops, looks like I broke your tests, and in an interesting way, too:

Let me look into that...

[max-schaefer]

...and now ppa.launchpad.net is unreachable; fun times!

[max-schaefer]

Ah, that's better!

[asgerf]

Sorry it's not really clear to me what the status of this PR is. Was there a 👍 from @max-schaefer and has there been an evaluation?

[max-schaefer]

Yes, that's a 👍 from me, but as a co-author of the PR I'd appreciate another pair of eyes. I think the last evaluation was from before my contributions.

[erik-krogh]

An updated evaluation shows a performance regression, but I think it is within the margin of error.

[asgerf]

Sorry for the late comment; I've looked over this PR several times already but always gotten bewildered by the lack context. It's just not clear to me why we're making this change.

[asgerf]

With this change it seems we're again mixing up type-names and value-names which I thought we could finally avoid when we introduced Node::ofType.

For example, for a module with exports

export const Foo = function() { .. }
export interface Foo { ... }

Doing getMember("Foo").getInstance() will return the nodes for both new Foo() and nodes for expressions of type Foo, which seem like they should always be the same, but often are not.

To get this straight we'd either need a separate member edge like typeMember for accessing the types exported from a namespace, or stop using instance for representing expression of a given type.

[max-schaefer]

@asgerf, it looks like your comments concern my additions to this PR. If @erik-krogh's original changes look good to you, then I suggest we back out my changes. They aren't high-priority enough for me to invest time in right now.

[asgerf]

Okay, well that's unfortunate. I think renaming the node to MkTypeUse still makes sense on its own, so we could stick to the first two commits for now?