Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

--noUncheckedIndexedAccess #39560

Merged
merged 11 commits into from Sep 11, 2020

Conversation

RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Jul 11, 2020

Pedantic Index Signatures noUncheckedIndexedAccess

Implements #13778

This is a draft PR for the purpose of soliciting concrete feedback and discussion based on people trying this out in their code. An npm-installable version will be available soon (watch for the bot).

Given current limitations in CFA we don't have any good ideas on how to fix, we remain guardedly skeptical this feature will be usable in practice. We're quite open to feedback to the contrary and that's why this PR is up - so that people can try out some bits and see how usable it is. This flag is not scheduled for any current TS release and please don't bet your entire coding strategy on it being shipped in a release! Anyway, here's the PR text.

Introduction

Under the flag --pedanticIndexSignatures, the following changes occur:

  • Any indexed access expression obj[index] used in a read position will include undefined in its type, unless index is a string literal or numeric literal with a previous narrowing in effect
  • Any property access expression obj.prop where no matching declared property named prop exists, but a string index signature does exist, will include undefined in its type, unless a previous narrowing on obj.prop is in effect

Other related behaviors are left unchanged:

  • Indexed access types, e.g. type A = SomeType[number], retain their current meaning (undefined is not added to this type)
  • Writes to obj[index] and obj.prop forms retain their normal behavior

In practice, it looks like this:

// Get a random-length array, perhaps
const arr = [1, 2, 3, 4, 5].slice(Math.random() * 5);
// Error, can't assign number | undefined to number
const n: number = arr[2];
if (arr[3]) {
    // OK, previous narrowing based on literal index is in effect
    arr[3].toFixed();
}

// Writes of 'undefined' are disallowed
arr[6] = undefined;

And like this:

const obj: { [s: string]: string } = { [(new Date().toString().substr(0, 3))]: "today" };
// Error, maybe it's not Tuesday
console.log(obj.Tue.toLowerCase());
if (obj.Fri) {
    // OK, previous narrowing on 'obj.Fri' in effect
   console.log(`${obj.Fri.toUpperCase()} IS FRIDAY`);
}

Restrictions on narrowing on non-literal indices (see below) mean this feature is not as ergonomic as some other TypeScript behavior, so many common patterns may require extra work:

let arr = [1, 2, 3, 4];
for (let i = 0; i < arr.length; i++) {
    // Error; 'i' might be out of bounds
    const n: number = arr[i];
    if (arr[i] !== undefined) {
        // Still an error; dynamic indices do not cause narrowing
        const m1: number = arr[i];
        i++;
        // This is sometimes a good thing, though!
        const m2: number = arr[i];
    }
}

Better patterns include for/of:

for (const el of arr) {
    // OK; for/of assumes no out-of-bounds problems
    const n: number = el;
}

forEach:

arr.forEach(el => {
    // OK; forEach won't exceed bounds
    const n: number = el;
});

Or stashing in a constant:

for (let i = 0; i < arr.length; i++) {
    const el = arr[i];
    if (el !== undefined) {
        // OK; constants may be narrowed
        console.log(el.toFixed());
    }
}

If you're targeting a newer environment or have an appropriate polyfill, you can also use Object.entries:

for (const [el, i] of Object.entries(arr)) {
    console.log(`Element at index ${i} had value ${el.toFixed()}`);
}

Other Caveats & Discussion

Naming

Name is up for bikeshedding, subject to the following caveats:

  • We do not intend to make this part of the --strict family with its current behavorial limitations, so anything starting with strict is a no-go
    • If, in the future, we find a way to make this palatable (i.e. can be turned on with nearly zero false positives), we can make it be an alias for a new --strictIndexSignatures setting
  • We don't really think this is a great mode to be in by default, so the name should connote "Here be dragons" in terms of ergonomics
  • The behavior changes both string and numeric index behavior, so we'd prefer to avoid Array as part of the name

More on Narrowing (or "Why doesn't my for loop just work?")

A predicted FAQ is why a "normal" C-style for loop is not allowed:

let arr = [1, 2, 3, 4];
for (let i = 0; i < arr.length; i++) {
    // Error; 'i' might be out of bounds... but it isn't, right?
    const n: number = arr[i];
}

