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

Issue a warning when generic type inference produces {} #360

Closed
RyanCavanaugh opened this issue Aug 5, 2014 · 18 comments
Closed

Issue a warning when generic type inference produces {} #360

RyanCavanaugh opened this issue Aug 5, 2014 · 18 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

Motivating example (one of many):

function compare<T>(lhs: T, rhs: T): boolean {
  return lhs === rhs;
}

if(compare('1', 1)) { // Expected: Error -- I made 'compare' generic for a reason!
  /* ... */
}

Proposal

When generic type inference performs its Best Common Type operation (4.12.2), it should be an error if this returns {} when {} was not one of the input types.

This is entirely consistent with other behavior already in the compiler:

var foo = bar ? '1' : 1; // Error, no BCT between '1' and 1
function fn() { // Error, no BCT between '1' and 1
  if(foo) {
    return '1';
  } else {
    return 1;
  }
}

Open Questions

From @KamyarNazeri -- should this apply when there are zero input types? e.g.:

class List<T> {
    items: T[];
}

var x = new List(); // Probably want an error here

That seems desirable, but has some collateral damage:

function foo<T>(x: number, a?: T, b?: T) { }
foo(0); // Error, zero input types provided for inference

This would come up slightly more often than naively expected because many .d.ts authors (mistakenly) create generic types which don't actually consume their type parameters. Perhaps this warning would effectively discourage them from doing so?

@basarat
Copy link
Contributor

basarat commented Aug 5, 2014

👍

@johnnyreilly
Copy link

Yes yes yes!

@saschanaz
Copy link
Contributor

Great :D

@knazeri
Copy link

knazeri commented Aug 5, 2014

This is really great!
I hope this also includes creating instances from a generic class without the type parameters in the constructor.

@RyanCavanaugh should this apply when there are zero input types?
I think it should, given the example above, type inference fails to correctly address generic instance methods:

class List<T> {
    items: T[]= [];
    add(item: T) {
        this.items.push(item);
    }
}

var t = new List();
t.add(1);
t.add("1");

@DanielRosenwasser
Copy link
Member

So @JsonFreeman, @vladima, and I discussed this offline a bit yesterday.

One outstanding question is whether or not we want to error in the case that there are no available candidates to determine the best common type for a type parameter. A good example of this was seen in @KamyarNazeri's example:

class List<T> {
    items: T[]
}
var xs = new List();

Here, our new call expression lacks any arguments for which to infer T.

If we do want to error on such a case, then we feel it is worth asking whether we also want to error if a type variable is entirely unused within a type signature.

For instance:

function f<T1,T2>(): void {
}

f();

In this example, there will never be candidates for which to instantiate T1 or T2's types. Using the simple approach, we would always complain at the call-site.

However, we could complain at the definition site instead.

Our feeling is that a type variable that is unused in the signature buys nothing in terms of type safety and utility. There is very little someone can do with a type parameter appearing within a function body if it does not appear within the signature.

@JsonFreeman
Copy link
Contributor

I agree with Daniel's comment, although I want to clarify that we don't error on signatures that don't use their own type parameters. So this would be a new error.

Another thing: I don't believe we should error in the absence of inference candidates if the type parameter has a constraint. Instead, we should fall back to the constraint, just like we do for inference results that are not assignable to the constraint.

@RyanCavanaugh
Copy link
Member Author

I'm thinking unconsumed generic type parameters in general to be an error under this rule. They're land mines for type inference -- people think that they can just write interface List<T> { } and have that work (see the current underscore.d.ts file for good examples), then log bugs when type inference doesn't work because there are no members to infer from.

Pre-documenting:

How do I fix "Type argument 'T' is not used in function 'f'" ?

Example:

function getThing<T>(name: string): T {
  /* ... */
}
var x = getThing<Animal>('cat');

This is a TypeScript anti-pattern because it implies that getThing is the function providing the type safety, when in reality it's the caller who's in control of the type. The better way to write this code is:

function getThing(name: string): {} {
  /* ... */
}
var x = <Animal>getThing('cat');

The only difference here is that <Animal> moved to the left of getThing rather than the right. One important note is that we've made getThing return {} instead of any. This avoids "implicit" any types caused by failing to convert the return value:

function getThingAny(name: string): any { /* ... */ }
function getThingEmpty(name: string): any { /* ... */ }

var x = getThingAny('cat'); // Forgot to type-assert, x: any
x.mov(); // No error
var y = getThingAny('cat'); // y: {}
y.mov(); // Error

@mhegazy
Copy link
Contributor

mhegazy commented Aug 6, 2014

@RyanCavanaugh I would group this with "Stricter" TypeScript #274, and not with #360

@basarat
Copy link
Contributor

basarat commented Aug 7, 2014

should this apply when there are zero input types?

👍

do we also want to error if a type variable is entirely unused within a type signature

👍

@zpdDG4gta8XKpMCd
Copy link

@RyanCavanaugh, being required to declare members in an interface in order for the type inference to work properly is a big inconvenience, what if there actually are members in an interface but I want to hide them intentionally in order to preserve the integrity? You may want to consider this as an example. I wish the members of List and Node were hidden because the caller has no business to see or use them directly. Does it mean that doing such sort of tricks (having an empty generic interface) is a crime in TypeScript? Is there a better idiomatic way of doing it without giving up on using generics?

@RyanCavanaugh
Copy link
Member Author

I don't think that's a common scenario. It seems very rare to have a generic type you want to traffic around on an object which doesn't use that type at all. If you really wanted to emulate that, you could do something like this:

interface Node<T> {
    '__element type'?: T;
}
interface NodeManager {
    create<T>(x: T): Node<T>;
    readValue<T>(x: Node<T>): T;
}
var mg: NodeManager;
var x = mg.create('foo');
var y = mg.readValue(x); // y: string

@zpdDG4gta8XKpMCd
Copy link

@RyanCavanaugh, speaking of common scenarios, here is another one for having poor-man units of measure for your consideration

@JsonFreeman
Copy link
Contributor

There is a way to have a truly private member on an interface. It takes advantage of the fact that interfaces can extend classes:

class NodePrivate<T> {
    private value: T;
}
interface Node<T> extends NodePrivate<T> { }

It's not pretty, but it would work if you really want an interface whose sole purpose is to wrap a type and not allow any interaction.

@zpdDG4gta8XKpMCd
Copy link

Scenarios like these are very common in functional programming. The motivation there is to have clear separation of data and logic contrary to what you see in OOP when a class is a mix of data and methods. Such separation has a number of advantages, one of them is the serialization and desalinization which are straight forward and simple compared to ones with classes involved. It's seems unfortunate that OOP was chosen as the main paradigm for TypeScript namely that encapsulation is only possible using private fields of a class. A class is a much heavier structure than an object literal yet according to the current vision and design it is a main building block.

@danquirk
Copy link
Member

Private fields of a class actually aren't private at runtime. Function closures are the only way to actually make something private and pass it around. OOP is most definitely not the main paradigm, the compiler sources themselves are fairly functional in style, primarily using object literals/interfaces and functions.

@danquirk
Copy link
Member

Status on this issue is that per #868 we do now give an error in the original case (multiple candidates leading to {}). The current implementation does not error on the additional suggests in this thread, namely generic inference with 0 candidates and unconsumed generic parameters.

@JsonFreeman
Copy link
Contributor

Pull request #951 makes the error for type argument inference failure more informative.

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Dec 2, 2015
@RyanCavanaugh
Copy link
Member Author

We have since fixed this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

10 participants