Prefix trivial issues when enabling no-unchecked-record-access for client packages #23432

RishhiB · 2025-01-02T23:50:57Z

Prefix trivial issues when enabling no-unchecked-record-access for client packages
AB#25378

…ient packages

Copilot reviewed 12 out of 27 changed files in this pull request and generated no comments.

Files not reviewed (15)

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts: Evaluated as low risk
packages/drivers/odsp-driver/src/compactSnapshotParser.ts: Evaluated as low risk
packages/drivers/odsp-driver/src/odspDocumentStorageServiceBase.ts: Evaluated as low risk
packages/drivers/odsp-driver/src/odspDriverUrlResolver.ts: Evaluated as low risk
packages/dds/sequence/src/revertibles.ts: Evaluated as low risk
packages/drivers/odsp-driver/src/WriteBufferUtils.ts: Evaluated as low risk
packages/drivers/odsp-driver/src/test/prefetchSnapshotTests.spec.ts: Evaluated as low risk
packages/drivers/odsp-driver/src/test/getUrlAndHeadersWithAuth.spec.ts: Evaluated as low risk
packages/dds/map/src/test/mocha/directory.spec.ts: Evaluated as low risk
packages/runtime/container-runtime/src/test/containerRuntime.spec.ts: Evaluated as low risk
packages/drivers/replay-driver/src/storageImplementations.ts: Evaluated as low risk
packages/runtime/id-compressor/src/test/idCompressor.spec.ts: Evaluated as low risk
packages/drivers/routerlicious-driver/src/documentServiceFactory.ts: Evaluated as low risk
packages/dds/merge-tree/src/test/beastTest.spec.ts: Evaluated as low risk
packages/loader/driver-utils/src/test/summaryCompresssionTester.spec.ts: Evaluated as low risk

packages/dds/merge-tree/src/test/beastTest.spec.ts

alexvy86 · 2025-01-03T22:32:23Z

packages/tools/devtools/devtools-core/src/data-visualization/DataVisualization.ts

@@ -263,7 +263,8 @@ export class DataVisualizerGraph
 				this.visualizers[sharedObject.attributes.type] ?? visualizeUnknownSharedObject;

 			// Create visualizer node for the shared object
-			const editorFunction = this.editors[sharedObject.attributes.type];
+			const editorFunction: EditSharedObject | undefined =


I'm confused, how can this compile... we're saying that the variable here now might be undefined, but we're still using it without any additional checks in places that do not accept undefined (e.g. line 272(. And I see that TS is even saying that at that point the type is just EditSharedObject, but we never did anything to prevent undefined 🤔 . What am I missing?

Yea, weird, VisualizerNode accepts a strict EditSharedObject, but its not erroring when I type editorFunction to EditSharedObject | undefined andpass it in. what do you advise me to do? assert this? Non null assert this?

Ok I think I found the reason. The type of this.editors is

export interface SharedObjectEditors { [k: SharedObjectType]: EditSharedObject; }

Notably, EditSharedObject and not EditSharedObject | undefined. So even after adding | undefined to the variable, TS is inferring that what we assigned to it can only be a EditSharedObject, never undefined, so code further down doesn't complain. See this similar example in TSPlayground.

I think the typing of the index-property in SharedObjectEditors is probably incorrect and should have | undefined as well, but we should address that separately. I'll approve the devtools changes.

cc @Josmithr , FYI

As it turns out, TypeScript is basically ignoring this additional, explicit type annotation. It thinks that the assignment is more narrow (excludes undefined) and thus these are not useful and even misleading in many cases. If the TS option noUncheckedIndexAccess was used, then we'd be okay. For a poignant example, see https://github.com/microsoft/FluidFramework/pull/23423/files#r1906075392.
(just want Alex said)
So, these are all reverted in the revert PR.

Co-authored-by: Abram Sanderson <[email protected]>

alexvy86

Approving for devtools.

jason-ha · 2025-01-07T01:12:17Z

experimental/dds/attributable-map/src/mapKernel.ts

-						{ value: [nextVal.value[0], nextVal.value[1].value], done: false };
+						{ value: [nextVal.value[0], nextVal.value[1]?.value], done: false };


@RishhiB, @alexvy86, @Abe27342, why do we like this change?
First, the type of nextVal.value[1] is ILocalValue in all type evaluation cases. There is no chance of undefined here (assuming typing is accurate).
Second, this changes the runtime behavior. Before we would throw here if nextVal.value[1] was actually undefined; now the undefined element is stored in the result.

The fact that there was a complaint appears to be a failing of the linter rule. I suspect it doesn't recognize tuples.

My review was only for the devtools changes. I agree in this case the change in runtime behavior is reason enough to undo this bit and the similar ones throughout the PR.

I've opened #23486

jason-ha · 2025-01-07T01:18:44Z

