Fix completion of partial identifier inside a JsxExpression

[pyBlob]

This originated from a typo (logical or instead of and) introduced here: #51855

I tried to use fourslash tests, but this didn't work, because they can only pass "includeInsertTextCompletions" to the preferences. This bug can only be triggered when passing the deprecated "includeInsertTextCompletions" to the CompletionsRequest.

Fixes #52288

[pyBlob]

@microsoft-github-policy-service agree

[jakebailey]

Hm. I didn't realize that you were saying you couldn't write a fourslash test because fourslash didn't have an option (I assumed it was some sort of hard to reproduce race). It might be easier just to add the option to fourslash.

[jakebailey]

That being said, if this is all due to a deprecated flag, do we need to make the client stop sending that flag?

[gabritto]

This looks like the right fix, but ideally we'd like to have a fourslash test instead of a unit test. Could you explain why passing includeInsertTextCompletions to the preferences in a fourslash test didn't work? It shouldn't make a difference whether includeInsertTextCompletions is passed to preferences or in the completions request itself, so maybe there is a bug in the fourslash test runner and we can look into that.

[pyBlob]

While the new includeInsertTextCompletions in the preferences should always override the deprecated includeInsertTextCompletions, this is not the case. I had a short look over the two remaining references of the deprecated property, and I think I found why it is like that. Will try this tomorrow. Moving the deprecated property before the spread of the preferences should solve this:

TypeScript/src/server/session.ts

Lines 2248 to 2252 in 2a8436c

	...convertUserPreferences(this.getPreferences(file)),
	triggerCharacter: args.triggerCharacter,
	triggerKind: args.triggerKind as CompletionTriggerKind | undefined,
	includeExternalModuleExports: args.includeExternalModuleExports,
	includeInsertTextCompletions: args.includeInsertTextCompletions,

That being said, if this is all due to a deprecated flag, do we need to make the client stop sending that flag?

Yes, when the mentioned change works out, you should be able to make the client stop sending that flag without a change in behaviour, and finally get rid of the deprecated property.

[gabritto]

I replaced the unit test with a fourslash test (and I checked that the fourslash test repros the issue), hope you don't mind. Thank you for the fix! :)

[pyBlob]

@gabritto Thanks for the fourslash test. I just came to the same conclusion while I've seen your commit pop up in the browser.

What confused me was my assumption that the deprecated property and the new preference have the same name, which is not the case. Apart from that, the old property and the new preference are merged correctly, and the client in vscode should be able to seamlessly switch to using the new preference.

[gabritto]

@gabritto Thanks for the fourslash test. I just came to the same conclusion while I've seen your commit pop up in the browser.

What confused me was my assumption that the deprecated property and the new preference have the same name, which is not the case. Apart from that, the old property and the new preference are merged correctly, and the client in vscode should be able to seamlessly switch to using the new preference.

I had the same moment of confusion with includeCompletionsWithInsertText and includeInsertTextCompletions, not realizing there were two different properties 😅