First, TS's control flow system doesn't track side effects in a sufficiently rich way to account for cases like:

    if (arr[i] !== undefined) {
        arr.length = 0; // Side effect of clearing the array
        let x: number = arr[i]; // Unsound if allowed
    }

so it would be incorrectly optimistic to assume this kind of mutation (either to i or arr) hasn't occurred. In practice, in JS there's not much reason to use for instead of for/of unless you're doing something with the index, and that "something" is usually math that could potentially result in an out-of-bounds index.

Second, every narrowing is based on syntactic patterns, and adding the form e[i] to the list of things we narrow on incurred a 5-10% performance penalty. That doesn't sound like much in isolation, but if you and your ten best friends each got a new feature that came with a 5% performance penalty, you probably wouldn't like the fact that the next release of TypeScript was 70% slower than the last one.

Design Point: Sparse Arrays

A sparse array is created in JavaScript whenever you write to an index that is not already in-bounds or one past the current end of the array:

let arr = [0];
arr[4] = 4;
// arr is "[0, empty 脳 3, 4]"

JS behavior around sparse arrays is not the most predictable thing.

for / of

for(const el of arr) console.log(el)
// Prints 0, undefined, undefined, undefined, 4

馃憠 for/of treats a sparse array like an array with filled-in undefineds

forEach

arr.forEach(e => console.log(e))
// Prints 0, 4

馃憠 forEach skips the sparse elements

... (spread)

const arr2 = [...arr];
arr2.forEach(e => console.log(e))
// Prints 0, undefined, undefined, undefined, 4

馃憠 When you spread a sparse array, you get a non-sparse version of it.

Given these behaviors, we have decided to just assume that sparse arrays don't exist in well-formed programs. They are extremely rare to encounter in practice, and handling them "correctly" will have some extremely negative side effects. In particular, TS doesn't have a way to change function signatures based on compiler settings, so forEach is always to be passing its element type T (not T | undefined) to the callback. If sparse arrays are assumed to exist, then we need [...arr].forEach(x => x.toFixed()) to issue an error, and the only way to do that is to say that [...arr] produces (number | undefined)[]. This is going to be annoying as heck, because you can't use ! to convert a (T | undefined)[] to a T[]. Given how common patterns like const copy = [...someArr] are for working with arrays, this does not seem palatable even for the most strict-enthusiast programmers.

Design Point: What's the type of T[number] / T[string] ?

A question when implementing this feature is given this type:

type Q = (boolean[])[number];

Should Q be boolean (the type that's legal to write to the array) or boolean | undefined (the type you'd get if you read from the array)?

Inspecting this code:

// N.B. not a good function signature; don't do this in real code
function zzz<T extends unknown[]>(arr: T, i: number): T[number] {
    return arr[i];
}

It feels like the job of the checker in this mode is to say "zzz does a possibly out-of-bounds read and needs clarification". You could then write either of these instead:

// Allows out-of-bounds access to silently occur
function zzz1<T extends unknown[]>(arr: T, i: number): T[number] | undefined {
    return arr[i];
}
// - or - 
// Bounds checks for you
function zzz2<T extends unknown[]>(arr: T, i: number): T[number] {
    // TODO: Validate that 'i' is in bounds
    return arr[i]!; // ! - safety not guaranteed but we tried
}

This has the very positive side effect that it means T[number] in a declaration file doesn't change meaning based on whether the flag is set, and maintains the identity that

type A = { [n: number]: B }[number]
// A === B

@RyanCavanaugh RyanCavanaugh added the Experiment A fork with an experimental idea which might not make it into master label Jul 11, 2020
@microsoft microsoft deleted a comment from typescript-bot Jul 11, 2020
@Kingwl
Copy link
Contributor

Kingwl commented Jul 11, 2020

Congrats!
BTW: IS that mean the pedantic-stuff is already started锛

@Skillz4Killz
Copy link

Skillz4Killz commented Jul 12, 2020

@RyanCavanaugh Gave this a quick whirl on TS playground and I am seeing a bug.

image

This is on 4.0-beta where the bug does not occur
image

Note: Was testing to see if it would fix #36635 can you confirm if this PR should fix this issue? Thank you!

Second Note: Installed this on my project locally in the hopes of testing if it fixed Array Destructuring and sadly it does not. Wasn't this PR supposed to also fix #36635 that was the reason it and other issues like it were closed and marked duplicate?

Thoughts on the name: Perhaps consider advanced which could help imply that devs should only enable this if they are advanced TS-users wanting the most accurate typings.

@orta
Copy link
Contributor

orta commented Jul 12, 2020

Perhaps rigorous for the name?, I'd expect those to be stricter than strict.

C/C++ uses pedantic but it comes from a different age, Rust's official eslint equivalent also uses pedantic which must be pretty modern.

Some good ideas from asking the Internet:

  • Austere
  • Stringent
  • Nitpick

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Jul 13, 2020

@Skillz4Killz a few things

  • Destructuring was in fact missed! I just pushed a few commits to implement it. There is still one TODO around the form ({ e } = (some expression of the form { e: number } & { [s: string]: number }))
  • The bug you're pointing to exists in the nightly build and is a Playground bug; it doesn't repro in tsc (cc @orta - this is happening in all nightly-based Playgrounds)
  • This feature can't be tested in the Playground because it's off by default and there's no Playground UI for toggling it

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Jul 13, 2020

@typescript-bot pack this

@microsoft microsoft deleted a comment from typescript-bot Jul 13, 2020
@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 13, 2020

Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/79615/artifacts?artifactName=tgz&fileId=5CD1710BC0ABDF65CF05EB16DEFD53CD67C727BEA5B29283F7C5D7A52E9DEED002&fileName=/typescript-4.0.0-insiders.20200713.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@microsoft microsoft deleted a comment from typescript-bot Jul 13, 2020
@microsoft microsoft deleted a comment from typescript-bot Jul 13, 2020
@orta
Copy link
Contributor

orta commented Jul 13, 2020

Made an issue: microsoft/TypeScript-Website#766

@awerlogus
Copy link

awerlogus commented Jul 15, 2020

Is it possible to enable this feature in tsconfig file?

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Jul 15, 2020

@awerlogus yes, it works the same way as other compiler options

@awerlogus
Copy link

awerlogus commented Jul 15, 2020

@RyanCavanaugh I upgraded my TypeScript version to dev.20200715 but vscode still shows me an error: Unknown compiler option 'pedanticIndexSignatures'. And the feature doesn't work too.

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Jul 15, 2020

@awerlogus This isn't in a dev build. You have to install the tgz the bot produced.

@awerlogus
Copy link

awerlogus commented Jul 15, 2020

@RyanCavanaugh It works now. Thanks.

@awerlogus
Copy link

awerlogus commented Jul 15, 2020

@RyanCavanaugh It looks very suspicious:

declare function test<T extends Record<string, string>>(listener: (obj: T) => string): void

// var a: string | undefined
// Type 'string | undefined' is not assignable to type 'string'.
// Type 'undefined' is not assignable to type 'string'.ts(2322)
test<{ a: string }>(({ a }) => a)

On hover the test function call:

// function test<{
//   a: string;
// }>(listener: (obj: {
//   a: string;
// }) => string): void

The call without of parameter destructuring works fine:

// No errors
test<{ a: string }>((data) => data.a)

@Retsam
Copy link

Retsam commented Jul 17, 2020

Would it be reasonable to consider splitting these two changes into separate flags/PRs, one for the array index issue and one for the object indexing?

I like both changes, but personally, I find the object access issue to be a minor issue that can be worked around pretty easily just by changing how objects are declared. (i.e. Partial<Record<string, Type>> instead of Record<string, Type>) But I find the array issue to be a much more detrimental and without a reasonable workaround.

I'd argue that the array change, on its own, is probably reasonable for a --strict* flag. I do feel like this behavior for arrays should probably be the strict recommended default, and I think there'll be issues to solve, like any of the strict flags, but probably fewer than trying to turn on strictNullChecks for the first time.

Whereas, I do think the object changes may cause more pain for less gain, and so maybe that option belongs behind a --pedantic flag.


In any case, I really hope this goes in in some form or another.

I wrote an eslint rule, no-unnecessary-conditions, which errors on conditions that are always true or always false, based on the types.

