fix broken regex on "src/services/completions.ts#getCompletionData" #37546

jeffy-g · 2020-03-24T06:22:58Z

Please verify that:

There is an associated issue in the Backlog milestone (required)
Code is up-to-date with the master branch
You've successfully run gulp runtests locally
There are new or updated unit tests validating the change

The regex literal /[^\*|\s|(/\*\*)]/ is obviously incorrect as a regex for the intent described in the line comment

thanks

src/testRunner/unittests/tsserver/completions.ts

jeffy-g · 2020-03-27T13:35:51Z

I wrote fourslash test
I would appreciate it if you have time to review

thanks

review of regex

sandersn · 2020-04-01T00:26:23Z

Couple of questions (that would normally be answered in a bug filed separately)

Can you describe the expected vs actual behaviour in a sentence or two?
How long has this been broken? The code mostly seems to have been written in 2017 March.

I'll try to take a look at the code when I have a chance.

jeffy-g · 2020-04-01T10:19:29Z

@sandersn Hi, thanks for your precious time :-)

Can you describe the expected vs actual behavior in a sentence or two?

Sorry, could not in a sentence or two :-

First, ts.Completions.getCompletionData has the following comment

When completion is requested without "@", we will have check to make sure that
there are no comments prefix the request position. We will only allow "*" and space.

1. expected vs actual behaviour

case	state	expected	actual
`/*+@\|c\| /`	before	no data	`CompletionDataKind.JsDocTagName`
	after	no data	no data
`/*\n ### text @\|c\|\n */`	before	no data	`CompletionDataKind.JsDocTagName`
	after	no data	no data

If there is a string other than " " or "*" due to my fix code this time
jsdoc tag related list will not appear (completion request by "@" etc)
As far as my fix code is concerned, it should fulfill the intent described in the line comment,
~~but the subsequent processing may return a jsdoc tag related list~~

2. How long has this been broken

The code mostly seems to have been written in 2017 March.

Yes, it doesn't seem to have changed since then.

I discovered about a year ago by LGTM - microsoft/TypeScript.
I remember it was at the top of the list at the time.

Few people can immediately judge /[^\*|\s|(/\*\*)]/ as "incorrect regular expression".

Since the jsdoc is formatted appropriately by the complement feature of tsserver(?),
it seems that the programmer does not particularly care and the problem did not surface.

thanks

sandersn

Looks good, I just want to understand how you were able to stop returning JsDocTagName when inside brackets. Was this code always wrong?

src/services/completions.ts

tests/cases/fourslash/completionsJsdocTag.ts

jeffy-g · 2020-05-04T15:14:13Z

@sandersn I'm sorry, my English reading ability is low and it is difficult to explain without misunderstanding 😭

breaking changes fourslash test Temporarily retain the before fix code for easy comparison

fourslash test

jeffy-g · 2020-05-11T12:11:55Z

@sandersn Originally I wanted to fix only /[^\*|\s|(/\*\*)]/, but I was worried about the behavior of the surrounding code, so settled on the current fix code.

When "@" is entered, JSDocTag name list is applied unconditionally, etc.
In *|c|, JSDocTag is listed when completion request is issued

However, if only modify the regex part without worrying about their behavior...

// Result of removing duplicate characters in regex class set part.
// Also, in this case, the `test` method is preferable to the `match` method, right?
if (!/[^\*|\s(/)]/.test(sourceFile.text.substring (lineStart, position))) {
     return {kind: CompletionDataKind.JsDocTag};
}

looks like above.

I suggest this fix.

sandersn · 2020-05-11T20:02:15Z

As-is, I think this is a nice cleanup of a misleading regex. However, could you make another PR with your other fix? It looked useful too. (No problem if you do not have time, we are happy for any contribution.)

@type

