editor/code: Enable `--noUncheckedIndexedAccess` & `--noPropertyAccessFromIndexSignature` ts option #15160

tetsuharuohzeki · 2023-06-28T09:22:09Z

This enables typescript's these option:

--noUncheckedIndexedAccess
- This checks whether indexed access is not null (or undefined) strictly just as like checking by std::option::Option::unwrap().
--noPropertyAccessFromIndexSignature
- This disallows bar.foo access if the bar type is { [key: string]: someType; }.

Additionally, to enable --noUncheckedIndexedAccess easily, this pull request introduces option-t as a dependency instead of defining a function in this repository like unwrapUndefinable() .

I'll remove it and define them byself if our dependency management policy is that to avoid to add a new package as possible.

lnicola · 2023-06-28T09:23:46Z

editors/code/tsconfig.json

@@ -3,6 +3,7 @@
    "compilerOptions": {
        "esModuleInterop": false,
        "module": "commonjs",
+        "moduleResolution": "Node16",


Should this be node16 as in the docs?

Ah, there is some reasons:

Future enhancement. This mode (node16) is more natural behavior with the current Node.js' package resolution than node (the current default value). And package.json's exports would be a wide spreading. I think it's better to enable as early as possible.

Compatibility with option-t that depends on exports field too.

Sorry -- I meant node16 instead of Node16, with a lower-case n.

(Mysteriously), it works in either. TypeScript compiler use lower-case but vscode's intellisense uses CamelCase.
I'll address it to lower-case.

I guess either is fine then as long as nothing complains.

I pushed the addressed commit: f780810

lnicola · 2023-06-28T09:32:35Z

LGTM except for the noUncheckedIndexedAccess commit, which feels really heavy-handed. But I guess I'm not opposed to that either.

r? @Veykril

lowr · 2023-06-28T16:00:20Z

I'm in favor of noUncheckedIndexedAccess, but I feel we can do better by considering the context instead of unconditionally "unwrapping" nullables. Unwrapping after checking array length/property existence/etc. seems redundant. For example, imo

// simplified for brevity
if (folders.length === 1) {
    const folder = unwrapUndefinable(folders[0]);
    return folder.uri.fsPath;
} else if (folders.length > 1) {
    const folder = unwrapUndefinable(folders[0]);
    // comments for justification
    return folder.uri.fsPath;
} else {
    return "";
}

should be

// comments for justification
const folder = folders[0];
return folder != null ? folder.uri.fsPath : "";

(I'm aware you mostly preserved the existing code, but we can always make them better ✨ )

Also, I'd rather not pull in another dependency for a utility function or two, but I'll leave the decision to others.

tetsuharuohzeki · 2023-06-29T15:55:09Z

I addressed @lowr 's comment.

@Veykril How do you think about this?

Veykril · 2023-07-03T14:41:56Z

Also, I'd rather not pull in another dependency for a utility function or two, but I'll leave the decision to others.

I agree here, we only use a single function from that library if I see this right, so if we can't get rid of the call locations for that, can we just hoist that function out into the local source here and drop the dependency?

https://www.typescriptlang.org/tsconfig#moduleResolution By this change, typescript compiler can read `exports` field in package.json for dependencies. https://nodejs.org/api/packages.html#exports-sugar

tetsuharuohzeki · 2023-07-04T16:01:51Z

@Veykril

I agree here, we only use a single function from that library if I see this right, so if we can't get rid of the call locations for that, can we just hoist that function out into the local source here and drop the dependency?

Sure. I addressed them and rebased my changeset again.

Veykril

One small question, but otherwise this should be good for now. Ideally we should get rid of the unwraps where we can, but most of them seem to be due to ts not being smart enough (like array length checks into indexing still resulting in undefined types)

Veykril · 2023-07-06T06:56:26Z

editors/code/src/commands.ts

@@ -129,7 +130,8 @@ export function matchingBrace(ctx: CtxInit): Cmd {
            ),
        });
        editor.selections = editor.selections.map((sel, idx) => {
-            const active = client.protocol2CodeConverter.asPosition(response[idx]);
+            const mayBeActive = client.protocol2CodeConverter.asPosition(response[idx]);


This confuses me, asPosition has the following overload asPosition(value: code.Position): proto.Position; which is what applies here, so why do we unwrap it. It shouldn't be undefined on the type level?

@Veykril

At here, response[idx] should be a ls.Position | undefined. Then TypeScript selects asPosition(value: ls.Position | undefined | null): code.Position | undefined; overload than asPosition(value: ls.Position): code.Position;.
https://github.com/microsoft/vscode-languageserver-node/blob/2041784436fed53f4e77267a49396bca22a7aacf/client/src/common/protocolConverter.ts#L34-L36

I'll update here to:

editor.selections = editor.selections.map((sel, idx) => { - const mayBeActive = client.protocol2CodeConverter.asPosition(response[idx]); - const active = unwrapUndefinable(mayBeActive); + const position = unwrapUndefinable(response[idx]); + const active = client.protocol2CodeConverter.asPosition(position); const anchor = sel.isEmpty ? active : sel.anchor; return new vscode.Selection(anchor, active); });

addressed: https://github.com/rust-lang/rust-analyzer/compare/edf14df49121e3d0d8d06bd9b650d4d74918a69e..f70845305f54181e1681828c7935305c66e743bb

Oh i missed the indexing into the array, that explains it sorry. Typescript inference is weird, given it tells me that indexing results in a non-undefined type ...
Nevertheless this change is better, thanks

https://www.typescriptlang.org/tsconfig#noUncheckedIndexedAccess

Veykril · 2023-07-06T07:20:19Z

Thanks
@bors r+

bors · 2023-07-06T07:20:21Z

📌 Commit f708453 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-07-06T07:20:28Z

⌛ Testing commit f708453 with merge 537f9b3...

bors · 2023-07-06T07:37:53Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 537f9b3 to master...

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2023

lnicola reviewed Jun 28, 2023

View reviewed changes

tetsuharuohzeki force-pushed the enable-noUncheckedIndexedAccess branch from bc9df53 to d1faf5d Compare June 28, 2023 09:39

tetsuharuohzeki force-pushed the enable-noUncheckedIndexedAccess branch 2 times, most recently from 597c7e5 to bf2a15a Compare June 28, 2023 16:19

editor/code: Set ts' --moduleResolution=node16 option

bb35d8f

https://www.typescriptlang.org/tsconfig#moduleResolution By this change, typescript compiler can read `exports` field in package.json for dependencies. https://nodejs.org/api/packages.html#exports-sugar

tetsuharuohzeki force-pushed the enable-noUncheckedIndexedAccess branch from bf2a15a to edf14df Compare July 4, 2023 16:00

tetsuharuohzeki requested a review from lnicola July 5, 2023 17:59

Veykril reviewed Jul 6, 2023

View reviewed changes

tetsuharuohzeki added 2 commits July 6, 2023 16:17

editor/code: Enable noUncheckedIndexedAccess ts option

72a3883

https://www.typescriptlang.org/tsconfig#noUncheckedIndexedAccess

editor/code: Enable noPropertyAccessFromIndexSignature ts option

f708453

tetsuharuohzeki force-pushed the enable-noUncheckedIndexedAccess branch from edf14df to f708453 Compare July 6, 2023 07:17

tetsuharuohzeki requested a review from Veykril July 6, 2023 07:17

bors merged commit 537f9b3 into rust-lang:master Jul 6, 2023
10 checks passed

tetsuharuohzeki deleted the enable-noUncheckedIndexedAccess branch July 6, 2023 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

editor/code: Enable `--noUncheckedIndexedAccess` & `--noPropertyAccessFromIndexSignature` ts option #15160

editor/code: Enable `--noUncheckedIndexedAccess` & `--noPropertyAccessFromIndexSignature` ts option #15160

tetsuharuohzeki commented Jun 28, 2023

lnicola Jun 28, 2023

tetsuharuohzeki Jun 28, 2023

lnicola Jun 28, 2023

tetsuharuohzeki Jun 28, 2023

lnicola Jun 28, 2023

tetsuharuohzeki Jun 28, 2023

lnicola commented Jun 28, 2023

lowr commented Jun 28, 2023 •

edited

tetsuharuohzeki commented Jun 29, 2023

Veykril commented Jul 3, 2023

tetsuharuohzeki commented Jul 4, 2023

Veykril left a comment

Veykril Jul 6, 2023

tetsuharuohzeki Jul 6, 2023

tetsuharuohzeki Jul 6, 2023

tetsuharuohzeki Jul 6, 2023

Veykril Jul 6, 2023

Veykril commented Jul 6, 2023

bors commented Jul 6, 2023

bors commented Jul 6, 2023

bors commented Jul 6, 2023

editor/code: Enable --noUncheckedIndexedAccess & --noPropertyAccessFromIndexSignature ts option #15160

editor/code: Enable --noUncheckedIndexedAccess & --noPropertyAccessFromIndexSignature ts option #15160

Conversation

tetsuharuohzeki commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnicola commented Jun 28, 2023

lowr commented Jun 28, 2023 • edited

tetsuharuohzeki commented Jun 29, 2023

Veykril commented Jul 3, 2023

tetsuharuohzeki commented Jul 4, 2023

Veykril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veykril commented Jul 6, 2023

bors commented Jul 6, 2023

bors commented Jul 6, 2023

bors commented Jul 6, 2023

editor/code: Enable `--noUncheckedIndexedAccess` & `--noPropertyAccessFromIndexSignature` ts option #15160

editor/code: Enable `--noUncheckedIndexedAccess` & `--noPropertyAccessFromIndexSignature` ts option #15160

lowr commented Jun 28, 2023 •

edited