8000 Add experimental support for Jump-to-def and Find-references by jcreedcmu · Pull Request #337 · github/vscode-codeql · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add experimental support for Jump-to-def and Find-references #337

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 11 commits into from
Apr 28, 2020

Conversation

jcreedcmu
Copy link
Contributor

This adds the ability to use vscode command palette commands Go To Definition and Go To References in source archive files for which there are appropriately tagged queries in the appropriate qlpack. The qlpack is inferred from the metadata of the database the source archive came from, in more or less the same way that quick query does.

This ability is gated for the time being by a hidden feature flag in the user's vscode settings. Internal users can add

 "codeQL.experimentalFeatures": true,

to their settings json file.

An example of "appropriately tagged queries" for C++ is added by github/codeql#3308.

Checklist

  • CHANGELOG.md has not been updated yet, because this is not yet generally user visible.
  • @github/product-docs-dsp now is a great time to test-drive this, but it can be shipped as-is without documentation, because users won't be able to accidentally stumble on it undocumented.

Copy link
Contributor
@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Some small changes, mostly nits. Apologies, I don't have time to run this locally right now.

* the default CLI search path is used.
* @returns A list of query files found
*/
resolveQueriesInSuite(suite: string, additionalPacks: string[], searchPath?: string[]): Promise<string[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing the suite param in the tsdoc

args.push('--search-path', path.join(...searchPath));
}
args.push(suite);
return this.runJsonCodeQlCliCommand<string[]>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you mark this function as async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a fair question, and one which applies to a bunch of other Promise-returning functions in this file, but I think that I like the existing convention that we mark a function as async if it needs to be, i.e., actually uses await, and otherwise let its return type being Promise being a sufficient signal that what it returns isn't immediately a value but a future.

* "codeQl.experimentalFeatures" directly in their vscode settings
* json file.
*/
export const EXPERIMENTAL_FEATURES_SETTING = new Setting('experimentalFeatures', ROOT_SETTING);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice. I like Easter eggs. :)

const SELECT_QUERY_NAME = "#select";

enum KeyType {
DefinitionQuery, ReferenceQuery
Copy link
Contributor

Choose a reason for hiding this comment

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

For easier debugging, I like to do:

DefinitionQuery = 'DefinitionQuery',
ReferenceQuery = 'ReferenceQuery'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, can do. Iirc with ts enums, you do have maps in both directions (e.g. KeyType.ReferenceQuery = 1 and KeyType[1] = "ReferenceQuery") but I can understand not wanting to have the extra step.

Copy link
Contributor
@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I haven't tried this out yet, so can't say if it works.

@jcreedcmu jcreedcmu merged commit 8efb060 into github:master Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0