* upstream/master: (54 commits) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Fix for jsdoc modifiers on constructor params (microsoft#38403) Improve assert message in binder (microsoft#38270) fix broken regex on "src/services/completions.ts#getCompletionData" (microsoft#37546) report error for duplicate @type declaration (microsoft#38340) fix(38073): hide 'Extract to function in global scope' action for arrow functions which use 'this' (microsoft#38107) Update user baselines (microsoft#38472) Update user baselines (microsoft#38405) Changed template strings to emit void 0 instead of undefined (microsoft#38430) Fix js missing type arguments on existing nodes and jsdoc object literal declaration emit (microsoft#38368) LEGO: check in for master to temporary branch. Make isDynamicFileName available publicly (microsoft#38269) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Exclude arrays and tuples from full intersection property check (microsoft#38395) Fix crash caused by assertion with evolving array type (microsoft#38398) Update user baselines (microsoft#38128) LEGO: check in for master to temporary branch. moveToNewFile: handle namespace imports too ... # Conflicts: # src/compiler/types.ts # src/compiler/utilities.ts

jeffy-g · 2020-05-12T13:16:12Z

@sandersn I am happy that this PR has been applied :-)

However, could you make another PR with your other fix?

In parallel with this PR, I was creating and developing a branch to support the completion of the inline jsdoc tag.

Can almost certainly control the completion of the inline jsdoc tag,
but There is the question of which jsdoc specification to refer to for a block type jsdoc tag that allows an inline jsdoc tag.

Also, I think that PR regarding strict application of jsdoc tag completion will be proposed collectively if there is time.

thanks

sandersn · 2020-05-12T15:27:09Z

There is the question of which jsdoc specification to refer to for a block type jsdoc tag that allows an inline jsdoc tag.

Currently TS doesn't understand inline jsdoc tags. It explicitly skips @link tags, and that's the only inline tag I know about. Are there others?

jeffy-g · 2020-05-13T15:35:43Z

In the @use JSDoc specification, inline jsdoc tags other than @link include @linkcode, @linkplain, @tutorial, etc.

Other than @tutorial seems to be recognized by vscode.
could see that it is recognized by jsdoc preview.(This is a feature of vscode, right?)

/**  
 * ### The JSDoc {@linkcode https://en.wikipedia.org/wiki/JSDoc JSDoc (Wiki)}
 * * see {@link https://en.wikipedia.org/wiki/JSDoc JSDoc (Wiki) - &#64;link}
 * * see {@linkplain https://en.wikipedia.org/wiki/JSDoc JSDoc (Wiki) - @linkplain}
 * * see {@linkcode https://en.wikipedia.org/wiki/JSDoc|JSDoc (Wiki) - @linkcode}
 * * see {@tutorial https://en.wikipedia.org/wiki/JSDoc JSDoc (Wiki) - &#64;tutorial}
 * * [JSDoc (Wiki)](https://en.wikipedia.org/wiki/JSDoc)
 * * JSDoc (Wiki) - https://en.wikipedia.org/wiki/JSDoc
 * @param {string} a - postfix string, see {@link https://en.wikipedia.org/wiki/JSDoc JSDoc (Wiki) - &#64;link}
 * @see {@link https://en.wikipedia.org/wiki/JSDoc|JSDoc (Wiki)}
 * @see String.fromCharCode
 * @returns {ReturnType<typeof JSDoc>} this function return a `string`. see {@link https://en.wikipedia.org/wiki/JSDoc|JSDoc (Wiki)}
 */
const JSDoc = (a) => `JSDoc (${a})`;

jeffy-g added a commit to jeffy-g/TypeScript that referenced this pull request Mar 26, 2020

test of src/services/completions.ts#getCompletionData (microsoft#37546)

c0ac99f

sheetalkamat reviewed Mar 26, 2020

View reviewed changes

src/testRunner/unittests/tsserver/completions.ts Outdated Show resolved Hide resolved

sheetalkamat requested a review from sandersn March 26, 2020 17:22

sheetalkamat assigned sandersn Mar 26, 2020

jeffy-g added a commit to jeffy-g/TypeScript that referenced this pull request Mar 27, 2020

