search

github / vscode-codeql

An extension for Visual Studio Code that adds rich language support for CodeQL
https://marketplace.visualstudio.com/items?itemName=GitHub.vscode-codeql
MIT License
421 stars 184 forks source link

Add a graph viewer for `@kind graph` queries #877

Open aeisenberg opened 3 years ago

aeisenberg commented 3 years ago

@hvitved has created two PRs that implement a graph viewer, which have been sitting in our PR triage queue for over 6 months. I've learned that there are a number of users of the viewer. I think this is a great feature to have and there is nothing fundamentally wrong with the PRs as they are (I've given both a quick look in the past). But, I do have some concerns about merging:

  1. Graph viewers are tricky in that they often work for smaller graphs, but become unwieldy for larger graphs. We would need to do some good testing of the viewer to see what its limitations are.
  2. Similarly, a previous attempt to integrate a graph viewer hit up against performance issues for larger graphs. We would need to know if any performance issues exist and how to get around them.
  3. I don't have the time now to do any of this deep testing. (The purpose of this issue is to document the PRs and make sure that when I (Or someone else on the team) has the time, we will know what to do.)

One possibility is that we release the graph viewer as a canary-only feature so that regular users don't try it out with unrealistic expectations.

gsingh93 commented 3 years ago

Any update on this? Releasing it but requiring the user to enable it with an experimental flag seems to be a good option. It seems like the main issues you pointed out are related to more thorough testing, and users like myself are interested in doing that if it's easy to do so.

aeisenberg commented 3 years ago

My concern is that https://github.com/github/vscode-codeql/pull/811 changes some of the core datastructures in the extension and some of its uses are not very well tested, so I need to make sure that the PR doesn't break any existing behaviour (things like pagination and sorting). And, I'd like to make sure that the changes to the datastructures are the minimum necessary to get the graph viewer to work. This will take a non-trivial look at and test, which is why it has been open for so long.

I do intend to look at this, but haven't been able to spend the time just yet. I think this is a good change and would like to get this in, but I also want to be cautious.

kjcolley7 commented 2 years ago

This issue is a more generalized solution for #571 and is a duplicate of #384. I'd also like to put a huge +1 on this issue, the graph viewer from QL4E was one of my favorite and most-used features. I'd often run a query to display some partial function call graph, such as all function calls that eventually call some target function for determining reachability. I'm also interested in using it for things such as drawing CFGs, partial data flow graphs, modified ASTs for domain-specific languages, etc. I'm very willing to deal with limitations on overall graph size for performance reasons, at least in the initial implementation. Optimizing this to work on larger graphs could be a separate task.

I know that the old graph viewer had support for graph attributes. I think it would be awesome if a query could use graph/node/edge attributes (a la Graphviz) to heavily customize the way a graph is displayed. That's a bigger ask though, and the most important thing to me is the core functionality of displaying the graph and having nodes/edges be linkable to source locations. Being able to pan, zoom, and move nodes around is also nice.

Luuthetruyen commented 2 years ago

Truyen