Make type annotations in function type literals mandatory

[DanielRosenwasser]

Too many times do we have new users using function type literals without type annotations on their parameters - recently, #3080. Currently the following:

var x: (number, string) => void;

is implicitly typed like:

var x: (number: any, string: any) => void;

even though a user probably meant to type x like in the following way:

var x: (a: number, b: string) => void;

It is actually extremely strange that parameter names, which function purely for documentation, are mandatory while their type annotations are optional.

While --noImplicitAny catches this, new users often don't know about that flag to begin with.

I don't see a good use case for a user who is wiling to type out an arrow type, but wants their parameters to implicitly be typed as any. While this is a breaking change, I'm sure most of the breaks would be with code like in the given example.

[donnut]

👍

[mhegazy]

it would be interesting to see if this pattern even exists in any of the RWC code bases. i suspect not.

[spion]

👍, current behavior was surprising to me when I first encountered it.

[RyanCavanaugh]

A scan over DefinitelyTyped shows no obvious cases where someone wrote a type name as a variable name.

Samples:

C:\github\DefinitelyTyped\bootstrap.paginator\bootstrap.paginator.d.ts:
   11:     itemContainerClass?: (type, page, current) => string;
   18:     tooltipTitles?: (type, page, current) => string;
   22:     onPageClicked?: (event, originalEvent, type, page) => void;
   23:     onPageChanged?: (event, originalEvent, type, page) => void;
   24  }

C:\github\DefinitelyTyped\durandal\durandal-1.x.d.ts:
   15        * Sets the module id on the module.
   16        */
   17:     export var setModuleId: (obj, id: string) => void;

C:\github\DefinitelyTyped\fabricjs\fabricjs.d.ts:
   18:     function parseSVGDocument(doc: SVGElement, callback: (results, options) => void , reviver?: (el, obj) => void );

C:\github\DefinitelyTyped\jquery.contextMenu\jquery.contextMenu.d.ts:
   13      delay?: number;
   14      determinePosition?: (menu) => void;
   15:     position?: (opt, x, y) => void;
   16      positionSubmenu?: (menu) => void;

C:\github\DefinitelyTyped\less\less.d.ts:
  533              mime: string;
  534              error;
  535:             push(path: string, callback: (e, root, imported) => void);