But it's always running into issues, due to this behavior of arrays. Code like if(arr[0]) always looks like an "unnecessary condition" based on the types. Example typescript-eslint/typescript-eslint#1798, typescript-eslint/typescript-eslint#1544, typescript-eslint/typescript-eslint#2255.

@InExtremaRes
Copy link

InExtremaRes commented Jul 17, 2020

@Retsam

I like both changes, but personally, I find the object access issue to be a minor issue that can be worked around pretty easily just by changing how objects are declared. (i.e. Partial<Record<string, Type>> instead of Record<string, Type>)

The problem then is that something like Object.values(record) will also return | undefined as a value:

declare const record: Partial<Record<string, Type>>;
const values = Object.values(record);
// typeof values -> (Type | undefined)[]

So the "workaround" only works to express the type of an object which values can be set/defined to undefined, but cannot express the type of an object in which any key may be missing (so get can give undefined) but the ones already set are never undefined.

On the other hand, for me, the problem with arrays are far less problematic since I almost never use sparse arrays and almost always just iterate through all its elements (for-of, .map, etc), so I don't think split this work is beneficial.

@Retsam
Copy link

Retsam commented Jul 17, 2020

@InExtremaRes

declare const record: Partial<Record<string, Type>>;
const values = Object.values(record);
// typeof values -> (Type | undefined)[]

FWIW, I'd consider this behavior to be largely correct, as Object.values will produce an array containing undefined if any objects are removed via obj[key] = undefined. (Which I find is fairly common, instead of delete obj[key])

And, even in the case where it's not necessary (e.g. values are never removed, or always removed with delete), at least this workaround errs on the side of too conservative. It's not a perfect workaround, but it's pretty good.

And, yeah, random access into arrays is less common than iteration over them, which works well currently, but random access into arrays isn't a pattern I'd consider to be uncommon, either. And it'd be nice if they weren't inherently and unavoidably type unsafe.

But I don't know if it matters which half of the PR is more important: if there's disagreement on that point, that's arguably more of a reason to split the rule not less.

@InExtremaRes
Copy link

InExtremaRes commented Jul 17, 2020

FWIW, I'd consider this behavior to be largely correct, as Object.values will produce an array containing undefined if any objects are removed via obj[key] = undefined. (Which I find is fairly common, instead of delete obj[key])

And, even in the case where it's not necessary (e.g. values are never removed, or always removed with delete), at least this workaround errs on the side of too conservative. It's not a perfect workaround, but it's pretty good.

But that is exactly one of the points that can be solved with this PR. With "pedanticIndexSignatures": true:

declare const record: Record<string, Type>;
const values = Object.values(record);
// typeof values -> Type[]

const t1 = record.somePropertyThatMaybeDontExists;
// typeof t1 -> Type | undefined

record.stupidProperty = undefined; // Error: `undefined` is not assignable to `Type`.

Quoting the PR description:

Writes to obj[index] and obj.prop forms retain their normal behavior.

So the type Record<string, Type> cannot have a property set to undefined (for every mutation that TypeScript can see, of course).

@Retsam
Copy link

Retsam commented Jul 17, 2020

I understand that. I'm not confused about what this PR does, and I'm not disagreeing that the PR's behavior is better than the existing workaround.

I'm just saying, there is an existing workaround, and it's pretty good. For the array case, there is no workaround. There is (AFAIK) no way to get TS to prevent errors from accidentally accessing arrays out-of-bounds.

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Jul 17, 2020

Interesting feedback. I'd still be interested in seeing diffs or hearing about how this played out in practice in your projects.

Regarding arrays vs maps, my impression was actually the opposite. Iterating an array correctly with a C-style loop is very easy to get right, so adding all the ceremony of postfix ! in all your array access code for the sake of finding the zero or one places where you do index math seems unworthwhile.

Conversely, if you have a map and some key you're indexing it with, where did that key come from? There aren't many answers that are clearly correct, especially compared to i < arr.length's certainty. That seems like a place where I'm much less commonly confident that I'm not doing an incorrect lookup.

I will just reiterate that we're looking for feedback on having tried this out in practice - discussion about whether it should just be the default behavior, etc, is already extensively discussed in the linked issue, and should continue there if needed.

