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

Annotation of this in function declarations #452

Closed
jussi-kalliokoski opened this issue May 18, 2015 · 24 comments
Closed

Annotation of this in function declarations #452

jussi-kalliokoski opened this issue May 18, 2015 · 24 comments

Comments

@jussi-kalliokoski
Copy link

@jussi-kalliokoski jussi-kalliokoski commented May 18, 2015

Currently it seems that you can only annotate the this magic reference by putting the function as a method in a class declaration.

However, in the light of the new bind syntax proposal, it would make sense to be able to annotate functions that take in this as data, e.g.

function head (count) {
    return this.slice(0, count);
}

[1,2,3]::head(2) // [1,2]

could be annotated something like:

function head <T> Array<T> -> (count : number) : Array<T> {
  ...
}
@samwgoldman
Copy link
Member

@samwgoldman samwgoldman commented May 18, 2015

Flow is (mostly) smart about this already without annotations. I'm not clear what this new bind syntax is (do you have a link to a spec?), but I imagine flow will support it the same way that flow currently supports bind, call, and apply.

/* @flow */

function head(count) {
  return this.slice(0, count);
}

head.bind([]) // OK
head.bind(1) // bad
@jussi-kalliokoski
Copy link
Author

@jussi-kalliokoski jussi-kalliokoski commented May 19, 2015

I'm not clear what this new bind syntax is (do you have a link to a spec?)

Ah, sorry, forgot the link, fixed.

Flow is (mostly) smart about this already without annotations

Yes, but it would be nice to be able to annotate this instead, for documentational purposes (easier to understand at a glance and also allows for better error messages, as well as documentation generation), but also for standalone definitions.

@jussi-kalliokoski
Copy link
Author

@jussi-kalliokoski jussi-kalliokoski commented Jun 9, 2015

Having thought about this a bit more, a simpler syntax would actually be to allow specifying this as a parameter, as so:

function add (this : number, b : number) : number {
  return this + b;
}

function add (/*: this : number, */ b /*: number */) /*: number */ {
  return this + b;
}

/*: (this : number, b: number) => number */
function add (b) {
  return this + b;
}
@avikchaudhuri
Copy link
Contributor

@avikchaudhuri avikchaudhuri commented Jun 19, 2015

Agree that we should support this to be annotated as a parameter. This requires some parser work too, btw, since this is not a valid identifier everywhere.

We need this whenever methods are exported, by the way. It's true Flow is good in inferring this types, but it sucks that this information is lost across modules.

Related: constructor functions can be annotated as of 618186a

@Artazor
Copy link

@Artazor Artazor commented Aug 3, 2015

Hi guys! what about the following examples: microsoft/TypeScript#1985 (comment)
Are they too complex/ugly?

@izaakschroeder
Copy link

@izaakschroeder izaakschroeder commented Aug 7, 2015

+1

@danvk
Copy link
Contributor

@danvk danvk commented Oct 22, 2015

Pulling in some material from #968: an example, and a link to how Google Closure does this.


There's an error (marked with <---) in this snippet which Flow could potentially catch:

class RemoteFile {
  url: string;
...
  promiseXHR(xhr: XMLHttpRequest): Q.Promise<[any, Event]> {
    var url = this.url;
    var deferred = Q.defer();
    xhr.addEventListener('load', function(e) {
      if (this.status >= 400) {
        deferred.reject(this.status + ' ' + this.statusText);
      } else {
        deferred.resolve([this.response, e]);
      }
    });
    xhr.addEventListener('error', function(e) {
      deferred.reject(`Request for ${this.url} failed: ${this.status}`);  // <---
    });
    this.numNetworkRequests++;
    xhr.send();
    return deferred.promise;
  }
}

The problem is that this inside the addEventListener callback refers to the XMLHttpRequest object (I believe) and not the RemoteFile class. The latter has a url property but not a status property, whereas the former has status but not url. Clearly this code isn't right!

Flow doesn't complain, though. It thinks this has type any in the event listener.

Where possible, the DOM declarations should indicate the type of this in callbacks to prevent this sort of mistake.

For example, Google's Closure Compiler provides an @this annotation. You can see an example of this in action in the declaration for Array.prototype.find.

@glortho
Copy link

@glortho glortho commented Apr 18, 2016

In case you land here before this +1 functionality is added, and you need types just for the fn body, just do something like this:

type Ctx = { foo: string, bar: number }
myMethod() {
  const myCtx: Ctx = this;
  // do stuff with myCtx instead of this
}
@STRML
Copy link
Contributor

@STRML STRML commented Aug 22, 2016

Just for reference, TS 2.0 landed with this feature last month. It special-cases the first parameter if its name is this. I like that, reminds me of Python.

function f(this: void) {
    // make sure `this` is unusable in this standalone function
}
interface UIElement {
    addClickListener(onclick: (this: void, e: Event) => void): void;
}

An interesting note from the ticket when they were speccing it:

Yeah, the callback literally does "receive" this object as a parameter. That's how it works for all functions. It just happens that there are different rules for how you pass the value of this, it doesn't appear in the arguments array, and there's a different default value in non-strict mode. Otherwise it's basically just another argument.

Makes sense to me. If babel-plugin-flow-strip-types stripped the entire first parameter when its name is this, it could work. Will likely require special-casing in ESLint etc.

@nmn
Copy link
Contributor

@nmn nmn commented Mar 4, 2017

