Dangerous calls allowed by TS: class generic type, when unknown, should be treated as never in the class methods/callback parameters

🔎 Search Terms

unknown never generic type methods

🕗 Version & Regression Information

This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________

⏯ Playground Link

💻 Code

// allows dangerous calls
{
    class X<T> {
        callback(t: T) {}
    }

    const xb = new X<boolean>();

	const x: X<unknown> = xb; // ok
	x.callback(34); // dangerous.
}
// can't assign if callback
{
    class X<T> {
		callback!: (t: T) => void
	}

    const xb = new X<boolean>();
	const x: X<unknown> = xb; // nok
	x.callback(34); // dangerous
}
// proposal
{
    class X<T> {
        callback(t: T extends unknown ? never : T) {}
    }

    const xb = new X<boolean>();

	const x: X<unknown> = xb; // ok
	x.callback(34); // error (good)
}

🙁 Actual behavior

When a class generic type is unknown, if used as parameters in a method/callback, will authorize dangerous calls :

(x as X<unknown>).callback(34); // we don't know the generic type of x so we should assume it is unsafe to call this method.

Also, due to that, if the class has a callback, we can't assign X<...> to X<unknown>.

🙂 Expected behavior

TS should assume the method parameter type is never to prevent its call, until the user has asserted the class real type.

// proposal
{
    class X<T> {
        callback(t: T extends unknown ? never : T) {}
    }

    const xb = new X<boolean>();

	const x: X<unknown> = xb; // ok
	x.callback(34); // error (good)
}

Additional information about the issue

This would also enable to use unknown instead of any in some cases :

// use unknown instead of any to not trigger LINT warning.
function foo<T extends Record<string, X<unknown>>>() {

}

Current workaround would be :

function foo<T extends Record<string, X<U>>, U>() {

}

This is working as intended, because method parameters are bivariant. That's why the "callback" version does result in an error. You can find more information here: https://github.com/microsoft/TypeScript/wiki/FAQ#why-are-function-parameters-bivariant

It's essentially the same issue as this:

class Foo<T> {
  callback(t: T) {}
}

const f = new Foo<1>();
const x: Foo<number> = f;

x.callback(2); // Can pass 2, even tho on instance it is typed 1.

Search for "bivariant method" and you will find many many issues about this.

This is working as intended, because method parameters are bivariant. That's why the "callback" version does result in an error. You can find more information here: https://github.com/microsoft/TypeScript/wiki/FAQ#why-are-function-parameters-bivariant

Urg, that's so dirty.

Surely, there are better ways to proceed.
EDIT: E.g. only authorizing such affectations when the target doesn't have any method parameters depending on its generic type that modifies variable outside of the method local scope (i.e. doesn't modifies the instance internal state). Otherwise, requiring an explicit cast with as.

For example, requiring the array to be a readonly array[].

On another note, maybe we'd need a way to indicate an unknown generic type parameter as opposed to an unknown type, e.g. :

Array<Unknown> // I don't know its parameters, so I shouldn't be able to call methods/callbacks whose parameters depends on the generic type.
Array<unknown> // An array which elements are of type unknown

fully read-only to prevent any issues (e.g. readonly Animal [] for an array)

Read-only types are implicitly compatible with the writable versions, so this wouldn't work. { readonly v: number } can be passed to { v: number } without issues. The readonly only applies to modifications via that interface. And there's no way to represent immutable types in TypeScript.

On another note, maybe we'd need a way to indicate an unknown generic type parameter as opposed to an unknown type, e.g. :

That would essentially be never.

edit: this issue is well known and has existed for quite a while. If there would be a practical solution the team would surely have implemented it already.

{ readonly v: number } can be passed to { v: number } without issues. The readonly only applies to modifications via that interface.

WTF. Why ???
That's crazy.

At least requiring an explicit cast, doing it implicitly is just dirty, really dirty.

Though,

let a = {} as readonly number[];
let b: number[] = a;

does raise an error, so there are hopes.

Read-only types are implicitly compatible with the writable versions, so this wouldn't work.

After thinking about it, it should be in fact "without methods whose parameters depends on the generic type".

Maybe also having a way to explicitly state we can accept super type of T, to allow things like:

class Base {}
class X extends Base {
    foo(){}
}

a.find( a as Base );

And there's no way to represent immutable types in TypeScript.

Isn't it a design choice ?

There is Readonly<> that do things. The only issue is that implicit cast are allowed... that's just crazy.

edit: this issue is well known and has existed for quite a while. If there would be a practical solution the team would surely have implemented it already.

Well, it is in other languages.
It feels like there were at some point one bad decision in TS design, that locked TS in this state as changing it would break everything.

That would essentially be never.

Nope. Array<Unknown>.pop(); would be unknown, not never. In parameters it'd be never (to prevent calls), and in return values unknown.

With never you also can't write:

function foo<T extends Record<string, never>>(t: T) {
    return t;
}

foo({"ok": 3});

does raise an error, so there are hopes

that’s not because of a readonly modifier anywhere but rather because mutable arrays have more methods in TS (like push, pop, etc). So its about the target side requiring more properties than the source has - not about per property modifiers difference

So its about the target side requiring more properties than the source has - not about per property modifiers difference

Indeed.

Still, I don't understand why TS isn't doing such very basic check.
We can very easily implement our own system, and not much needs to be added to TS to make it cleaner and more generic:

const RW = Symbol("");

class X {
    private [RW] = true; // cleaner if automatically added in classes def, and removed from generated JS code.

    foo_ro() {}
    faa_rw(){}
}

type RO<T> = Readonly<{
    // cleaner if condition would be the presence of a flag in the method.
    [K in keyof Omit<T, typeof RW> as K extends `${infer _X}_ro` ? K : never ]: T[K]
}>

let x: RO<X> = new X();
x.foo_ro();
x.faa_rw();
let y: X = x;

Still, I don't understand why TS isn't doing such very basic check.

Reasons are provided in the issues. Just have to search for them properly and read through them. You may also want to join the TypeScript Discord.

In general:

resources are limited.
priorities differ.
trade-offs are being made.
simple and basic things are often not simple and basic.
TypeScript is not designed or aims to have a fully sound type system.

Why are methods bivariant? See #18654, where strictFunctionTypes was implemented:

The stricter checking applies to all function types, except those originating in method or construcor declarations. Methods are excluded specifically to ensure generic classes and interfaces (such as Array<T>) continue to mostly relate covariantly. The impact of strictly checking methods would be a much bigger breaking change as a large number of generic types would become invariant (even so, we may continue to explore this stricter mode).

You may also want to join the TypeScript Discord.

Maybe I'll do it next time.

* resources are limited.

Is a flag check really what is the most resources intensive in TS ?

* priorities differ.

I understand, but doing things in half will just result into things blowing in your hand.
If a readonly is introduced, it is expected that it would have at least the very minimal and basic features.

If I declare something readonly, I don't want it to explode just because of a bad affectation.

* trade-offs are being made.

Adding a if vs things blowing up ?
Sorry, I'm a little salty tonight.

* TypeScript is not designed or aims to have a fully sound type system.

Yeah, I'm starting to understand it...
But there are fully sound in one side, and messed up in the other.
I kind of understand some inconsistencies, but the last things are messed up.

More than a week, lost because of issues and bugs (because yes, even if it is intentional, theses are bugs) known for 5 to almost 10 years... I think I earned the right to be a little salty tonight.

Is a flag check really what is the most resources intensive in TS ?

I'm referring to the team. Someone has to implement and maintain the code.

If a readonly is introduced, it is expected that it would have at least the very minimal and basic features.

It does have very a minimal and basic feature: via that type you can't modify the property. That's what most people wanted.

Adding a if vs things blowing up ?

Trade-offs in terms of language design and complexity, as well as priority of features to implement.

Sorry, I'm a little salty tonight.

You should really abstain from public discussions then. It's not the right place to vent your salt.

even if it is intentional, theses are bugs

No, they're not. Undesired behavior, sure. Bug, no.