馃槥 arr[i]
馃檪馃憠 arr[i]!

@awerlogus
Copy link

awerlogus commented Jul 17, 2020

Do you plan to fix parameter destructuring?
#39560 (comment)

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Jul 17, 2020

@awerlogus I think I would fix that bug if and only if we ship this feature. Is this blocking you from trying it out?

@awerlogus
Copy link

awerlogus commented Jul 17, 2020

Not so much. I'm just used to looking for bugs one by one.

@awerlogus
Copy link

awerlogus commented Jul 17, 2020

const a = [1, 2] as const

// Got: 1 | undefined
// Expected: 1
const b = a[0]

I think indexing tuples should be not affected by this feature.

@glasser
Copy link

glasser commented Apr 2, 2021

The example of using Object.entries in the PR description (which is referenced from the TS 4.1 release notes) seems wrong 鈥 I think this should be using arr.entries(), and the order of el and i are reversed?

glasser added a commit to apollographql/federation that referenced this pull request Apr 8, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at
least non-ideal error-handling cases) that arose from TypeScript's default
behavior of assuming that all index operations yield valid values. That is, by
default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y`
rather than `Y | undefined`, and it assumes that all array access attempts are
in bounds.

noUncheckedIndexedAccess changes this so that you generally need to actually
make sure that your indexes worked. (This is a bit awkward for arrays because it
doesn't let you rely on `.length` checks, but it's not the end of the world.)
More details on the flag are available at
microsoft/TypeScript#39560

Here's an overview of the sorts of changes involved:

- One of the issues tracked in #624 is that buildComposedSchema could fail with
  unclear errors if the input schema doesn't declare directives correctly; the
  code assumed that any `@join__owner` or `@join__type` directive application
  *must* have a non-null `graph` argument whose value matches one of the
  `join__Graph` enums. We probably should validate the definitions, but in the
  meantime this PR validates that the value we get out of the directive
  application is good, with explicit errors if not.
- Our code uses a lot of record types like `{[x: string]:
  Y}`. noUncheckedIndexedAccess means we need to actually check that the value
  exists. (Another alternative would be to change to `Map` which does not have
  this issue.) I added a helper `getOrSetRecord` which allowed us to express a
  very common case more succinctly, which also led to less duplication of code.
- In some cases where it is very clear from context that a lookup must
  succeed (eg, a length check), I just used `!`.
- Some cases in composition explicitly checked to see if directives had
  arguments but not if it had at least one argument; adding a `?.[0]` helped
  here.
- Many cases were fixed by just using `?.` syntax or saving a lookup in a
  variable before moving on.
- Iteration over `Object.keys` could be replaced by `Object.values` or
  `Object.entries` to eliminate the index operation.
- In some cases we could change the declaration of array types to be "non-empty
  array" declarations, which look like `[A, ...A[]]`. For example, the groupBy
  operation always makes the values of the returned map be non-empty arrays. I
  was also able to replace a bunch of the FieldSet types in the query planner
  with a NonemptyFieldSet type; there are other places where it might be good to
  improve our checks for nonempty FieldSets though.
- Nested `function`s that close over values in their surrounding scope can't
  rely on narrowing of those values, but arrow functions assigned to a `const`
  that happen after the narrowing can, so in some cases I replaced nested
  functions with arrow functions (which generally involved moving it around
  too).
glasser added a commit to apollographql/federation that referenced this pull request Apr 9, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at
least non-ideal error-handling cases) that arose from TypeScript's default
behavior of assuming that all index operations yield valid values. That is, by
default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y`
rather than `Y | undefined`, and it assumes that all array access attempts are
in bounds.

noUncheckedIndexedAccess changes this so that you generally need to actually
make sure that your indexes worked. (This is a bit awkward for arrays because it
doesn't let you rely on `.length` checks, but it's not the end of the world.)
More details on the flag are available at
microsoft/TypeScript#39560

Here's an overview of the sorts of changes involved:

- One of the issues tracked in #624 is that buildComposedSchema could fail with
  unclear errors if the input schema doesn't declare directives correctly; the
  code assumed that any `@join__owner` or `@join__type` directive application
  *must* have a non-null `graph` argument whose value matches one of the
  `join__Graph` enums. We probably should validate the definitions, but in the
  meantime this PR validates that the value we get out of the directive
  application is good, with explicit errors if not.