fourslash test for PR microsoft#37546

09bc47c

jeffy-g added a commit to jeffy-g/TypeScript that referenced this pull request Mar 29, 2020

moved fourslash test for PR microsoft#37546 to fourslash native category

da3116c

review of regex

jeffy-g force-pushed the completions_fix-broken-regex branch from 983bbf1 to 5b9da77 Compare March 30, 2020 14:35

sandersn added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 1, 2020

jeffy-g changed the title ~~fix broken regex on "src/services/completions.ts#840"~~ fix broken regex on "src/services/completions.ts#getCompletionData" Apr 1, 2020

jeffy-g force-pushed the completions_fix-broken-regex branch 2 times, most recently from a0160dd to 0bcf716 Compare April 4, 2020 15:25

jeffy-g force-pushed the completions_fix-broken-regex branch from 0bcf716 to b355a34 Compare April 10, 2020 12:04

sandersn requested changes Apr 29, 2020

View reviewed changes

src/services/completions.ts Outdated Show resolved Hide resolved

src/services/completions.ts Outdated Show resolved Hide resolved

tests/cases/fourslash/completionsJsdocTag.ts Outdated Show resolved Hide resolved

jeffy-g force-pushed the completions_fix-broken-regex branch from cc5f790 to 8dfc253 Compare April 29, 2020 14:06

jeffy-g added 9 commits May 11, 2020 19:12

fix broken regex on src/services/completions.ts#840

ece9f34

update forslash test

ff454d5

breaking changes fourslash test Temporarily retain the before fix code for easy comparison

regex

6ad12e0

fourslash test (Temporary code

3336102

regex: more restrictive in terms of jsdoc format

c4108f2

fourslash test

regex: max indent restriction

ada017e

achieved goal by partial match strategy (probably

92ac5cb

cleanup

8c58df3

apply requested changes

fe460f0

Change to a version with minimal modifications

3f1bb2d

jeffy-g force-pushed the completions_fix-broken-regex branch from 8dfc253 to 3f1bb2d Compare May 11, 2020 12:13

sandersn approved these changes May 11, 2020

View reviewed changes

sandersn merged commit fd71eb2 into microsoft:master May 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix broken regex on "src/services/completions.ts#getCompletionData" #37546

fix broken regex on "src/services/completions.ts#getCompletionData" #37546

jeffy-g commented Mar 24, 2020 •

edited

Loading

jeffy-g commented Mar 27, 2020

sandersn commented Apr 1, 2020

jeffy-g commented Apr 1, 2020 •

edited

Loading

sandersn left a comment

jeffy-g commented May 4, 2020

jeffy-g commented May 11, 2020 •

edited

Loading

sandersn commented May 11, 2020

jeffy-g commented May 12, 2020 •

edited

Loading

sandersn commented May 12, 2020

jeffy-g commented May 13, 2020 •

edited

Loading

fix broken regex on "src/services/completions.ts#getCompletionData" #37546

fix broken regex on "src/services/completions.ts#getCompletionData" #37546

Conversation

jeffy-g commented Mar 24, 2020 • edited Loading

jeffy-g commented Mar 27, 2020

sandersn commented Apr 1, 2020

jeffy-g commented Apr 1, 2020 • edited Loading

1. expected vs actual behaviour

2. How long has this been broken

sandersn left a comment

Choose a reason for hiding this comment

jeffy-g commented May 4, 2020

jeffy-g commented May 11, 2020 • edited Loading

sandersn commented May 11, 2020

jeffy-g commented May 12, 2020 • edited Loading

sandersn commented May 12, 2020

jeffy-g commented May 13, 2020 • edited Loading

jeffy-g commented Mar 24, 2020 •

edited

Loading

jeffy-g commented Apr 1, 2020 •

edited

Loading

jeffy-g commented May 11, 2020 •

edited

Loading

jeffy-g commented May 12, 2020 •

edited

Loading

jeffy-g commented May 13, 2020 •

edited

Loading