Skip to content

JS: Address some code that weren't affecting any query result #8422

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 10 commits into from
Mar 14, 2022

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Mar 13, 2022

As a followup to #8347 I checked if some code had become dead.
There was no new dead code, but I found some interesting things regardless.

  1. Some code doesn't affect query result.
    That is partly expected (e.g. a lot of classes in Expr.qll/TypeScript.qll are not used anywhere).
    But there were also some unexpected result, like a few Sanitizer classes that weren't referenced anywhere.
  2. Code that was already dead. That is, code that doesn't affect any query, and isn't referenced by any public API.
    (Not this PR, I'll address that in another PR).

This query partially addresses 1) in JS.
There's still plenty of code that doesn't affect any query, but all the remaining seems benign.

@github-actions github-actions bot added the JS label Mar 13, 2022
@erik-krogh erik-krogh marked this pull request as ready for review March 14, 2022 07:03
@erik-krogh erik-krogh requested a review from a team as a code owner March 14, 2022 07:03
@erik-krogh erik-krogh added the no-change-note-required This PR does not need a change note label Mar 14, 2022
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

Interesting findings. Thanks once again for doing this kind of cleanup!

One nit: Should all of the deprecations have a qldoc string pointing the users to the alternative?


Also, please confirm that https://github.com/github/codeql/blob/4fc85a791d45d931ec6dac0304e8c314eb0ba23a/javascript/ql/test/library-tests/Classes/tests.ql contains the inlined query predicate version of the files deleted in 5e52a71.

@erik-krogh
Copy link
Contributor Author

One nit: Should all of the deprecations have a qldoc string pointing the users to the alternative?

👍

Also, please confirm that https://github.com/github/codeql/blob/4fc85a791d45d931ec6dac0304e8c314eb0ba23a/javascript/ql/test/library-tests/Classes/tests.ql contains the inlined query predicate version of the files deleted in 5e52a71.

Confirmed 👍

@esbena
Copy link
Contributor

esbena commented Mar 14, 2022

We need a change note for the same reason as in #8323 (comment):

If people are going to start seeing deprecated warnings they weren't seeing before, this definitely requires a change note.

@erik-krogh erik-krogh mentioned this pull request Mar 14, 2022
@erik-krogh erik-krogh merged commit 7c4f9f9 into github:main Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants