Skip to content

Inferred type union allowed to assign to an uncomparable type #33657

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

Closed
ducin opened this issue Sep 28, 2019 · 11 comments
Closed

Inferred type union allowed to assign to an uncomparable type #33657

ducin opened this issue Sep 28, 2019 · 11 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@ducin
Copy link

ducin commented Sep 28, 2019

TypeScript Version: 3.6.3

Search Terms: "as keyword", "union types"

Code

type Employee = {
    salary: number
}

const employees = [
    { salary: 3456 },
    { salar_y: 3456 }, // this line should fail, it doesn't
    { salary: 3456 },
] as Employee[]

const employees = [
    { salar_y: 3456 }, // however, this line fails, as expected
] as Employee[]

Expected behavior:

Error thrown: Type '{ salar_y: number; }' is not comparable to type 'Employee'. It should fail because { salar_y: 3456 } is incomparable to { salary: number } And it does fail for var e = { salar_y: 3456 } as Employee

Actual behavior:

Compilation passes.

Playground Link

Any usecase where this is harmful? Consuming this hardcoded array injects undefined, where a number was expected, resulting in a NaN in runtime. I'd expect TypeScript not to allow that.

@jack-williams
Copy link
Collaborator

You should not be using casts here; you get the correct behaviour if you use a type annotation instead:

const employees2: Employee[] = [
    { salary: 3456 },
    { salar_y: 3456 }, // error
];

const employees3: Employee[] = [
    { salar_y: 3456 }, // error
    { salary: 3456 },
];

const employees4: Employee[] = [
    { salary: 3456 },
    { salar_y: 3456 }, // error
    { salary: 3456 },
];

@MartinJohns
Copy link
Contributor

Casts, or type assertions, are a way to tell the compiler: "Sshhh.. trust me.. I am right..". But you weren't.

@ducin
Copy link
Author

ducin commented Sep 28, 2019

Yes, I know. : and as have different semantics and serve different purposes. And the behavior of as in this case seems just invalid. That is, 2 types are comparable (A as B) if A is a subtype of B or B is subtype of A - here it's neither of them. Could we please concentrate on the behavior and whether the team wants to fix, if it is invalid, instead of discussing about what you consider good and bad practices?

@AnyhowStep
Copy link
Contributor

They don't have to be subtypes. Just have properties that sufficiently overlap, in general.

@jcalz
Copy link
Contributor

jcalz commented Sep 29, 2019

This is working as intended. Type assertions are allowed to narrow values to subtypes, which is what's happening here. Even when A and B are not comparable, Array<A> is a subtype of Array<A | B>, right? Observe:

type Employee = {
  salary: number;
};

type Employe_e = {
  salar_y: number;
};

let justEmploye_es = [{ salar_y: 3456 }]; // Array<Employe_e>
let justEmployees = [{ salary: 3456 }]; // Array<Employee>
let mixedBag = [{ salary: 3456 }, { salar_y: 3456 }]; // Array<Employee | Employe_e>;

let mixedBagIsASupertypeOfBoth: typeof mixedBag;
mixedBagIsASupertypeOfBoth = justEmploye_es; // okay
mixedBagIsASupertypeOfBoth = justEmployees; // okay

let employees = mixedBag as Array<Employee>; // legitimate narrowing, no error
let notEmployees = justEmploye_es as Array<Employee>; // neither widening nor narrowing, error

Link to code

I welcome all your various hand emojis. Yes, even that one. 👌✌🤞🖐☝👇👈👉🖖🤘

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 29, 2019

Array<A> is a subtype of Array<A | B>, right?

It is, at the moment... But should it? =P

const a : string[] = ["hi"];
const b : (string|number)[] = a;
b.push(123);
console.log(a[1]); //not a string

/off-topic

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 29, 2019
@RyanCavanaugh
Copy link
Member

@jcalz is correct. There has to be some line between "all assertions are allowed" and "the only allowed assertions are ones you don't need in the first place".

In spirit this is similar to #19541, but changing the behavior to the proposed one would introduce another inconsistency between an array literal and an alias to that same literal.

I would be curious about the original scenario (or if this was a "I noticed something" bug); in practice downcasts of literals should be exceptionally rare. Whatever motivated you to write that in the first place is something of interest.

@ducin
Copy link
Author

ducin commented Sep 29, 2019

@jcalz thanks for the explanation! Didn't see that 😶it is a subtype, but I do find that confusing.

@RyanCavanaugh sure I understand. The usecase is: I'm trying to find a simple way to fake data for unit tests for data processing. I've got objects with lots of properties (20+) that define the original Employee type. The functions that I need, obviously, are annotated to accept Employee type objects. Now, one solution is to provide lots of fake data with all those 20+ properties but it's just not very readable if you're using 1 or at most 2 properties in a single scenario; introduces other problems too. So I wanted to define only the property that I need, but in a way that checks if I'm not passing typos, just as here. I was searching for a lightweight solution (e.g. typemoq is not really lightweight).

One attempt was to do const fake = <T>(data: Partial<T>) => T { return T }. It works fine with literals, but for for arrays I need signature overrides. So type assertion like "shh, trust me that's the type" was what I actually wanted for the tests, as long as typos would get caught. And I don't want const fake = <T>(data: any) => T { return T } because that would accept everything (any, too broad).

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 29, 2019

type Employee = {
    salary: number,
    propWeDoNotCareAbout : string,
}

function mockEmployees<K extends keyof Employee>(
    arr : Pick<Employee, K>[]
) : Employee[] {
    /**
     * This is an unsafe cast but should serve the use case
     * well enough.
     */
    return arr as Employee[];
}

const employees : Employee[] = mockEmployees<"salary">([
    { salary: 3456 },
    { salar_y: 3456 }, // this line should fail, it fails
    { salary: 3456 },
]);

Playground


The functions that I need, obviously, are annotated to accept Employee type objects.

You should change those functions to accept Pick<Employee, KeysFooCaresAbout> instead of just Employee, then.

Instead of,

function calculateMonthlySalary (e : Employee) {
  return e.dailySalary * 20;
}

Use,

function calculateMonthlySalary (e : Pick<Employee, "dailySalary">) {
  return e.dailySalary * 20;
}

@jack-williams
Copy link
Collaborator

jack-williams commented Sep 29, 2019

Looks to me like you want to create exact subsets of Employee. @AnyhowStep’s approach is probably what you want, using type annotations to exploit excess property checking. This issue goes into even more detail on exact types: #12936.

For the sake of posterity: if you had put the use-case in the original example you would have received more useful feedback immediately.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

7 participants