C:\github\DefinitelyTyped\linq\linq.d.ts:
   34      interface Enumerable {
   35          //Projection and Filtering Methods
   36:         CascadeBreadthFirst(func: ($) => any[], resultSelector: (v, i: number) => any): Enumerable;

C:\github\DefinitelyTyped\sharepoint\SharePoint.d.ts:
  808      getToolTop(): string;
  809      getDisabledToolTip(): string;
  810:     getOnClickCallback(): (event, action: CalloutAction) => any;

[zpdDG4gta8XKpMCd]

👎 RTFM

come on, it's typescript why don't you use types?

unless we go pointree functional where parameter names don't mean a thing and signatures like this would mean types rather than names

[spion]

Aren't those typeless declarations broken when noImplicitAny is used? I would consider that a bug in the definition as they're supposed to work in all modes.

[dead-claudia]

👍 I find this behavior surprising coming from other strongly typed languages, such as C/C++ and Haskell.

// C++
// Argument types: string, double
// Return type: Type
Type foo(string, double);

-- Haskell
-- Argument types: [Char], Double
-- Return type: Type
foo :: [Char] -> Double -> Type

// TypeScript
// Argument types: any, any
// Return type: Type
function foo(string, number): Type;

[dead-claudia]

And, @spion I agree with that.

[zpdDG4gta8XKpMCd]

A tslint rule for this is trivial and operates at the syntax level. If
there is a result node in the AST for a function then you are good. Can
share code if you want.
On Jun 16, 2015 7:57 PM, "impinball" notifications@github.com wrote:

And, @spion https://github.com/spion I agree with that.

—
Reply to this email directly or view it on GitHub
#3081 (comment)
.

[alexeagle]

TSLint does allow to fix the problem, but it has the downside of disallowing code which is probably okay. I would rather have a static analysis check that the names of the parameters are not also valid types in the scope. So this would be okay shorthand:

function funcFactory(): (_) => void { ... }

but this would be an error:

function funcFactory(): (number) => void { ... }

and also an error:

function funcFactory<T>(): (T) => void { ... }

[adidahiya]

@alexeagle It should be possible to extend TSLint's variable-name rule with a new option that applies the check you are proposing. The part where you compare the argument identifier against all valid types in the scope would require linting programs as a whole, which we don't do yet (palantir/tslint#680).

EDIT: filed a TSLint issue to create a rule which addresses this problem palantir/tslint#723

[dead-claudia]

@alexeagle

That's because you need (_: number) => void and (_: T) => void,
respectively. That's the TypeScript language itself, not TSLint. It's a
gotcha, but it's mainly to me consistent with TypeScript's optional typing
when not using the noImplicitAny option.

On Fri, Oct 9, 2015, 12:58 Alex Eagle notifications@github.com wrote:

TSLint does allow to fix the problem, but it has the downside of
disallowing code which is probably okay. I would rather have a static
analysis check that the names of the parameters are not also valid types in
the scope. So this would be okay shorthand:

function funcFactory(): (_) => void { ... }

but this would be an error:

function funcFactory(): (number) => void { ... }

and also an error:

function funcFactory(): (T) => void { ... }

—
Reply to this email directly or view it on GitHub
#3081 (comment)
.

[alexeagle]

@adidahiya that would be a great check to have, I'd turn that on in Angular right away 👍

@IMPinball true, you need those expressions with --noImplicitAny. My point is that I'd like behavior where the untyped parameter named _ is permitted, but not an untyped parameter named number. And that behavior is not currently possible. Does that make sense?

[dead-claudia]

The fact number doesn't work sounds like a bug, to be honest. I would
file a separate issue for that.

On Fri, Oct 9, 2015, 14:29 Alex Eagle notifications@github.com wrote:

@adidahiya https://github.com/adidahiya that would be a great check to
have, I'd turn that on in Angular right away [image: 👍]

@IMPinball https://github.com/impinball true, you need those
expressions with --noImplicitAny. My point is that I'd like behavior
where the untyped parameter named _ is permitted, but not an untyped
parameter named number. And that behavior is not currently possible. Does
that make sense?

—
Reply to this email directly or view it on GitHub
#3081 (comment)
.

[alexeagle]

number is currently allowed, and I would like to disallow it.

[dead-claudia]

@alexeagle The behavior I'm used to expecting from TS is that these three are equivalent types:

(number, number) => void
(number: any, number: any) => void
(a: any, b: any) => void

[dead-claudia]

If you change that behavior, it changes the semantics enough that it would break a lot of existing code. Not everyone uses --noImplicitAny, so it's not purely an additive change.

That significant of a change would be concerning for even a major version increment.

[alexeagle]

I think we are talking past each other here. Those are indeed three equivalent types. But the first one is so wrong, I would like a static analysis check to say "you surely, surely did not mean that, and it is clearly a bug". This would not be a language spec change, and in the current landscape, it fits as a tslint check.

However, our experience at Google has been that linters are rarely run electively by engineers, and we want these rules consistently applied, so we prefer to plug such rules into the compiler, where everyone is bound by them.

[dead-claudia]

Ok...I think we can agree to disagree on what we think should be the case.

On Tue, Oct 13, 2015, 13:22 Alex Eagle notifications@github.com wrote:

I think we are talking past each other here. Those are indeed three
equivalent types. But the first one is so wrong, I would like a static
analysis check to say "you surely, surely did not mean that, and it is
clearly a bug". This would not be a language spec change, and in the
current landscape, it fits as a tslint check.

However, our experience at Google has been that linters are rarely run
electively by engineers, and we want these rules consistently applied, so
we prefer to plug such rules into the compiler, where everyone is bound by
them.

—
Reply to this email directly or view it on GitHub
#3081 (comment)
.

[dead-claudia]

I just don't feel like arguing it any more.

On Tue, Oct 13, 2015, 13:28 Isiah Meadows impinball@gmail.com wrote:

Ok...I think we can agree to disagree on what we think should be the case.

On Tue, Oct 13, 2015, 13:22 Alex Eagle notifications@github.com wrote:

I think we are talking past each other here. Those are indeed three
equivalent types. But the first one is so wrong, I would like a static
analysis check to say "you surely, surely did not mean that, and it is
clearly a bug". This would not be a language spec change, and in the
current landscape, it fits as a tslint check.

However, our experience at Google has been that linters are rarely run
electively by engineers, and we want these rules consistently applied, so
we prefer to plug such rules into the compiler, where everyone is bound by
them.

—
Reply to this email directly or view it on GitHub
#3081 (comment)
.

[spion]

Its very unfortunate that this issue can't be resolved, and I think it should be on the list if / (for whenever) its decided that there will be a breaking release:

Just take this type for example:

declare function compose<T,U,V>(f1:(u:U) => V, f2: (t:T) => U): (t:T) => V;

It could've been written as

declare function compose<T,U,V>(U => V, T => U): T => V;

which is much more readable.

This may not be as important to people already used to the syntax, but it certainly detracts some people away from the language, e.g. https://news.ycombinator.com/item?id=10420940

[dead-claudia]

@spion

I think it's more of an artifact of basing the function type semantics on the arrow function semantics. If you don't have both, the type of that parameter is implicitly any. It's consistent with the rest of the language, just as JavaScript functions before ES6 were super consistent. [1] If anything, I've picked up that consistency only takes you so far before it starts tripping up people.

---

[1]
Compare (these are equivalent):

var f: x => number
var f = x => x + 1

var f: (x: any) => number
var f = (x: any) => x + 1

Before ES6, this was actually extremely simple and consistent: every function has its own independent this. Simple as that. For example:

function foo() {
  var self = this; // foo's this
  return function bar() {
    return [
      self, // a closure variable
      this // bar's this
    ];
  };
}

foo.call(1).call(2); // [1, 2]

Or, if s-expressions help make this clearer:

(defun foo ()
  (let self this) ; foo's this
  (defun bar ()
    (list
      self ; a closure variable
      this ; bar's this
    ))
)

((foo.call 1).call 2) ; [1, 2]

And new Class(...arguments) did this under the hood until ES6 introduced new.target. Emulated in ES5 JavaScript (except for then-deprecated __proto__)

function Construct(Class, args) {
  var self = {};
  var proto = Class.prototype;
  if (typeof proto === "object" && proto !== null) {
    self.__proto__ = proto;
  }
  var res = Class.apply(self, args);
  if (res !== undefined) return res;
  return self;
}

It just tripped up a lot of people.

---

The simplicity is also a reason why I've been slow to adopt the new ES6 object model. I can't stand new. It looks simple and elegant until you come across cases where you have multiple is-a relationships. A dolphin is a mammal, but it also is a fish. You can't model that in a single-inheritance environment.