I was reading about in out annotations in the announcement of 4.7 (couldn't find them in the doc when searching on Google).

The initial issue is in fact solved by using class X<in T> in the declaration of X.
Could be nice to have a flag to get a warning to remember us to use in and out inside generics (I don't think there are cases where it is neither in nor out)...

Also, isn't Array<T> ill defined ? Shouldn't it be Array<in out T> ?

I'm referring to the team. Someone has to implement and maintain the code.

Trade-offs in terms of language design and complexity, as well as priority of features to implement.

Ok, this I can understand.

No, Array<in out T> would make it invariant. Array is intentionally covariant because making Dog[] not be assignable to Animal[] would break a lot of real-world code. The unsoundness there is deliberate.

Array is intentionally covariant because making Dog[] not be assignable to Animal[] would break a lot of real-world code.

A code that would make that is already kind of broken.
Only Dog[] -> readonly Animal[] (ReadonlyArray<out T>) should be correct, no ?

The unsoundness there is deliberate.

Why allowing such a dangerous unsoundness in TS, i.e. in scripts where we want some protections, instead of simply allowing to skip some tests in some files when needed ? Outside of theses files, we'd just need an explicit cast if necessary.

Denis Migdal (@denis-migdal)

Denis Migdal (@denis-migdal) I'm just going to ask that you please not learn TypeScript's various ins and outs via filing bugs on the issue tracker. If you have some behavior that also repros in 3.3, it's 99.99% certain that it's not a longstanding bug that's just been overlooked for 5+ years.

Denis Migdal (@denis-migdal)

Denis Migdal (@denis-migdal) I'm just going to ask that you please not learn TypeScript's various ins and outs via filing bugs on the issue tracker.

I'm sorry for that, but you'll have to admit that TS has LOT of undocumented unsoundness, at a point you really should have a whole "Designs choices and intentional unsoundness" chapter in the documentation...

Some features are even undocumented, like in and out annotations, only talked about in the 4.7 announcement.
With such feature, it should either (from best to worst) :

TS deduces the in/out by itself and explicit annotation is used to change this behavior.
every generic be in out by default.
every generic requiring explicit in/out annotations

But instead of introducing some compiler flag(s), TS introduced unsoundness instead and again...
This is a living hell of undesirable undocumented behavior... just to dirty-solve some specific cases, when more control on the compiler flags would do the trick...

If you have some behavior that also repros in 3.3, it's 99.99% certain that it's not a longstanding bug that's just been overlooked for 5+ years.

If if walk like a bug, and quack like a bug, it is very likely a bug. You can call intentional bugs "intentional unsoundness", it still remains a bug (more specifically a flow or a fault in the design).

How many big issues are still open and has been overlooked for 5+ years, while having lot of duplicates ?

TS deduces the in/out by itself and explicit annotation is used to change this behavior.

This is, in fact, exactly what happens. The annotations were added later because TS sometimes gets it wrong.

If if walk like a bug, and quack like a bug, it is very likely a bug. You can call intentional bugs "intentional unsoundness", it still remains a bug (more specifically a flow or a fault in the design).

It’s not a fault in the design if soundness was never a design goal in the first place (and this fact is documented). You may not like it, but that’s the direction the designers chose and if that’s unsuitable for your use, you’ll need to look for a different tool.

i wish strictMethodTypes were a thing. for the time being, my advice is to avoid classes as much as possible. most of the time, type aliases and function suffice and even if it's a bit more work (or less performance) it's imho well worth the additional help the compiler can give you.

This is, in fact, exactly what happens. The annotations were added later because TS sometimes gets it wrong.

When TS gets it wrong on very basic examples... is it really the case ?

class X<T> { // should be "in"...
    #t!: T;

    callback(t: T) {

        this.#t = t; // even without that, this should be "in"
    }
}

At this point, better having a compiler option to enable the second behavior : everything is in out by default.

It’s not a fault in the design if soundness was never a design goal in the first place (and this fact is documented).

The fact that bugs are called "unsoundness" without documenting them is a fault in the design.
As I already stated, I can understand little unsoundness on edges cases due to the internal implementation of things. But, by default, allowing implicit Dog[] -> Animal[] cast isn't an unsoundness, this is a dangerous behavior.

This should:

only allow implicit Dog[] -> readonly Animal[] cast
else, requires an explicit cast
else, compiler options

The same for cast with readonly properties.

if that’s unsuitable for your use, you’ll need to look for a different tool.

If only this different tool existed...

class X { // should be "in"...

This is a classic invariant case - "represented" as in out in TS. This works very much as expected and is correct. If you are after soundness you should, at the very least, be happy about this one.

If only this different tool existed...

It's an open market. You can create an alternative or stop paying for this piece of software that doesn't satisfy your needs.

This is a classic invariant case - "represented" as in out in TS. This works very much as expected and is correct. If you are after soundness you should, at the very least, be happy about this one.

? I do not think TS gets the annotations right in theses cases:

	class X<T> {
		#t!: T;
	
		callback(t: T) {
			this.#t = t;
		}
	}

	const _a: X<unknown> = {} as X<boolean> // authorized

	class X<in out T> {
		#t!: T;
	
		callback(t: T) {
			this.#t = t;
		}
	}

	const _a: X<unknown> = {} as X<boolean> // forbidden (ok)

Without the attribute:

{
	class X<T> { // should be "in"...
	
		callback(t: T) {}
	}

	const _a: X<unknown> = {} as X<boolean> // authorized
}

{
	class X<in T> { // should be "in"...
	
		callback(t: T) {}
	}

	const _a: X<unknown> = {} as X<boolean> // forbidden (ok)
}

? I do not think TS gets the annotations right in these cases:

Ah, right. I've misinterpreted my quick tests. This has already been mentioned in the thread - but well, methods are bivariant in TS. You might not agree with this decision but it's here to stay. You can always create a linting rule to avoid the method syntax altogether. This behaves like you expect it to:

class X<T> {
  t!: T;

  callback = (t: T) => {
    this.t = t;
  };
}

const _a: X<unknown> = {} as X<boolean>; // forbidden (ok)

methods are bivariant in TS. You might not agree with this decision but it's here to stay.

And this decision is an example of a design flaw:
This produces unsound and unsafe behaviors (cf example), while simple alternatives (some I presented here) would easily solve issues...

Why do safe and easy, when we can do unsafe and complex ?

You can always create a linting rule to avoid the method syntax altogether. This behaves like you expect it to

There are differences between methods and attributes. One of them is that you can't super an attribute, e.g. :

class A {

	a = () => 4
}

class B extends A {
	override a = () => { return super.a() + 1 }
    // Class field 'a' defined by the parent class is not accessible in the child class via super.ts(2855)
}

It's not like we're not aware of the downsides of this. If it were easy and simple to fix, we would have done it already.

And this decision is an example of a design flaw:
This produces unsound and unsafe behaviors (cf example), while simple alternatives (some I presented here) would easily solve issues...

As I said before, it's not a design flaw because soundness and correctness are not design constraints.

From https://github.com/microsoft/TypeScript/wiki/TypeScript-Design-Goals under Non-Goals:

[Do not try to] Apply a sound or "provably correct" type system. Instead, strike a balance between correctness and productivity.

It's not like we're not aware of the downsides of this. If it were easy and simple to fix, we would have done it already.

Why would e.g. an --require-explicit-generics-inout (set every generics in out by default) or a --require-explicit-ro-casts option be hard ?
You use it if you need for security, and don't if you need compatibility.

Navigation Menu

Repository navigation

Dangerous calls allowed by TS: class generic type, when `unknown`, should be treated as `never` in the class methods/callback parameters #57519

🔎 Search Terms

🕗 Version & Regression Information

⏯ Playground Link

💻 Code

🙁 Actual behavior

🙂 Expected behavior

Additional information about the issue

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Dangerous calls allowed by TS: class generic type, when unknown, should be treated as never in the class methods/callback parameters #57519

Description

🔎 Search Terms

🕗 Version & Regression Information

⏯ Playground Link

💻 Code

🙁 Actual behavior

🙂 Expected behavior

Additional information about the issue

Activity

MartinJohns commented on Feb 24, 2024

denis-migdal commented on Feb 24, 2024

MartinJohns commented on Feb 24, 2024

denis-migdal commented on Feb 24, 2024

Andarist commented on Feb 24, 2024

denis-migdal commented on Feb 24, 2024

MartinJohns commented on Feb 24, 2024

fatcerberus commented on Feb 24, 2024

denis-migdal commented on Feb 24, 2024

MartinJohns commented on Feb 24, 2024

denis-migdal commented on Feb 24, 2024

fatcerberus commented on Feb 24, 2024

denis-migdal commented on Feb 24, 2024

RyanCavanaugh commented on Feb 25, 2024

denis-migdal commented on Feb 25, 2024

fatcerberus commented on Feb 25, 2024

ritschwumm commented on Feb 25, 2024

denis-migdal commented on Feb 26, 2024

Andarist commented on Feb 26, 2024

denis-migdal commented on Feb 26, 2024

Andarist commented on Feb 26, 2024

denis-migdal commented on Feb 26, 2024

RyanCavanaugh commented on Feb 26, 2024

fatcerberus commented on Feb 26, 2024

denis-migdal commented on Feb 26, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Dangerous calls allowed by TS: class generic type, when `unknown`, should be treated as `never` in the class methods/callback parameters #57519