- Our code uses a lot of record types like `{[x: string]:
  Y}`. noUncheckedIndexedAccess means we need to actually check that the value
  exists. (Another alternative would be to change to `Map` which does not have
  this issue.) I added a helper `getOrSetRecord` which allowed us to express a
  very common case more succinctly, which also led to less duplication of code.
- In some cases where it is very clear from context that a lookup must
  succeed (eg, a length check), I just used `!`.
- Some cases in composition explicitly checked to see if directives had
  arguments but not if it had at least one argument; adding a `?.[0]` helped
  here.
- Many cases were fixed by just using `?.` syntax or saving a lookup in a
  variable before moving on.
- Iteration over `Object.keys` could be replaced by `Object.values` or
  `Object.entries` to eliminate the index operation.
- In some cases we could change the declaration of array types to be "non-empty
  array" declarations, which look like `[A, ...A[]]`. For example, the groupBy
  operation always makes the values of the returned map be non-empty arrays. I
  was also able to replace a bunch of the FieldSet types in the query planner
  with a NonemptyFieldSet type; there are other places where it might be good to
  improve our checks for nonempty FieldSets though.
- Nested `function`s that close over values in their surrounding scope can't
  rely on narrowing of those values, but arrow functions assigned to a `const`
  that happen after the narrowing can, so in some cases I replaced nested
  functions with arrow functions (which generally involved moving it around
  too).
