Skip to content
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

Add QL for QL #7410

Merged
merged 860 commits into from
Dec 17, 2021
Merged

Add QL for QL #7410

merged 860 commits into from
Dec 17, 2021

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Dec 15, 2021

This PR merges the QL for QL project into github/codeql.
Included is a new workflow that runs QL for QL on all PRs.

I moved the implementation into a ql/ folder, and I updated the workflow files such that they should work well in this repository.

You can see the implementation here.
That is the commit right before the merge (the merge was clean, there were no conflicting files).

You can inspect the results found by QL for QL by clicking on the failing code-scanning check in the bottom of this PR.

How to review

There are really only two things that need review:

The rest is the unmodified QL for QL implementation.

We don't have a QL for QL team, so I think everyone that is part of a CodeQL team is qualified to review (and approve) this PR.

esbena and others added 30 commits October 14, 2021 15:34
…step

New performance query: Transitive step in recursion.
New performance query: Transitive step in recursion.
Finds a single result in

```
semmle.code.java.dataflow.internal.rangeanalysis.SignAnalysisSpecific.qll
```
which starts with

```ql
module Private {
  import semmle.code.java.dataflow.RangeUtils as RU
  private import semmle.code.java.dataflow.SSA as Ssa
  private import semmle.code.java.controlflow.Guards as G
  private import java as J
  private import Sign
  ...
```
Finds a single result in

```
semmle.code.java.dataflow.internal.rangeanalysis.SignAnalysisSpecific.qll
```
which starts with

```ql
module Private {
  import semmle.code.java.dataflow.RangeUtils as RU
  private import semmle.code.java.dataflow.SSA as Ssa
  private import semmle.code.java.controlflow.Guards as G
  private import java as J
  private import Sign
  ...
```
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Would it be possible to configure categories for each of our CodeQL languages? (python, javascript, java, ...) that should allows us to filter down on just a single language when viewing Code Scanning alerts.

.github/workflows/ql-for-ql-build.yml Show resolved Hide resolved
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.

Some initial comments on the workflows. It may be best to try out the suggested changed outside of this PR to avoid the commit notification noise repeated failures could result in.

.github/workflows/ql-for-ql-build.yml Outdated Show resolved Hide resolved
.github/workflows/ql-for-ql-build.yml Show resolved Hide resolved
.github/workflows/ql-for-ql-build.yml Show resolved Hide resolved
.github/workflows/ql-for-ql-build.yml Outdated Show resolved Hide resolved
.github/workflows/ql-for-ql-build.yml Outdated Show resolved Hide resolved
.github/workflows/ql-for-ql-dataset_measure.yml Outdated Show resolved Hide resolved
.github/workflows/ql-for-ql-dataset_measure.yml Outdated Show resolved Hide resolved
.github/workflows/ql-for-ql-dataset_measure.yml Outdated Show resolved Hide resolved
.github/workflows/ql-for-ql-tests.yml Outdated Show resolved Hide resolved
.github/workflows/ql-for-ql-dataset_measure.yml Outdated Show resolved Hide resolved
@erik-krogh
Copy link
Contributor Author

I've fixed all the comments (some things are left for followup PRs).

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.

The workflow parts look good enough to merge now.

I do agree with Arthur that the codeql-cli download trick through codeql-action appears to just complicate matters with little tangible benefit. Perhaps the next iteration of that logic should use https://github.com/github/gh-codeql?

Some integration concerns:

  • Could we exclude the known test directories from extraction to avoid lots of noisy alerts?
  • Will we be overriding the results from the ordinary codeql-action run if that runs happen before this customized codeql action run (or vice versa)? I'm too fuzzy on how code-scanning behaves.

uses: actions/cache@v2
with:
path: ${{ runner.temp }}/query-pack.zip
key: queries-${{ hashFiles('**/*.ql*') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Technically, the dbscheme and its stats belong to this key as well. But so does the hash of the codeql-cli itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the version (from codeql-version) and the hash of the dbscheme files to the key.

@erik-krogh
Copy link
Contributor Author

Could we exclude the known test directories from extraction to avoid lots of noisy alerts?

Maybe... I think I'll look into that in a followup PR.
(I'll look into it when I address this comment by Rasmus).

Will we be overriding the results from the ordinary codeql-action run if that runs happen before this customized codeql action run (or vice versa)? I'm too fuzzy on how code-scanning behaves.

That is not an issue.
code-scanning is fine with multiple workflows uploading results.
Code-scanning uses categories to split out results of multiple workflows

@erik-krogh erik-krogh force-pushed the erik-krogh/publish-ql-for-ql branch 9 times, most recently from ec1b574 to 1b98661 Compare December 16, 2021 21:25
@erik-krogh erik-krogh merged commit c70a2be into github:main Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation no-change-note-required This PR does not need a change note QL-for-QL
Projects
None yet
Development

Successfully merging this pull request may close these issues.