Skip to content

Unsafe assignment of Record into interface with optionals #40529

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
villasv opened this issue Sep 14, 2020 · 3 comments
Closed

Unsafe assignment of Record into interface with optionals #40529

villasv opened this issue Sep 14, 2020 · 3 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@villasv
Copy link

villasv commented Sep 14, 2020

Search Terms:
typescript unsafe Record unknown assignment interfaces all optional undefined sound type system

Code

I discovered the following code to be legal. It was pretty scary.

interface ObjWithPropFalsy {
    prop?: false;
}

const a: Record<string, boolean> = { prop: true };
const b: ObjWithPropFalsy = a;

if (b.prop) { // b.prop is of type false | undefined, but its value is true
    console.log("typing says it never happens") // <-- will print
} else {
    console.log("typing says it always happens")
}

This is so dangerous for me* that I decided to propose a new eslint rule warning about it, but @bradzacher suggested I brought this issue to the TypeScript team. He also added the following example, which is even more intense because unknown isn't really relevant here:

interface ObjWithPropFalsy {
    prop?: false;
}

const a: Record<string, string> = { prop: 'str' };
const b: ObjWithPropFalsy = a;

type T = (typeof b)['prop']; // === false | undefined;
typeof a.prop === 'string' // runtime

Expected behavior:
Compilation error.

Actual behavior:
Compilation successful.

*(can explain the use-case that led to the discovery if it's relevant)

I understand this might be an inevitable design limitation coming from index signatures, so this might be closer to a feature request than a bug? If this is the case, sorry for the wrong labeling.

@RyanCavanaugh
Copy link
Member

The problem is much worse than you're suggesting 😉

interface OhNo {
    prop?: string;
}

const a: Record<string, boolean> = { prop: true };
const b: OhNo = a;
b.prop; // claimed to be string | undefined, but is boolean

In general index signatures are assumed to not contain every arbitrary key for the purposes of assignability, so prop is assumed to be missing and therefore not a blocker to the assignment. There are a lot of potential unsoundnesses in index signatures and if your primary goal is type safety, not using Record at all would be the correct thing to do.

If you want a feature request for this, it's probably #12936

@villasv
Copy link
Author

villasv commented Sep 14, 2020

not using Record at all would be the correct thing to do

Makes sense. On the other hand, I've been using Record primarily because I've been warned that Object, {} and object should be avoided as well. So what do you recommend on its place?

Also, would you agree that the lint rule I proposed (assigning index signatures to interfaces with all optionals) makes sense then? To make developers aware of the type unsoundness that it may introduce.

If you want a feature request for this

You mean a feature request that would fix index signatures to behave more safely? I read the Exact Types feature request but I don't see the immediate connection.

@RyanCavanaugh
Copy link
Member

I try not to comment on lint settings; people have opinions and that's OK. That said, the "don't use object" rule is actively harmful and is bad advice IMO. Exactly which type you should use depends on the situation and I don't see enough information here to be more specific than that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

2 participants