Skip to content

JS: Various TypeScript extraction fixes. #15072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 14, 2023
Merged

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Dec 12, 2023

And add a new SEMMLE_TYPESCRIPT_SKIP_EXTRACTING_TYPES environment variable to skip extraction of all types.
This is a workaround for an internal report about long extraction times.

I'm not sure why those fixes weren't needed previously, and I couldn't replicate them when running tests.
They're clearly basic TypeScript errors, so I'm not sure what's going on.
I think it's something for @asgerf to look at when he's back?

I tried the new flag locally on microsoft/vscode.
The extraction got faster, and we only lost a single alert on that project.
And extraction is way faster with the flag on a minimal example from the internal report.

Evaluation were very uneventful (nightly-old.yml, typescript-full.yml)

e.g. the `lib.es5.d.ts` file was not excluded
`this` didn't refer to anything specific, and it was in fact `undefined` in the context it was invoked. There was already a  `let typeTable = this;` further up (where `this` refers to the class instance), so I used `typeTable`.
@github-actions github-actions bot added the JS label Dec 12, 2023
@erik-krogh erik-krogh marked this pull request as ready for review December 12, 2023 16:30
@erik-krogh erik-krogh requested a review from a team as a code owner December 12, 2023 16:30
@aibaars
Copy link
Contributor

aibaars commented Dec 12, 2023

Looks good to me. Shall we change the name of the environment variable to match the ones for extractor options?

@erik-krogh
Copy link
Contributor Author

Looks good to me. Shall we change the name of the environment variable to match the ones for extractor options?

That sounds like a good idea.
I haven't looked into those. Have you? What would the name be?

@aibaars
Copy link
Contributor

aibaars commented Dec 13, 2023

I haven't looked into those. Have you? What would the name be?

The prefix is CODEQL_EXTRACTOR_JAVASCRIPT_OPTION_ . If you want to document the flag as a proper extractor option you can add something like this https://github.com/github/codeql/blob/main/csharp/codeql-extractor.yml#L19-L50 to the codeql-extractor.yml file.

@erik-krogh
Copy link
Contributor Author

Thanks for the pointer.
The codeql-extractor.yml file for the JS extractor is in our internal repository, so I'll look into updating that file in a followup PR.

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

Successfully merging this pull request may close these issues.

2 participants