This already works in Flow:

function head (count: number) {
  (this: Array<number>);
  return this.slice(0, count);
}

head.call([1, 2, 3], 10);
head.call(['123'], 20); // error

Do we really need new syntax for this?

However there are some bugs with the approach as this totally bugs out:

function head<T>(count: number): Array<T> {
  (this: Array<T>);
  return this.slice(0, count);
}

head.call([1, 2, 3], 10);
head.call(['123'], 20);
@HerringtonDarkholme
Copy link

@HerringtonDarkholme HerringtonDarkholme commented Mar 4, 2017

@nmn Yes we do need.

It's for library declaration and use site completion. This is quite common for context injection.

function addListener<T: HTMLElement>(elem: T, func: (this: T) => void) {
  // implementation
}
addListener(htmlInputElement , function() {
  this.value // should have this completion
})

Another usage is in library like Vue/backbone.

Vue.extend({
  methods: {
    log() {
      this.instanceProperty // this is injected as Vue instance
    }
  }
})

Without this annotation, it is impossible for library author to provide a good type declaration for automatic completion.

@kazupon
Copy link

@kazupon kazupon commented Mar 6, 2017

I hope this feature is provided 🙏

@scottbedard
Copy link

@scottbedard scottbedard commented Mar 21, 2017

+1 for this feature

@iclanzan
Copy link
Contributor

@iclanzan iclanzan commented Feb 16, 2018

Has there been no progress on this yet?

@dmnd
Copy link
Contributor

@dmnd dmnd commented Feb 16, 2018

@calebmer was looking for people to help implement it a while ago. I'm not sure if anyone started on it. I'd love to do it if I had more time.

@KindWizzard
Copy link

@KindWizzard KindWizzard commented Apr 5, 2018

Any updates on this? :)

@akoppela
Copy link

@akoppela akoppela commented Jul 30, 2018

@danvk It seems that you're using flow type version of Q.Promise, is it? If so do you mind to share a link to flow typed Q.Pomise library?

@danvk
Copy link
Contributor

@danvk danvk commented Jul 31, 2018

@akoppela https://github.com/hammerlab/pileup.js/blob/0182b225205e52e1489727004d69e84442eafb88/lib/q.js but take note that these are ~3 years old and there may be better ones available elsewhere.

lgeiger added a commit to lgeiger/flowgen that referenced this issue Oct 13, 2018
flow doesn't have support for [`this` annotation](https://www.typescriptlang.org/docs/handbook/functions.html) in functions: facebook/flow#452

This will result in prettier failing to parse the generated code. This PR removes the this annotation in function declarations.
@goodmind goodmind added the Has PR label Jun 8, 2019
@goodmind
Copy link
Contributor

@goodmind goodmind commented Jun 8, 2019

Hey! I tried to implement it at #7807

@Bannerets
Copy link

@Bannerets Bannerets commented Jul 19, 2019

Note that this constraints in classes can be emulated via $Call

declare class O<T> {
  m(): $Call<(string => string), T>;
  m(): $Call<(number => number), T>;
}
  
declare var o1: O<number>
  
const a1: number = o1.m() // ok
const b1: string = o1.m() // error

declare var o2: O<string>
  
const a2: number = o2.m() // error
const b2: string = o2.m() // ok

declare var o3: O<{}>
  
const a3 = o3.m() // error
const b3 = o3.m() // error

^ flow try

class M<+State> {
  +s: State
  constructor(s: State) { this.s = s }
  to2: $Call<(1 => () => M<2>) & () => {...}, State> = () => {
    return new M(2) // ok
  }
  to3: $Call<(1 => () => M<3>) & () => {...}, State> = () => {
    return new M(2) // error
  }
}

const m1 = new M(1)
const m2 = m1.to2() // ok
const m3 = m2.to2() // error

^ flow try

declare class O<+T> {
  constructor(T): void;
  m(): $Call<<U>(Array<U>) => U, T>
}
const oarr = new O([1,2,3])
const n: number = oarr.m() // ok
const s: string = oarr.m() // error

declare class Ap<+T> {
  ap<A, B>(Ap<A>): $Call<(A => B) => Ap<B>, T>
}
const a = new Ap<number => string>
const m: Ap<number> = a.ap(new Ap<3>) // error
const d: Ap<string> = a.ap(new Ap<3>) // ok

^ flow try

declare class A {
  m(): $Call<B => 1, this>;
  n(): $Call<<U>(C<U>) => U, this>;
}
declare class B extends A {}
declare class C<T> extends A {}

const a = new A
a.m() // error (not in this place tho)
const b = new B
b.m() // ok

const c = new C<string>()
const n0: string = c.n() // ok
const n1: number = c.n() // error

^ flow try

@iclanzan
Copy link
Contributor

@iclanzan iclanzan commented Jul 19, 2019

Wow, this is gold! Thank you @Bannerets!

@dmnd
Copy link
Contributor

@dmnd dmnd commented Mar 10, 2020

Fixing this was mentioned in the latest roadmap.

@STRML
Copy link
Contributor

@STRML STRML commented Jan 25, 2021

The this annotation was released in 0.139.

To use, add this to flowconfig:

[options]
experimental.this_annot`= true

To use:

function foo(this: {toString: (number) => string}, a: number): string {
  return this.toString(a);
}

It is also usable at https://flow.org/try.

@gkz gkz closed this Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.