glasser added a commit to apollographql/federation that referenced this pull request Apr 16, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at
least non-ideal error-handling cases) that arose from TypeScript's default
behavior of assuming that all index operations yield valid values. That is, by
default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y`
rather than `Y | undefined`, and it assumes that all array access attempts are
in bounds.

noUncheckedIndexedAccess changes this so that you generally need to actually
make sure that your indexes worked. (This is a bit awkward for arrays because it
doesn't let you rely on `.length` checks, but it's not the end of the world.)
More details on the flag are available at
microsoft/TypeScript#39560

Here's an overview of the sorts of changes involved:

- One of the issues tracked in #624 is that buildComposedSchema could fail with
  unclear errors if the input schema doesn't declare directives correctly; the
  code assumed that any `@join__owner` or `@join__type` directive application
  *must* have a non-null `graph` argument whose value matches one of the
  `join__Graph` enums. We probably should validate the definitions, but in the
  meantime this PR validates that the value we get out of the directive
  application is good, with explicit errors if not.
- Our code uses a lot of record types like `{[x: string]:
  Y}`. noUncheckedIndexedAccess means we need to actually check that the value
  exists. (Another alternative would be to change to `Map` which does not have
  this issue.) I added a helper `getOrSetRecord` which allowed us to express a
  very common case more succinctly, which also led to less duplication of code.
- In some cases where it is very clear from context that a lookup must
  succeed (eg, a length check), I just used `!`.
- Some cases in composition explicitly checked to see if directives had
  arguments but not if it had at least one argument; adding a `?.[0]` helped
  here.
- Many cases were fixed by just using `?.` syntax or saving a lookup in a
  variable before moving on.
- Iteration over `Object.keys` could be replaced by `Object.values` or
  `Object.entries` to eliminate the index operation.
- In some cases we could change the declaration of array types to be "non-empty
  array" declarations, which look like `[A, ...A[]]`. For example, the groupBy
  operation always makes the values of the returned map be non-empty arrays. I
  was also able to replace a bunch of the FieldSet types in the query planner
  with a NonemptyFieldSet type; there are other places where it might be good to
  improve our checks for nonempty FieldSets though.
- Nested `function`s that close over values in their surrounding scope can't
  rely on narrowing of those values, but arrow functions assigned to a `const`
  that happen after the narrowing can, so in some cases I replaced nested
  functions with arrow functions (which generally involved moving it around
  too).
glasser added a commit to apollographql/federation that referenced this pull request Apr 16, 2021
When reviewing some code in this repo, I noticed some potential bugs (or at
least non-ideal error-handling cases) that arose from TypeScript's default
behavior of assuming that all index operations yield valid values. That is, by
default TS assumes that if `x: Record<string, Y>` then `x[anyString]` is `Y`
rather than `Y | undefined`, and it assumes that all array access attempts are
in bounds.

noUncheckedIndexedAccess changes this so that you generally need to actually
make sure that your indexes worked. (This is a bit awkward for arrays because it
doesn't let you rely on `.length` checks, but it's not the end of the world.)
More details on the flag are available at
microsoft/TypeScript#39560

Here's an overview of the sorts of changes involved:

- One of the issues tracked in #624 is that buildComposedSchema could fail with
  unclear errors if the input schema doesn't declare directives correctly; the
  code assumed that any `@join__owner` or `@join__type` directive application
  *must* have a non-null `graph` argument whose value matches one of the
  `join__Graph` enums. We probably should validate the definitions, but in the
  meantime this PR validates that the value we get out of the directive
  application is good, with explicit errors if not.
- Our code uses a lot of record types like `{[x: string]:
  Y}`. noUncheckedIndexedAccess means we need to actually check that the value
  exists. (Another alternative would be to change to `Map` which does not have
  this issue.) I added a helper `getOrSetRecord` which allowed us to express a
  very common case more succinctly, which also led to less duplication of code.
- In some cases where it is very clear from context that a lookup must
  succeed (eg, a length check), I just used `!`.
- Some cases in composition explicitly checked to see if directives had
  arguments but not if it had at least one argument; adding a `?.[0]` helped
  here.
- Many cases were fixed by just using `?.` syntax or saving a lookup in a
  variable before moving on.
- Iteration over `Object.keys` could be replaced by `Object.values` or
  `Object.entries` to eliminate the index operation.
- In some cases we could change the declaration of array types to be "non-empty
  array" declarations, which look like `[A, ...A[]]`. For example, the groupBy
  operation always makes the values of the returned map be non-empty arrays. I
  was also able to replace a bunch of the FieldSet types in the query planner
  with a NonemptyFieldSet type; there are other places where it might be good to
  improve our checks for nonempty FieldSets though.
- Nested `function`s that close over values in their surrounding scope can't
  rely on narrowing of those values, but arrow functions assigned to a `const`
  that happen after the narrowing can, so in some cases I replaced nested
  functions with arrow functions (which generally involved moving it around
  too).
@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Jun 23, 2022

The fact that even the following still prints an error makes this unusable IMO. Indexed access should at least respect the narrowing provided by the in operator:

function foo(obj: Record<string, number>, key: string) {
  let value: number = 0;
  if (key in obj) value = obj[key];
}

Sure I can write obj[key]! but then I won't get a warning if I change the signature of obj to something that allows null or undefined values (which is more likely to happen in real code than in my contrived example).

It's a pity because the option did help me find some cases of potentially unbounded or at least not clearly bounded array access that I was able to refactor, but it's not worth wading through all the false positives.

@papb
Copy link

papb commented Aug 4, 2022

@ehoogeveen-medweb I think you should file a new issue reporting your example as a bug. It is clearly a bug to me.

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Aug 4, 2022

How it started:

Given current limitations in CFA we don't have any good ideas on how to fix, we remain guardedly skeptical this feature will be usable in practice.

How it's going:

The fact that even the following still prints an error makes this unusable IMO.

馃槄

@RyanCavanaugh
Copy link
Member Author

RyanCavanaugh commented Aug 4, 2022

For real though, the point of the flag is to be as conservative as possible and CFA doesn't track where possible out-of-band mutations occur. If you had written

function foo(obj: Record<string, number>, key: string) {
  let value: number = 0;
  if (key in obj) {
    mut();
    value = obj[key];
  }

  function mut() { key += "oops" }
}

then the code is wrong.

@nicu-chiciuc
Copy link

nicu-chiciuc commented Aug 4, 2022

@ehoogeveen-medweb
here's a fun way to do it: Playground link

function isIn<Key extends string,T, Obj extends Record<string, T>>
    (key: Key, obj: Obj): obj is Obj & Record<Key, T> {
    return key in obj;
}

function foo<TString extends string>(obj: Record<string, number>, key: TString) {
  let value: number = 0;

  //              鈱-- Error here
  if (key in obj) value = obj[key];

  //                  No error
  if (isIn(key, obj)) value = obj[key];

  return value;
}

function foo2<TString extends string>(obj: Record<string, number>, key: TString) {
  let value: number = 0;

  if (key in obj) {
    mut();
    // 鈱-- Error
    value = obj[key];
  }

  if (isIn(key, obj)) {
    mut();
    // No error, but we also can't mutate `key`, so we're safe
    value = obj[key];
  }

  //               鈱-- Type 'string' is not assignable to type 'TString'.
  function mut() { key += "oops" }

  return value;
}
}

@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Aug 4, 2022

For real though, the point of the flag is to be as conservative as possible and CFA doesn't track where possible out-of-band mutations occur.

I don't know anything about how the analysis works, but if it could be taught to distinguish the binary "nothing happened between the check and the access" from "something happened between the check and the access" I think that would already be good enough. Even if

  if (key in obj) {
    value = obj[key];
  }

worked and

  if (key in obj) {
    "do nothing";
    value = obj[key];
  }

didn't, I think it would already make a big difference to the usability of this option. I could be mistaken of course.

@thw0rted
Copy link

thw0rted commented Aug 5, 2022

Ryan's counterexample is kind of a red herring though, because this also does not work:

function foo(obj: Record<string, number>, key: string) {
  const kk = key;
  let value: number = 0;
  if (kk in obj) {
    value = obj[kk];
  }
}

This is the same pattern I use for narrowing in arrow functions, like if (isKey(obj, kk)) { arr.forEach(x => x.doStuff(obj[kk])) }. TS knows to preserve narrowing across scopes because it's a const.

(As an aside: sure would be nice to allow readonly parameters so I wouldn't have to make a const duplicate/alias just for cases like this.)

@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Aug 23, 2022

I had another crack this using the helper function in #39560 (comment) as a starting point and eventually managed to make it work.

I ended up using the following helper functions. They're less safe than the original helper function because they don't guard against mutating the key, but they also don't require the calling function to be generic:

// A declaration in a d.ts file is also enough.
const __detail: unique symbol = Symbol();
type RecordWithProp<V> = { [__detail]: V };

// Used instead of `key in obj`.
function objectHasProp<K extends number | string | symbol, V>(
  obj: Record<K, V>,
  key: K,
): obj is Record<K, V> & RecordWithProp<V> {
  return key in obj;
}

// Used instead of `obj[key]`.
function objectGetProp<K extends number | string | symbol, V>(obj: Record<K, V> & RecordWithProp<V>, key: K) {
  return obj[key] as V;
}

// Checks whether the array contains at least the given number of elements.
type ArrayAtleast<T, n extends number, U extends T[] = []> =
  // Limit the instantiation depth to 10; we don't need it to be high.
  U['length'] extends n | 10 ? [...U, ...T[]] : ArrayAtleast<T, n, [T, ...U]>;
function arrayAtleast<T, N extends number>(array: T[], atleastLength: N): array is ArrayAtleast<T, N> {
  return array.length >= atleastLength;
}

The main annoyance that I ran into: Because array types don't have any range information associated with them, you end up having to check or assert trivial array accesses like 'foo|bar'.split('|')[0].

I used arrayAtleast above to check some arrays, but of course the downside is that anything that modifies the length of an array in-place (like Array.prototype.pop) can make the type information invalid. The object access helpers have a similar issue (once objectHasProp says that an object contains a key, as far as objectGetProp is concerned it then contains any key).

So to make this check more ergonomic (and not require these unsafe helper functions), I think TypeScript would have to:

  1. Track range information for arrays (e.g. "contains at least 1 element" or "at most 10 elements"), informed by Array.prototype.length checks among other things.
  2. Allow narrowing the keys of an object with the value contained in a binding (e.g. keyof myObj == string & valueof myVar).

But those both sound like big and tricky features to add to the language.

Edit: Replaced ts-toolbelt's L.Repeat with a custom version that adds the array type itself at the end. With the intersection U = [T] & T[], U['length'] === 1 - but with U = [T ...T[]], the length stays variable (while still working with --noUncheckedIndexedAccess).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet