Skip to content

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Sep 1, 2021

Adds support for TypeScript 4.4.
Adding support for TypeScript 4.4 was completely straight-forward, except for static initializer blocks.

It looks like static initializer blocks is making it's way into ECMAScript. So we should look into that later.
(I'll check it out after this PR is merged).

The List<Node> body; type in ClassBody is not the prettiest type.
But we need an ordered list that contains a mix of member-declarations and block-statements, so I think the only way to do better is to create an entirely new interface.

I haven't added getters for the new index signatures, because the available API still only exposes string/number index signatures.

No evaluation planned.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! Although as you say the representation of static initializers is indeed not very nice.

Couldn't we represent the static initializers using MemberDefinition? I mean that's literally defined as "a member definition in a class body". How about we add a new subclass of MemberDefinition, set the name to null and the value to the BlockStatement (and relaxing the bound on T).

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Sep 3, 2021

Couldn't we represent the static initializers using MemberDefinition? I mean that's literally defined as "a member definition in a class body". How about we add a new subclass of MemberDefinition, set the name to null and the value to the BlockStatement (and relaxing the bound on T).

I've done that.
But I've set the value to null instead of relaxing the bound on T for now.

I also need to do an update script, I'll do that later today.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks much better now 👍

@@ -155,6 +156,8 @@
import com.semmle.util.collections.CollectionUtil;
import com.semmle.util.data.IntList;

import jdk.internal.org.objectweb.asm.commons.StaticInitMerger;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray import

@erik-krogh
Copy link
Contributor Author

I added an upgrade script.

I've rebased on main, but other than that I didn't change the commit history.

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Sep 3, 2021

These are the failures from the CI:

    ql/javascript/ql/test/library-tests/CFG/CFG.ql: Extractor error (Extraction failed)
    ql/javascript/ql/test/library-tests/CFG/StaticInit.ql: Extractor error (Extraction failed)
    ql/javascript/ql/test/library-tests/TypeScript/Types/printAst.ql: Extractor error (Extraction failed)
    ql/javascript/ql/test/library-tests/TypeScript/Types/tests.ql: Extractor error (Extraction failed)

I'm not sure why, because it works just fine on my machine.

Edit: I'm even more confused.
It works just fine when I throw dca at it (report).

@asgerf
Copy link
Contributor

asgerf commented Sep 8, 2021

Hm, it also works on my machine. 😕

@erik-krogh
Copy link
Contributor Author

I've pulled the JS changes into this PR. Might make it easier to debug.
I'll ping when the tests start working.

@erik-krogh
Copy link
Contributor Author

ping @asgerf

The tests are working.
The StaticInitializer was a parent of a Statement, but @static_initializer wasn't in the @stmt_parent type.
So some of the parent values in the stmts relation didn't match the annotated type.

I still have no idea why that only failed in the CI.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad we got that sorted! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants