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

Index signature is assignable to weak type whose properties don't match the signature type #27144

Open
mattmccutchen opened this issue Sep 17, 2018 · 10 comments
Labels
Bug A bug in TypeScript
Milestone

Comments

@mattmccutchen
Copy link
Contributor

From https://stackoverflow.com/q/52368008 . Based on #16047 (comment), it appears that @sandersn may have thought about the rule, but it still makes no sense to me and no rationale is stated.

TypeScript Version: master (394ee31)

Search Terms: weak type index signature

Code

interface Foo {
    a?: string;
}
interface Bar {
    [n: string]: number;
}
declare let b: Bar;
// No error; expected "Type 'Bar' has no properties in common with type 'Foo'."
let a: Foo = b;

Expected behavior: Error: "Type 'Bar' has no properties in common with type 'Foo'."

Actual behavior: No error.

Playground Link: link

Related Issues: Possibly #9900

@sandersn
Copy link
Member

I think of index signatures as asserting that a type may contain every property, in which case the weak type check is trivially satisfied. But if you think of them as having no defined properties, similar to {}, you would still hit the exception for empty source types that is part of the weak type check. That is, an empty object type is always assignable to a weak type, in order to enable the pattern let options: Weak = {}.

@mattmccutchen
Copy link
Contributor Author

mattmccutchen commented Sep 17, 2018

Thanks for the prompt response!

I think of index signatures as asserting that a type may contain every property, in which case the weak type check is trivially satisfied.

That interpretation of an index signature doesn't seem consistent with the fact that TypeScript doesn't check that the return type of the index signature is compatible with the type of a. I know that the weak type check itself is never concerned with property types, but the way that the weak type check interprets property names vs. index signatures should be consistent with the way that property type compatibility does.

More practically, we've just heard from a user who mistakenly assigned a type with only an index signature to a weak type. Are there reasonable cases in which such an assignment is intentional?

@sandersn sandersn added Bug A bug in TypeScript In Discussion Not yet reached consensus labels Sep 17, 2018
@sandersn
Copy link
Member

Ah, I didn't see the types. :) I have the weak type implementation so much in my head that I just looked at properties.

So it sounds like the correct check for index signatures is to make sure that the signature type is assignable to the union of the weak type's property types.

@sandersn sandersn changed the title Type with only index signature is assignable to weak type Index signature is assignable to weak type whose properties don't match the signature type Sep 17, 2018
@sandersn
Copy link
Member

sandersn commented Sep 17, 2018

Well, here's what it looks like, inside the main weak type check:

const index = getIndexInfoOfType(source, IndexKind.String);
if (index && !isRelatedTo(getIntersectionType([index.type, ...map(getPropertiesOfType(source), getTypeOfSymbol)]), getUnionType(map(getPropertiesOfType(target), getTypeOfSymbol)))) {
    if (reportErrors) {
	// NB Should have a specific error here
	reportError(Diagnostics.Type_0_has_no_properties_in_common_with_type_1, typeToString(source), typeToString(target));
    }
    return Ternary.False;
}

This is a related check to the current weak type check, though. It's not the same thing. It says that even though there is a value that satisfies both types, that value is {}, which is not a useful value. Instead, we assume that both types have a value with at least one property, in which case no value could satisfy both types.

@DanielRosenwasser what do you think about this check? I need to run it on the user tests to see if it finds a bunch of errors, but nothing has turned up in suite 0 tests.

Edit: Doesn't find any errors in the user test suite, although it's mostly Javascript, so not too many weak types there.

Edit: Nope, nothing in RWC either.

@mattmccutchen
Copy link
Contributor Author

I didn't know a "Bug" could be "In Discussion". 😄

For that matter, if we interpret an index signature as "there might exist a property of any name", why do we ever allow a type with an index signature to be assignable to a type with an incompatible optional property, regardless of whether the target type is weak? Are there real-world cases in which the error would be undesirable? Example:

interface Foo {
    a: number;
    b?: string;
}
interface Bar {
    a: number
    [n: string]: number;
}
let b: Bar = {a: 42, b: 43};
let f: Foo = b;  // OK; expect error?

@sandersn sandersn removed the In Discussion Not yet reached consensus label Sep 17, 2018
@sandersn sandersn added this to the Future milestone Sep 17, 2018
@sandersn
Copy link
Member

@mattmccutchen I think you have found the real issue. I'm not sure why it works this why. Maybe the spec has a justification?

mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Oct 7, 2018
against optional properties of the target.

Fix one good break in the compiler itself.

Fixes microsoft#27144.
@mattmccutchen
Copy link
Contributor Author

I wanted to see how many TypeScript PRs I could submit in a day, so I went ahead and submitted #27591 to check a source index signature (or rest type if the source is a tuple type) against target optional properties. There was one good (IMO) break in the compiler itself. I guess you can see what happens with the "real-world code" suite.

@jcalz
Copy link
Contributor

jcalz commented May 16, 2022

Relevant SO question where someone smacks into this.

@umenosuke
Copy link

I think this issue could cause potential bugs in the code.
Why has this PR been closed?

If the PR was closed because existing code with potential issues fails to transpile,

Would it be possible to consider implementing individual checks through options like StrictXXX?
What are your thoughts on this?

@umenosuke
Copy link

umenosuke commented Nov 20, 2024

By the way, I had trouble with this

Playground Link: link

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
4 participants