Add a language label next to databases in the UI by aeisenberg · Pull Request #697 · github/vscode-codeql

aeisenberg · 2020-12-03T00:07:44Z

This change will only work on databases created by cli >= 2.4.1. In that
version, a new primaryLanguage field in the codeql-database.yml
file. We use this property as the language.

This change also includes a refactoring of the logic around extracting
database information heuristically based on file location. As much
as possible, it is extracted to the helpers module. Also, the
initial quick query text is generated based on the language (if known)
otherwise it falls back to the old style of generation.

Fixes #321

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

adityasharad

Refactoring looks good. I suggest getting the language from a CLI command instead of the YAML.

adityasharad · 2020-12-03T00:26:50Z

extensions/ql-vscode/src/helpers.ts

+  'semmlecode.javascript.dbscheme': 'javascript',
+  'semmlecode.cpp.dbscheme': 'cpp',
+  'semmlecode.dbscheme': 'java',
+  'semmlecode.python.dbscheme': 'pyhton',


Suggested change

'semmlecode.python.dbscheme': 'pyhton',

'semmlecode.python.dbscheme': 'python',

adityasharad · 2020-12-03T00:28:16Z

extensions/ql-vscode/src/helpers.ts

+  return !!path.basename(dbPath).startsWith('db-');
+}
+
+export async function getPrimaryLanguage(root: string) {


Instead of parsing the config file, make a call to codeql resolve database --format json and get the first value in the languages field.

Agreed. Especially because the open-sourced VSCode extension is our showcase for the Right way to do things.

That will also mean that the extension will not need its own dbSchemeToLanguage lookup table, since the codeql resolve database now already tries a dbscheme-based heuristic for language detection.

The reason I chose not to do that is that currently, there is no cli available on openDatabase. Also, making a filesystem check will be faster than starting up a jvm to get this information.

I could certainly stuff a reference to the cli into the constructor. But I'd prefer not to do that for the reasons above unless the approach I'm taking would produce incorrect results.

I would lean towards considering "people who write their own IDE integrations for CodeQL think this is the right thing to do, and proceed to bake half-understood internal knowledge of database internals into their own work" to be an incorrect result. :-)

On the other hand, I don't have the insight to appreciate the possible problems in dealing with "there is no cli available on openDatabase". Why is that?

The cli is a singleton that gets passed around when needed. The DatabaseManager does not hold a reference to the cli. I would need to change that in order to call a cli command here. It's not a big change, but it is an architectural one.

Also, I still need dbSchemeToLanguage as a fallback in case the database was not created by a modern CLI. I will add a comment around using that so it's known that this is not recommended.

So, the question is, should I be getting the database label from the codeql-database.yml or from database resolve? I'm not sure what your comment was on this or about the dbSchemeToLanguage.

hmakholm · 2020-12-03T16:22:46Z

Also, I still need dbSchemeToLanguage as a fallback in case the database was not created by a modern CLI. I will add a comment around using that so it's known that this is not recommended.

If the CLI is new enough to return languages from resolve database at all, it will already do its own dbscheme-based heuristics when it's asked about old databases.

So, the question is, should I be getting the database label from the codeql-database.yml or from database resolve? I'm not sure what your comment was on this or about the dbSchemeToLanguage.

I think the extension should rely exclusively on codeql resolve database. I would really want to avoid exposing hacky heuristics in the open-sourced extension. That's why I suggested adding a feature to resolve database in the first place.

aeisenberg · 2020-12-03T16:30:50Z

Two questions:

What happens if the CLI is not new enough to return a languages field? That's one scenario I am trying to handle. I guess in this case we could just avoid returning anything and this would be an ecouragement to migrate to a newer version. But we still need to use dbscheme heuristics for generating the import statements of quick queries
Are you saying that even if there is no primaryLanguage field in the codeql-database.yml, the database resolve command will still return the languages list?

EDIT: Looked at the code again and the answer to (2) seems to be "yes".

hmakholm · 2020-12-03T19:35:33Z

Hmmyes, an existing quick-query feature of course needs to keep working.

I was thinking more in terms of the new feature with showing language tags for databases in the UI. Here I think it is reasonably to expect that you need both a new extension and a CLI that supports the feature.

This change will only work on databases created by cli >= 2.4.1. In that version, a new `primaryLanguage` field in the `codeql-database.yml` file. We use this property as the language. This change also includes a refactoring of the logic around extracting database information heuristically based on file location. As much as possible, it is extracted to the `helpers` module. Also, the initial quick query text is generated based on the language (if known) otherwise it falls back to the old style of generation.

This commit moves to using codeql resolve database instead of inspecting the `codeql-database.yml` file. When the extension starts and if the cli supports it, the extension will attempt to get the name for any databases that don't yet have a name. Once a name is searched for once by the cli, it will be cached so we don't need to rediscover the name again.

aeisenberg · 2020-12-03T23:56:41Z

I think this addresses all the concerns. Look at the second commit for the changes. I rebased to remove merge conflicts.

hmakholm

I don't feel qualified to review the TS part, but I'm happy with the general approach to finding the language now.

hmakholm · 2020-12-04T00:02:42Z

extensions/ql-vscode/src/helpers.ts

+ * CLI versions before the langauge name was introduced to dbInfo. Implementations
+ * that do not require backwards compatibility should call
+ * `cli.CodeQLCliServer.resolveDatabase` and use the first entry in the


Implementations sounds like an "off" word to use here. Applications? Features?

Features sounds good.

adityasharad

A few minor questions.

adityasharad · 2020-12-04T00:27:29Z

extensions/ql-vscode/src/cli.ts

  private static CLI_VERSION_WITH_DECOMPILE_KIND_DIL = new SemVer('2.3.0');

+  /**
+   * CLI version where --kind=DIL was introduced


Copy-paste comment.

adityasharad · 2020-12-04T00:27:52Z

extensions/ql-vscode/src/cli.ts

+    return (await this.getVersion()).compare(CodeQLCliServer.CLI_VERSION_WITH_DECOMPILE_KIND_DIL) >= 0;
+  }
+
+  public async supportsLangaugeName() {


extensions/ql-vscode/src/cli.ts

adityasharad · 2020-12-04T00:31:38Z

extensions/ql-vscode/src/databases.ts

+
+  private async getPrimaryLanguage(dbPath: string) {
+    if (!(await this.cli.supportsLangaugeName())) {
+      // return undefined so that we continually recalculate until the cli version is bumped


What do you mean by recalculate?

I'll explain better.

adityasharad · 2020-12-04T00:36:45Z

extensions/ql-vscode/src/helpers.ts

+ * Returns the initial contents for an empty query, based on the language of the selected
+ * databse.
+ *
+ * First try to get the contents text based on language. if that fails, try to get based on


Suggested change

* First try to get the contents text based on language. if that fails, try to get based on

* First try to use the given language name. If that doesn't exist, try to infer it based on

adityasharad · 2020-12-04T00:38:19Z

extensions/ql-vscode/src/helpers.ts

+ * CLI versions before the langauge name was introduced to dbInfo. Implementations
+ * that do not require backwards compatibility should call
+ * `cli.CodeQLCliServer.resolveDatabase` and use the first entry in the


Features sounds good.

extensions/ql-vscode/src/databases.ts

aeisenberg requested a review from adityasharad December 3, 2020 00:07

aeisenberg force-pushed the aeisenberg/lang-label branch from 62654cc to 5a4b479 Compare December 3, 2020 00:10

adityasharad requested changes Dec 3, 2020

View reviewed changes

aeisenberg force-pushed the aeisenberg/lang-label branch 2 times, most recently from 22afde9 to 7d86f1b Compare December 3, 2020 05:16

aeisenberg added 2 commits December 3, 2020 15:51

aeisenberg force-pushed the aeisenberg/lang-label branch from c75044f to dfbcf81 Compare December 3, 2020 23:56

hmakholm approved these changes Dec 4, 2020

View reviewed changes

adityasharad reviewed Dec 4, 2020

View reviewed changes

Fix typos and augment comments around language labels

6b2410e

aeisenberg requested a review from adityasharad December 4, 2020 17:04

aeisenberg assigned adityasharad Dec 4, 2020

adityasharad approved these changes Dec 4, 2020

View reviewed changes

aeisenberg merged commit 06a1fd9 into github:main Dec 4, 2020

	'semmlecode.python.dbscheme': 'pyhton',
	'semmlecode.python.dbscheme': 'python',

	* First try to get the contents text based on language. if that fails, try to get based on
	* First try to use the given language name. If that doesn't exist, try to infer it based on

Conversation

aeisenberg commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmakholm commented Dec 3, 2020

Uh oh!

aeisenberg commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmakholm commented Dec 3, 2020

Uh oh!

aeisenberg commented Dec 3, 2020

Uh oh!

hmakholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aeisenberg commented Dec 3, 2020 •

edited

Loading

aeisenberg commented Dec 3, 2020 •

edited

Loading