Skip to content

JS: Monorepo support: bugfixes and separate from dependency installation #3731

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 12 commits into from
Jun 26, 2020

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jun 17, 2020

This PR largely does four things:

  • Make extraction more deterministic by sorting files and choosing more carefully to extract TypeScript files with the right tsconfig.json file.
  • Bugfixes:
    • Use LGTM_SRC as source root instead of CWD. The latter only worked in codeql.
    • Inititialize fields correctly 🤦
  • Separate monorepo support from dependency installation. This means monorepo support is on by default but dependency installation is still feature flagged.
  • To avoid an OOM when extracting kibana, restrict properties extracted for type alias types.
    • Some projects do a lot of type meta-programming, where a type alias is essentially a function call at the type-level. From that point of view, the restriction on aliases makes sense IMO, and isn't as arbitrary as it might seem at first.
    • I'm getting less and less sure about the value we're getting out of the incomplete type property graph we're extracting. It's something we might consider removing if it gets in the way of more important things.

Evaluations

  • Metrics on typescript-full shows overall more call edges and more typed expressions. The few regressions are due to sorting files in a way that happens to be worse for those snapshots.
  • security/smoke-test was completely uneventful.
  • security/nightly shows a new result from the new call edges. Based on the above evaluation I'd say the timings are just noise.
  • A simple offline test shows the extraction time for kibana going from 2148s to 1808s, a speedup likely resulting from the restricted handling of aliasing.
  • This JS-Differences job shows the extraction time for vscode increase by 2%.

Dist upgrade

  • After review, I'd like to do a distribution upgrade to verify that we don't break the world.

Commit-by-commit review recommended.

@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Jun 17, 2020
@asgerf asgerf requested a review from a team as a code owner June 17, 2020 08:51
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

It all looks good to me.

But the tests are failing, and not in a good way.
I'm not sure what is happening, but it might be that you are missing some files during the extraction?

@asgerf
Copy link
Contributor Author

asgerf commented Jun 17, 2020

Oh, I completely messed up the alias check. Surprisingly it still seems to work for kibana after fixing that, but I'll run some evaluations again.

@asgerf
Copy link
Contributor Author

asgerf commented Jun 18, 2020

Another evaluation looks exactly the same after fixing that bug. ¯\_(ツ)_/¯

erik-krogh
erik-krogh previously approved these changes Jun 18, 2020
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Another evaluation looks exactly the same after fixing that bug. ¯\_(ツ)_/¯

¯\_(ツ)_/¯

👍

@asgerf
Copy link
Contributor Author

asgerf commented Jun 19, 2020

Hm, strangely, the only project failing the distribution build was kibana. 🤔

@asgerf
Copy link
Contributor Author

asgerf commented Jun 25, 2020

Rebased the PR after merging #3781. I've removed the special-casing of type aliases (pointless after #3781).

Will run some more evaluations with this new baseline.

@asgerf
Copy link
Contributor Author

asgerf commented Jun 26, 2020

New evaluations:

  • Security/typescript-full looks good. A few new results popped up due to call graph improvements.
  • Extraction time on vscode and angular increased by 4% and 17%, respectively, which seems reasonable.

@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jun 26, 2020
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Just a tiny non-blocking comment about documentation.

Otherwise LGTM 👍

/**
* Returns the virtual source root or <code>null</code> if no virtual source root exists.
*
* <p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* <p>
*

There is a whole bunch of the <p> throughout the documentation, and none of them have an corresponding </p>.
You don't have to fix this one, because there is a bunch more throughout our .java files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The </p> is not needed and our auto-formatter removes them.

Without the <p> javadoc won't create a separate paragraph for the following text, not even a newline.

@semmle-qlci semmle-qlci merged commit 1b4df57 into github:master Jun 26, 2020
* tsconfig.json file.
*/
public static final Comparator<Path> PATH_ORDERING = new Comparator<Path>() {
public int compare(Path f1, Path f2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this change introduces an alert on the internal LGTM instance:

This method overrides Comparator.compare; it is advisable to add an Override annotation.

* Like {@link #PATH_ORDERING} but for {@link File} objects.
*/
public static final Comparator<File> FILE_ORDERING = new Comparator<File>() {
public int compare(File f1, File f2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

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.

4 participants