Skip to content

Commit

Permalink
Revert "eslint(container-runtime): Prefix container-runtime before en…
Browse files Browse the repository at this point in the history
…abling no-unchecked-record-access" (#23499)

Reverts #23437

1. Where `?` 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. Where `T | undefined` was used to address linter defect, TypeScript
will narrow without `undefined` without `noUncheckedIndexAccess`
enabled. Thus, the code appears more confusing as there is a
non-respected type annotation.
  • Loading branch information
jason-ha authored Jan 8, 2025
1 parent c8670c4 commit acc9dbf
Show file tree
Hide file tree
Showing 10 changed files with 24 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const loadV1 = async (
return {};
}
let redirectTableEntries: [string, string][] = [];
const tableId: string | undefined = blobsTree.blobs[redirectTableBlobName];
const tableId = blobsTree.blobs[redirectTableBlobName];
if (tableId) {
redirectTableEntries = await readAndParse(context.storage, tableId);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1551,7 +1551,7 @@ export function getSummaryForDatastores(
}

if (rootHasIsolatedChannels(metadata)) {
const datastoresSnapshot: ISnapshotTree | undefined = snapshot.trees[channelsTreeName];
const datastoresSnapshot = snapshot.trees[channelsTreeName];
assert(!!datastoresSnapshot, 0x168 /* Expected tree in snapshot not found */);
return datastoresSnapshot;
} else {
Expand Down
3 changes: 1 addition & 2 deletions packages/runtime/container-runtime/src/containerRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ import {
ISequencedDocumentMessage,
ISignalMessage,
type ISummaryContext,
type SummaryObject,
} from "@fluidframework/driver-definitions/internal";
import { readAndParse } from "@fluidframework/driver-utils/internal";
import type { IIdCompressor } from "@fluidframework/id-compressor";
Expand Down Expand Up @@ -4152,7 +4151,7 @@ export class ContainerRuntime
// Counting dataStores and handles
// Because handles are unchanged dataStores in the current logic,
// summarized dataStore count is total dataStore count minus handle count
const dataStoreTree: SummaryObject | undefined = summaryTree.tree[channelsTreeName];
const dataStoreTree = summaryTree.tree[channelsTreeName];

assert(dataStoreTree.type === SummaryType.Tree, 0x1fc /* "summary is not a tree" */);
const handleCount = Object.values(dataStoreTree.tree).filter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import { IRequest } from "@fluidframework/core-interfaces";
import { assert, LazyPromise, Timer } from "@fluidframework/core-utils/internal";
import type { ISnapshotTree } from "@fluidframework/driver-definitions/internal";
import {
IGarbageCollectionDetailsBase,
ISummarizeResult,
Expand Down Expand Up @@ -227,7 +226,7 @@ export class GarbageCollector implements IGarbageCollector {

try {
// For newer documents, GC data should be present in the GC tree in the root of the snapshot.
const gcSnapshotTree: ISnapshotTree | undefined = baseSnapshot.trees[gcTreeKey];
const gcSnapshotTree = baseSnapshot.trees[gcTreeKey];
if (gcSnapshotTree === undefined) {
// back-compat - Older documents get their gc data reset for simplicity as there are few of them
// incremental gc summary will not work with older gc data as well
Expand Down
2 changes: 1 addition & 1 deletion packages/runtime/container-runtime/src/gc/gcConfigs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export function generateGCConfigs(
tombstoneTimeoutMs =
testOverrideTombstoneTimeoutMs ?? computeTombstoneTimeout(sessionExpiryTimeoutMs);

const gcGeneration = createParams.gcOptions?.[gcGenerationOptionName];
const gcGeneration = createParams.gcOptions[gcGenerationOptionName];
if (gcGeneration !== undefined) {
persistedGcFeatureMatrix = { gcGeneration };
}
Expand Down
4 changes: 2 additions & 2 deletions packages/runtime/container-runtime/src/gc/gcHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export function concatGarbageCollectionStates(
}

for (const [nodeId, nodeData] of Object.entries(gcState2.gcNodes)) {
let combineNodeData: IGarbageCollectionNodeData | undefined = combinedGCNodes[nodeId];
let combineNodeData = combinedGCNodes[nodeId];
if (combineNodeData === undefined) {
combineNodeData = {
outboundRoutes: Array.from(nodeData.outboundRoutes),
Expand Down Expand Up @@ -202,7 +202,7 @@ export async function getGCDataFromSnapshot(
continue;
}

const blobId: string | undefined = gcSnapshotTree.blobs[key];
const blobId = gcSnapshotTree.blobs[key];
if (blobId === undefined) {
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export function runGarbageCollection(

// Get the node for the referenced id and add its outbound routes to referencedIds since they are
// also referenced.
const routes: string[] | undefined = referenceGraph[id];
const routes = referenceGraph[id];
if (routes !== undefined) {
referencedIds.push(...routes);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ describe("Runtime", () => {
assert.strictEqual(snapshot.id, "channels-id", "Should be lower-level");
assert.strictEqual(Object.keys(snapshot.trees).length, 4, "Should have 4 datastores");
// Put in variable to avoid type-narrowing bug
const nonDataStore1: ISnapshotTree | undefined = snapshot.trees[nonDataStorePaths[0]];
const nonDataStore1 = snapshot.trees[nonDataStorePaths[0]];
assert.strictEqual(
nonDataStore1?.id,
"lower-non-datastore-1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ import { ContainerErrorTypes } from "@fluidframework/container-definitions/inter
import { IErrorBase, ITelemetryBaseEvent } from "@fluidframework/core-interfaces";
import { Timer } from "@fluidframework/core-utils/internal";
import { SummaryType } from "@fluidframework/driver-definitions";
import {
ISnapshotTree,
type SummaryObject,
} from "@fluidframework/driver-definitions/internal";
import { ISnapshotTree } from "@fluidframework/driver-definitions/internal";
import {
IGarbageCollectionDetailsBase,
ISummarizeResult,
Expand Down Expand Up @@ -1510,8 +1507,7 @@ describe("Garbage Collection Tests", () => {
assert(summaryTree?.summary.type === SummaryType.Tree, "The summary should be a tree");

// Get the deleted node ids from summary and validate that its the same as the one GC loaded from.
const deletedNodesBlob: SummaryObject | undefined =
summaryTree.summary.tree[gcDeletedBlobKey];
const deletedNodesBlob = summaryTree.summary.tree[gcDeletedBlobKey];
assert(
deletedNodesBlob.type === SummaryType.Blob,
"Deleted blob not present in summary",
Expand Down Expand Up @@ -1560,8 +1556,7 @@ describe("Garbage Collection Tests", () => {
assert(gcSummary?.summary.type === SummaryType.Tree, "The summary should be a tree");

// Get the deleted node ids from summary and validate that its the same as the one GC loaded from.
const deletedNodesBlob: SummaryObject | undefined =
gcSummary.summary.tree[gcDeletedBlobKey];
const deletedNodesBlob = gcSummary.summary.tree[gcDeletedBlobKey];
assert(
deletedNodesBlob.type === SummaryType.Handle,
"Deleted nodes state should be a handle",
Expand Down Expand Up @@ -1649,12 +1644,13 @@ describe("Garbage Collection Tests", () => {
assert(summaryTree.type === SummaryType.Tree, "Expecting a summary tree!");

let rootGCState: IGarbageCollectionState = { gcNodes: {} };
for (const [key, gcBlob] of Object.entries(summaryTree.tree)) {
for (const key of Object.keys(summaryTree.tree)) {
// Skip blobs that do not start with the GC prefix.
if (!key.startsWith(gcBlobPrefix)) {
continue;
}

const gcBlob = summaryTree.tree[key];
assert(gcBlob?.type === SummaryType.Blob, `GC blob not available`);
const gcState = JSON.parse(gcBlob.content as string) as IGarbageCollectionState;
// Merge the GC state of this blob into the root GC state.
Expand Down Expand Up @@ -1955,7 +1951,7 @@ describe("Garbage Collection Tests", () => {
defaultGCData.gcNodes[nodeE] = [nodeA];

// 4. Add reference from A to D with calling addedOutboundReference
defaultGCData.gcNodes[nodeA]?.push(nodeD);
defaultGCData.gcNodes[nodeA].push(nodeD);
garbageCollector.addedOutboundReference(nodeA, nodeD, Date.now());

// 5. Run GC and generate summary 2. E = [A -\> B, A -\> C, A -\> E, D -\> C, E -\> A].
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,15 @@ describe("GCSummaryStateTracker tests", () => {
);
assert(summary?.summary.type === SummaryType.Tree, "GC summary should be a tree");
assert(
summary.summary.tree[gcStateBlobKey]?.type === SummaryType.Blob,
summary.summary.tree[gcStateBlobKey].type === SummaryType.Blob,
"GC state should be written as a blob",
);
assert(
summary.summary.tree[gcTombstoneBlobKey]?.type === SummaryType.Handle,
summary.summary.tree[gcTombstoneBlobKey].type === SummaryType.Handle,
"Tombstone state should be written as handle",
);
assert(
summary.summary.tree[gcDeletedBlobKey]?.type === SummaryType.Handle,
summary.summary.tree[gcDeletedBlobKey].type === SummaryType.Handle,
"Deleted nodes should be written as handle",
);
});
Expand All @@ -106,15 +106,15 @@ describe("GCSummaryStateTracker tests", () => {
);
assert(summary?.summary.type === SummaryType.Tree, "GC summary should be a tree");
assert(
summary.summary.tree[gcStateBlobKey]?.type === SummaryType.Handle,
summary.summary.tree[gcStateBlobKey].type === SummaryType.Handle,
"GC state should be written as handle",
);
assert(
summary.summary.tree[gcTombstoneBlobKey]?.type === SummaryType.Blob,
summary.summary.tree[gcTombstoneBlobKey].type === SummaryType.Blob,
"Tombstone state should be written as a blob",
);
assert(
summary.summary.tree[gcDeletedBlobKey]?.type === SummaryType.Handle,
summary.summary.tree[gcDeletedBlobKey].type === SummaryType.Handle,
"Deleted nodes should be written as handle",
);
});
Expand All @@ -131,15 +131,15 @@ describe("GCSummaryStateTracker tests", () => {
);
assert(summary?.summary.type === SummaryType.Tree, "GC summary should be a tree");
assert(
summary.summary.tree[gcStateBlobKey]?.type === SummaryType.Handle,
summary.summary.tree[gcStateBlobKey].type === SummaryType.Handle,
"GC state should be written as handle",
);
assert(
summary.summary.tree[gcTombstoneBlobKey]?.type === SummaryType.Handle,
summary.summary.tree[gcTombstoneBlobKey].type === SummaryType.Handle,
"Tombstone state should be written as handle",
);
assert(
summary.summary.tree[gcDeletedBlobKey]?.type === SummaryType.Blob,
summary.summary.tree[gcDeletedBlobKey].type === SummaryType.Blob,
"Deleted nodes should be written as a blob",
);
});
Expand Down

0 comments on commit acc9dbf

Please sign in to comment.