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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

probable bug in typeof type guard #10442

Open
opensrcken opened this issue Aug 19, 2016 · 10 comments
Open

probable bug in typeof type guard #10442

opensrcken opened this issue Aug 19, 2016 · 10 comments
Labels
Bug A bug in TypeScript
Milestone

Comments

@opensrcken
Copy link

opensrcken commented Aug 19, 2016

TypeScript Version: 1.8.0

Code

const numbers: {[key: string]: number} = {a: 1, b: 2};

const mixed: {[key: string]: number|string} = {a: 1, b: 'two'};

function test(something: {[key: string]: number|string}) {
  Object.keys(something).forEach(k => {
        if (typeof something[k] === 'number') {
            Math.floor(something[k]);
        } else {
            something[k].toLowerCase();
        }
  });
}

test(numbers);

Link: https://www.typescriptlang.org/play/#src=const%20numbers%3A%20%7B%5Bkey%3A%20string%5D%3A%20number%7D%20%3D%20%7Ba%3A%201%2C%20b%3A%202%7D%3B%0D%0A%0D%0Aconst%20mixed%3A%20%7B%5Bkey%3A%20string%5D%3A%20number%7Cstring%7D%20%3D%20%7Ba%3A%201%2C%20b%3A%20'two'%7D%3B%0D%0A%0D%0Afunction%20test(something%3A%20%7B%5Bkey%3A%20string%5D%3A%20number%7Cstring%7D)%20%7B%0D%0A%20%20Object.keys(something).forEach(k%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20if%20(typeof%20something%5Bk%5D%20%3D%3D%3D%20'number')%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20Math.floor(something%5Bk%5D)%3B%0D%0A%20%20%20%20%20%20%20%20%7D%20else%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20something%5Bk%5D.toLowerCase()%3B%0D%0A%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%7D)%3B%0D%0A%7D%0D%0A%0D%0Atest(numbers)%3B

Expected behavior:
Type guard would apply to something[k].

Actual behavior:
Type guard does not get applied to something[k].

@kitsonk
Copy link
Contributor

kitsonk commented Aug 20, 2016

There are limitations to code analysis for TypeScript, and even in TypeScript 2.0, TypeScript does not narrow indexed based values. To get this to work properly, you would need to put the value into something that can be more easily tracked by TypeScript:

const numbers: {[key: string]: number} = {a: 1, b: 2};

const mixed: {[key: string]: number|string} = {a: 1, b: 'two'};

function test(something: {[key: string]: number|string}) {
  Object.keys(something).forEach(k => {
        const value = something[k];
        if (typeof value === 'number') {
            Math.floor(value);
        } else {
            value.toLowerCase();
        }
  });
}

test(numbers);

Prior to TypeScript 2.0, properties of objects weren't narrowed anyways. The TypeScript team though might consider it a suggestion to enhance the code flow analysis in TypeScript to track indexed property values where the static analysis can determine the index value will not change within the scope the narrowing occurs (like in this example).

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Aug 22, 2016
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 2.1 milestone Aug 22, 2016
@RyanCavanaugh
Copy link
Member

An index expression where the indexer expression is const (or inferred const as k should be here) should be narrowed. This may be fixed by existing work going into 2.0, otherwise we can fix it in 2.1

@jkjustjoshing
Copy link

jkjustjoshing commented Sep 15, 2016

Would this fall under the same category?

let a: number[] | string = 'foo';

if (Array.isArray(a)) {
    a.push(1);
}

let b: { foo: number[] | string } = {};

if (Array.isArray(b.foo)) {
    b.foo.push(1); // Error: Property "push" does not exist on type 'number[] | string'
}

Link:
https://www.typescriptlang.org/play/#src=let%20a%3A%20number%5B%5D%20%7C%20string%20%3D%20'foo'%3B%0D%0A%0D%0Aif%20(Array.isArray(a))%20%7B%0D%0A%09a.push(1)%3B%0D%0A%7D%0D%0A%0D%0Alet%20b%3A%20%7B%20foo%3A%20number%5B%5D%20%7C%20string%20%7D%20%3D%20%7B%7D%3B%0D%0A%0D%0Aif%20(Array.isArray(b.foo))%20%7B%0D%0A%09b.foo.push(1)%3B%0D%0A%7D

@mhegazy
Copy link
Contributor

mhegazy commented Sep 15, 2016

@jkjustjoshing this should be working as expected in TS 2.0. The playground is still targeting TS 1.8

@swashata
Copy link

swashata commented Oct 2, 2018

Hello,

I am new to typescript and maybe I got hit by this? I was writing something like this.

playground

interface Entry {
	[x: string]: string[] | string;
}
interface NormalizedEntry {
	[x: string]: string[];
}
interface File {
	name: string;
	entry: Entry;
}

const getEntry: Function = (file: File): NormalizedEntry => {
    const { name, entry } = file;

    const normalizedEntry: NormalizedEntry = {};
	// Loop over all user defined entries and add to the normalizedEntry
	Object.keys(entry).forEach((key: string) => {
		normalizedEntry[key] =
			Array.isArray(entry[key]) ? entry[key] : [entry[key]];
    });

    return normalizedEntry;
}

But I got error

Type 'string | (string | string[])[]' is not assignable to type 'string[]'.
  Type 'string' is not assignable to type 'string[]'.
const normalizedEntry: NormalizedEntry

So I changed to explicit variable as suggested by @kitsonk

playground

interface Entry {
	[x: string]: string[] | string;
}
interface NormalizedEntry {
	[x: string]: string[];
}
interface File {
	name: string;
	entry: Entry;
}

const getEntry: Function = (file: File): NormalizedEntry => {
    const { name, entry } = file;

    const normalizedEntry: NormalizedEntry = {};
	// Loop over all user defined entries and add to the normalizedEntry
    Object.keys(entry).forEach((key: string) => {
        const entryPoint: string[] | string = entry[key];
		normalizedEntry[key] = Array.isArray(entryPoint)
			? entryPoint
			: [entryPoint];
    });

    return normalizedEntry;
}

And now it works fine.

Does it work under the same category or shall I raise a new issue?

@RyanCavanaugh
Copy link
Member

@swashata type guards can't narrow expressions like obj[k] where k is a variable. You need to write something like this:

	Object.keys(entry).forEach((key: string) => {
		const en = entry[key]; 
		normalizedEntry[key] = Array.isArray(en) ? en : [en];
    });

@swashata
Copy link

swashata commented Oct 2, 2018

Thanks @RyanCavanaugh for your response 😄. This is exactly what I did (as you can see in the second part of the code).

Bdw, do you think documenting this behavior somewhere would help newbies like me? If so, anyplace where I can send a PR?

@sandersn sandersn removed their assignment Jan 7, 2020
@Andarist
Copy link
Contributor

This is a known design limitation and not a bug - so I think that at the very least the labels should be changed here (cc @jakebailey ). I also believe that there are already open issues related to this, ones that have more examples/explanations/etc. So it might be worth closing this one in favor of other ones. I can't exactly find those other issues but something tells me that @fatcerberus @MartinJohns and @jcalz might have them on speed dial.

@fatcerberus
Copy link

fatcerberus commented Aug 27, 2023

@Andarist Both Martin and Ryan usually dupe this to #10530, though that's questionable since the case in the OP of that issue (indexed access with a literal string key) actually works now. I'm not aware of another "canonical" issue for this, though.

@Andarist
Copy link
Contributor

Ah yes, this one i have found but skipped it exactly because of that reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants