Add code lens for quick evaluation by aeisenberg · Pull Request #1035 · github/vscode-codeql

aeisenberg · 2021-12-06T20:12:49Z

Replace this with a description of the changes your pull request makes. Closes #446

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.
@github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.

adityasharad

A good start, and nice to see what we can achieve just from the editor.

adityasharad · 2021-12-07T20:32:42Z

extensions/ql-vscode/src/CodeLensProvider.ts

+  Range
+} from 'vscode';
+
+class MyCodeLensProvider implements CodeLensProvider {


Give this class a meaningful name, e.g. QuickEvalCodeLensProvider. The file name can be codeLensProvider.ts to remain consistent with surrounding files.

adityasharad · 2021-12-07T20:35:59Z

extensions/ql-vscode/src/CodeLensProvider.ts

+        const startIndex = textLine.text.search(regex);
+        const range: Range = new Range(
+          textLine.range.start.line, startIndex,
+          textLine.range.end.line, startIndex - 1


Negative moves left, positive moves right.

Suggested change

textLine.range.end.line, startIndex - 1

textLine.range.end.line, startIndex + 1

I was thinking of something else when I made this change, but I didn't need to. Not sure now what that thought was. Changing back now. :)

adityasharad · 2021-12-07T20:36:36Z

extensions/ql-vscode/src/CodeLensProvider.ts

+      const textLine = document.lineAt(index);
+      const regex = new RegExp(/(?=(\w+\(\s*.*(?:,\s*)*\)\s*\{))\1/g);
+      if (textLine.text.match(regex)) {
+        const uri = document.uri;


Minor cleanup: just use document.uri directly in the list of arguments.

adityasharad · 2021-12-07T20:45:07Z

extensions/ql-vscode/src/CodeLensProvider.ts

+    for (let index = 0; index < document.lineCount; index++) {
+      const textLine = document.lineAt(index);
+      const regex = new RegExp(/(?=(\w+\(\s*.*(?:,\s*)*\)\s*\{))\1/g);
+      if (textLine.text.match(regex)) {


You're regex matching twice: once with match and once with search. That can be avoided by just calling const startIndex = textLine.text.search(regex). If there is no match this will be -1, if there is a match this will be the start index of the matching substring.

adityasharad · 2021-12-07T20:56:01Z

extensions/ql-vscode/src/CodeLensProvider.ts

+
+    for (let index = 0; index < document.lineCount; index++) {
+      const textLine = document.lineAt(index);
+      const regex = new RegExp(/(?=(\w+\(\s*.*(?:,\s*)*\)\s*\{))\1/g);


As written, this regex performs lookahead to find a pattern without capturing it, and then uses \1 to capture that same pattern. I think you can simplify that. Try something like this.

It's also a good idea to include a human-readable comment every time you write a regex, either explaining the intent or giving a short example. Reading them is hard 😄

Suggested change

const regex = new RegExp(/(?=(\w+$\s*.*(?:,\s*)*$\s*\{))\1/g);

// Match a predicate signature, including the predicate name, parameter list, and the opening brace.

const regex = new RegExp(/\w+$[^()]*$\s*\{/);

Making this change. I've always struggled writing regex (although I'm getting more comfortable with it now), so I always appreciate feedback. The only reason I included the positive lookahead was I was getting a code scanning failure about potential exponential backtracking. But I have also since simplified the regex, so maybe that won't be an issue this time.

Yup I think your new regex is free of this problem, because of the simpler handling of the (...) parameters!

If you'd like to learn more about exponential regexes, the CodeQL docs for the alert you saw are pretty good: https://un5kwke0ketx6vwhy3c87d8.julianrbryant.com/codeql-query-help/javascript/js-redos/. Exponential backtracking usually happens if you have something like (pattern1|pattern2)*, because at each step of matching the next character in the * sequence, we don't know whether we just saw a pattern1 or a pattern2, and have to backtrack to try both options if the next match fails.

extensions/ql-vscode/src/CodeLensProvider.ts

extensions/ql-vscode/src/extension.ts

extensions/ql-vscode/CHANGELOG.md

aeisenberg · 2021-12-08T00:10:44Z

Neat. I tried this out and it works. Though, it shows a quick eval suggestion even when the code is commented.

eg- this still shows a quick eval option:

// query BlockStmt foo(BlockStmt b) {
//   result = b and b.getNumStmt() >= 1
// }

aeisenberg · 2021-12-08T00:12:36Z

And the following code does not show a quick eval option (notice the space before the (:

query BlockStmt foo (BlockStmt b) {
  result = b and b.getNumStmt() >= 1
}

This is an inherent limitation of using regexes. I think it's important to fix the first problem I mentioned, but this one is less important.

aeisenberg · 2021-12-08T00:14:59Z

extensions/ql-vscode/src/CodeLensProvider.ts

+    for (let index = 0; index < document.lineCount; index++) {
+      const textLine = document.lineAt(index);
+      // Match a predicate signature, including predicate name, parameter list, and opening brace.
+      const regex = new RegExp(/\w+\(\s*.*(?:,\s*)*\)\s*\{/);


It would be really nice to get the name of the predicate in the codelens text. You can do that by capturing the word as below:

Suggested change

const regex = new RegExp(/\w+$\s*.*(?:,\s*)*$\s*\{/);

const regex = new RegExp(/(\w+)$\s*.*(?:,\s*)*$\s*\{/);

Then you can call: const matches = textLine.text.match(regex);

matches[1] contains the matched word.

Made these changes. I also added an additional whitespace match between the word match and opening '(' to account for any predicate definition that has that space.

aeisenberg · 2021-12-08T00:17:30Z

extensions/ql-vscode/src/CodeLensProvider.ts

+
+      const command: Command = {
+        command: 'codeQL.codeLensQuickEval',
+        title: 'CodeQL: Quick Evaluation',


Suggested change

title: 'CodeQL: Quick Evaluation',

title: `Quick Evaluation: ${matches[1]}`,

adityasharad

Good work wiring this together. A few follow up suggestions.

adityasharad · 2021-12-08T23:16:40Z

extensions/ql-vscode/src/quickEvalCodeLensProvider.ts

+      const startIndex = textLine.text.search(regex);
+
+      if (startIndex !== -1) {


Avoid calling both match and search, because you're doubling the computation cost at each line by regex matching twice. Now that we're using information from the result of match, stick with that.
matches.index will give you the start index you need, and if (matches) will give you the condition to check whether the match succeeded (because it returns null if there is no match.

Source: https://un5j2j18xhuv2emkwgjjkgb49yug.julianrbryant.com/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match

Thanks for pointing this out. I got it fixed now. :)

adityasharad · 2021-12-08T23:17:24Z

extensions/ql-vscode/src/extension.ts

+      async (
+        progress: ProgressCallback,
+        token: CancellationToken,
+        uri: Uri | undefined,


I don't think this can ever be undefined, so might as well make the type strict.

adityasharad · 2021-12-08T23:18:53Z

extensions/ql-vscode/src/extension.ts

+  const codelensProvider = new QuickEvalCodeLensProvider();
+
+  languages.registerCodeLensProvider({ scheme: 'file', language: 'ql' }, codelensProvider);


Please move this further down in the function: I don't think code lens creation should happen before we call languageSupport.install(), and certainly not before we've set up the extension and logging.

Moved it to be after languageSupport.install(), but if you think it should go further down in the function I can move it again.

adityasharad · 2021-12-08T23:32:15Z

extensions/ql-vscode/CHANGELOG.md


 ## [UNRELEASED]

+- Add CodeLens feature to make the CodeQL quick evaluation command more accessible. [#1035](https://github.com/github/vscode-codeql/pull/1035)


I've attempted to word this in a way that tells the user how to use the feature. What do you think?

Suggested change

- Add CodeLens feature to make the CodeQL quick evaluation command more accessible. [#1035](https://github.com/github/vscode-codeql/pull/1035)

- Add a CodeLens to make the Quick Evaluation command more accessible. Click the `Quick Evaluation` prompt above a predicate definition in the editor to evaluate that predicate on its own. [#1035](https://github.com/github/vscode-codeql/pull/1035)

I like it. I tried to make it concise, but I should have included more information.

aeisenberg

Looking really good. Just a couple of more small cleanups.

aeisenberg · 2021-12-09T21:53:27Z

extensions/ql-vscode/src/quickEvalCodeLensProvider.ts

+  
 class QuickEvalCodeLensProvider implements CodeLensProvider {


nit: you can delete this boilerplate.

Removed this 👍🏻

aeisenberg · 2021-12-09T22:01:22Z

extensions/ql-vscode/src/quickEvalCodeLensProvider.ts

+      if (matches) {
+        const range: Range = new Range(
+          textLine.range.start.line, matches.index!,
+          textLine.range.end.line, matches.index! + 1


Type assertion no longer necessary.

Suggested change

title: `Quick Evaluation: ${matches![1]}`,

title: `Quick Evaluation: ${matches[1]}`,

Change has been made. 👍🏻

aeisenberg · 2021-12-09T22:53:25Z

extensions/ql-vscode/src/quickEvalCodeLensProvider.ts

+      const regex = new RegExp(/(\w+)\s*\(\s*.*(?:,\s*)*\)\s*\{/);
+      const matches = textLine.text.match(regex);
+
+      if (matches) {


Ensure that commented code removes the code lens. It's too tricky to do in a single regex, so I think doing another check is ok.

Suggested change

if (matches) {

if (matches && textLine.text.search(/^\s*\//)) {

We had an illuminating discussion offline about why this works (search returns 0 when it matches the start of the line and -1 on mismatch, which JS converts into false and true respectively). I suggested a slightly more verbose but arguably simpler use of test instead -- hopefully that's still captured the meaning you suggested here!

A downside with the suggestion is that the regex is evaluated twice for every line, even if there is no initial match. The original suggestion only ran the second regex if the first one matched. I'm not sure if the extra overhead would be noticeable, but this code is run after every file change (in a background thread).

That's a good point. We can keep using !test rather than search, but keep it inline instead of in its own variable.
So if (matches && /.../.test(textLine.text)

Yep....that makes sense.

Made this change. Sorry it took a bit. I forgot I had resolved the CHANGELOG conflict from here but I hadn't updated my local branch yet. Had to get that squared away. Does it look better?

aeisenberg

Looks good, but I can't approve since I was the one who created the PR.

aeisenberg · 2021-12-09T23:22:38Z

@adityasharad Can you approve? I can't since I created this PR.

…odelens-quick-eval

…e-codeql into codelens-quick-eval

aeisenberg

(I can't approve.)

aeisenberg requested a review from a team as a code owner December 6, 2021 20:12

marcnjaramillo force-pushed the codelens-quick-eval branch from e73f4fc to d384943 Compare December 7, 2021 18:32

aeisenberg assigned marcnjaramillo Dec 7, 2021

adityasharad reviewed Dec 7, 2021

View reviewed changes

aeisenberg commented Dec 8, 2021

View reviewed changes

extensions/ql-vscode/CHANGELOG.md Outdated Show resolved Hide resolved

aeisenberg commented Dec 8, 2021

View reviewed changes

adityasharad reviewed Dec 8, 2021

View reviewed changes

aeisenberg commented Dec 9, 2021

View reviewed changes

Add code lens for quick eval command

b9c5e38

marcnjaramillo force-pushed the codelens-quick-eval branch from d4079cb to b9c5e38 Compare December 9, 2021 22:46

aeisenberg commented Dec 9, 2021

View reviewed changes

Ensure commented out predicates do not have code lens

5d40748

aeisenberg commented Dec 9, 2021

View reviewed changes

aeisenberg enabled auto-merge (rebase) December 9, 2021 23:21

Improve conditional check for commented out predicate detection

0a27a72

auto-merge was automatically disabled December 10, 2021 00:48
Head branch was pushed to by a user without write access

marcnjaramillo added 5 commits December 9, 2021 17:04

Refactor regex

452ad4d

Merge branch 'main' into codelens-quick-eval

a39e47f

Move comment check to eliminate evaluating regex more than once

806e594

Merge branch 'main' of github.com:marcnjaramillo/vscode-codeql into c…

c429c9a

…odelens-quick-eval

Merge branch 'codelens-quick-eval' of github.com:marcnjaramillo/vscod…

153a378

…e-codeql into codelens-quick-eval

aeisenberg commented Dec 10, 2021

View reviewed changes

adityasharad changed the title ~~Codelens quick eval~~ Add code lens for quick evaluation Dec 10, 2021

adityasharad approved these changes Dec 10, 2021

View reviewed changes

adityasharad enabled auto-merge (squash) December 10, 2021 19:15

adityasharad merged commit c8ed8b2 into github:main Dec 10, 2021

aeisenberg mentioned this pull request Dec 15, 2021

Add a configuration option to disable codeql codelens #1051

Closed

shati-patel mentioned this pull request Dec 16, 2021

Add a CodeLens for allowing users to run quick eval #446

Closed

	textLine.range.end.line, startIndex - 1
	textLine.range.end.line, startIndex + 1

	const regex = new RegExp(/(?=(\w+\(\s.(?:,\s)\)\s*\{))\1/g);
	// Match a predicate signature, including the predicate name, parameter list, and the opening brace.
	const regex = new RegExp(/\w+\([^()]\)\s\{/);

	const regex = new RegExp(/\w+\(\s.(?:,\s)\)\s*\{/);
	const regex = new RegExp(/(\w+)\(\s.(?:,\s)\)\s*\{/);

	title: 'CodeQL: Quick Evaluation',
	title: `Quick Evaluation: ${matches[1]}`,

		const startIndex = textLine.text.search(regex);

		if (startIndex !== -1) {

		const codelensProvider = new QuickEvalCodeLensProvider();

		languages.registerCodeLensProvider({ scheme: 'file', language: 'ql' }, codelensProvider);


		## [UNRELEASED]

		- Add CodeLens feature to make the CodeQL quick evaluation command more accessible. [#1035](https://github.com/github/vscode-codeql/pull/1035)

	- Add CodeLens feature to make the CodeQL quick evaluation command more accessible. [#1035](https://github.com/github/vscode-codeql/pull/1035)
	- Add a CodeLens to make the Quick Evaluation command more accessible. Click the `Quick Evaluation` prompt above a predicate definition in the editor to evaluate that predicate on its own. [#1035](https://github.com/github/vscode-codeql/pull/1035)


		class QuickEvalCodeLensProvider implements CodeLensProvider {

	title: `Quick Evaluation: ${matches![1]}`,
	title: `Quick Evaluation: ${matches[1]}`,

	if (matches) {
	if (matches && textLine.text.search(/^\s*\//)) {

Conversation

aeisenberg commented Dec 6, 2021 • edited by shati-patel 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aeisenberg commented Dec 8, 2021

Uh oh!

aeisenberg commented Dec 8, 2021

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!

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!

aeisenberg 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!

marcnjaramillo Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aeisenberg commented Dec 6, 2021 •

edited by shati-patel

Loading

marcnjaramillo Dec 9, 2021 •

edited

Loading