packages/dds/sequence/src/revertibles.ts

 				revertible.intervals.push({
 					intervalId: interval.getIntervalId(),
-					label: interval.properties.referenceRangeLabels[0],
+					label: interval.properties.referenceRangeLabels?.[0],


@RishhiB, @alexvy86, @Abe27342, I don't know why TypeScript allows this. label is typed are string. The ? could allow undefined. Perhaps TypeScript is thinking referenceRangeLabels is always defined and then ignoring the ?.
This doesn't look like an okay code pattern. Either we assert with ! (which is removed at transpile) or do something to handle undefined case. Silent storing of undefined for a string appears incorrect.

…cord-access for client packages (microsoft#23432)" This reverts commit 88caebd where `?` was used to address linter defect. Many of the test uses of `?` in original change are permissible as there is a follow-up assertion that will fail. But those were not separated during revert. Changes from original commit that used expected safe adjustments are retained.

…rd-access for client packages (microsoft#23432)" This reverts commit 88caebd where `T | undefined` was used to address linter defect. TypeScript will narrow without `undefined` without noUncheckedIndexAccess enabled. So remove ineffective pattern. The only files that retained changes are: packages/dds/merge-tree/src/test/beastTest.spec.ts packages/drivers/odsp-driver/src/WriteBufferUtils.ts packages/runtime/id-compressor/src/test/idCompressor.spec.ts packages/tools/fetch-tool/src/fluidFetchSnapshot.ts

…23486) Partially revert "Prefix trivial issues when enabling no-unchecked-record-access for client packages (#23432)" This reverts commit 88caebd where: 1. `?` was used to address linter defect. TypeScript appears to mostly ignore that these cases may lead to `undefined` result which is not accepted per type specifications. Use of `?` is thus a behavior change that will shift point of failure away from where it could first be detected - revert those behavior changes. (Many of the test uses of `?` in original change are permissible as there is a follow-up assertion that will fail. But those were not separated during revert.) 2. `T | undefined` was used to address linter defect. TypeScript will narrow without `undefined` without `noUncheckedIndexAccess` enabled. The only files that retained changes from original commit are: - packages/dds/merge-tree/src/test/beastTest.spec.ts - packages/drivers/odsp-driver/src/WriteBufferUtils.ts - packages/runtime/id-compressor/src/test/idCompressor.spec.ts - packages/tools/fetch-tool/src/fluidFetchSnapshot.ts

Prefix trivial issues when enabling no-unchecked-record-access for cl…

83e38c5

…ient packages

RishhiB added 2 commits January 2, 2025 18:51

Merge branch 'main' into preFixFetchTool

9cc07a7

odsp changes

3403a09

github-actions bot added the area: odsp-driver label Jan 3, 2025

RishhiB added 4 commits January 3, 2025 00:23

container runtime and test service load trivial changes

8ef1cec

trivial container runtime changes

126d244

Merge branch 'main' into preFixFetchTool

90a1bbe

attributable-map changes

5d927da

RishhiB marked this pull request as ready for review January 3, 2025 16:57

Copilot bot review requested due to automatic review settings January 3, 2025 16:57

RishhiB requested review from a team as code owners January 3, 2025 16:57

Copilot AI reviewed Jan 3, 2025

View reviewed changes

Abe27342 reviewed Jan 3, 2025

View reviewed changes

packages/dds/merge-tree/src/test/beastTest.spec.ts Outdated Show resolved Hide resolved

Abe27342 approved these changes Jan 3, 2025

View reviewed changes

alexvy86 reviewed Jan 3, 2025

View reviewed changes

Update packages/dds/merge-tree/src/test/beastTest.spec.ts

a2deca9

Co-authored-by: Abram Sanderson <[email protected]>

alexvy86 approved these changes Jan 6, 2025

View reviewed changes

RishhiB merged commit 88caebd into microsoft:main Jan 6, 2025
34 checks passed

jason-ha reviewed Jan 7, 2025

View reviewed changes

jason-ha mentioned this pull request Jan 7, 2025

Partially revert #23432 "Prefix ... no-unchecked-record-access..." #23486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefix trivial issues when enabling no-unchecked-record-access for client packages #23432

Prefix trivial issues when enabling no-unchecked-record-access for client packages #23432

RishhiB commented Jan 2, 2025 •

edited by azure-boards bot

Loading

alexvy86 Jan 3, 2025

RishhiB Jan 6, 2025

alexvy86 Jan 6, 2025

alexvy86 Jan 6, 2025

jason-ha Jan 7, 2025 •

edited

Loading

alexvy86 left a comment

jason-ha Jan 7, 2025

jason-ha Jan 7, 2025

alexvy86 Jan 7, 2025

jason-ha Jan 7, 2025

jason-ha Jan 7, 2025

		{ value: [nextVal.value[0], nextVal.value[1].value], done: false };
		{ value: [nextVal.value[0], nextVal.value[1]?.value], done: false };

Prefix trivial issues when enabling no-unchecked-record-access for client packages #23432

Prefix trivial issues when enabling no-unchecked-record-access for client packages #23432

Conversation

RishhiB commented Jan 2, 2025 • edited by azure-boards bot Loading

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

jason-ha Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

alexvy86 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

RishhiB commented Jan 2, 2025 •

edited by azure-boards bot

Loading

jason-ha Jan 7, 2025 •

edited

Loading