eslint-plugin-flowtype-errors icon indicating copy to clipboard operation
eslint-plugin-flowtype-errors copied to clipboard

Report error which Flow does not report

Open pizzafroide opened this issue 7 years ago • 8 comments

This use case is where, in C/C++, a forward declaration would be needed.

A.js:

// @flow

import B from './B';

export default class A {
  a() { return new B(this); }
}

B.js:

// @flow

import type A from './A';

export default class B {
  a: A;

  constructor(a: A) {
    this.a = a;
  }
}

Flow says No errors! but flowtype-errors/show-errors reports the following in A.js:

6:23  A:  This type is incompatible with the expected param type of A. See ./B.js:8

pizzafroide avatar Mar 10 '17 13:03 pizzafroide

This use case is where, in C/C++, a forward declaration

Not sure what C/C++ has to do with flow. Can you please elaborate on this. Is there a feature of C that you want in flow?

amilajack avatar Mar 10 '17 14:03 amilajack

Is there a feature of C that you want in flow?

Not at all. I'd just like flowtype-errors to not report an error when Flow does not. The C++ reference was just intended to explain the use case. More specifically, type checking class A needs B type to be known, and type checking class B needs A type to be known. (That's where, in C++, a forward declaration is needed.)

Again, Flow says No errors! while flowtype-errors/show-errors reports something.

pizzafroide avatar Mar 10 '17 15:03 pizzafroide

@pizzafroide would be great if you could make a PR for this

Here's an example https://github.com/amilajack/eslint-plugin-flowtype-errors/issues/68 https://github.com/amilajack/eslint-plugin-flowtype-errors/pull/74

amilajack avatar Mar 25 '17 20:03 amilajack

@amilajack I'm a little busy right now and have no time to address this issue. For now, I've just added a note telling that this kind of error is actually OK. But I'll consider making a proper PR later on. Thanks for the links to the related issues anyway.

pizzafroide avatar Mar 27 '17 07:03 pizzafroide

It is an issue with flow. Not sure if we can find a work around...

jdmota avatar Mar 27 '17 17:03 jdmota

Closing this, as we're waiting on flow. @jdmota Thanks for finding this!

amilajack avatar Mar 27 '17 17:03 amilajack

Hi,

It's been more than a year since the issue was reported to Flow and it is still open. Could we think about a way of solving it from this side ?

I understand that the first purpose was to save processing time by running Flow on only one file. But the Flow process is actually blazingly fast, so wouldn't it be conceivable to spawn the Flow process once at the start of the eslint process, then filter the output for each file ?

I tried to run Flow without the check-contents flag (which is the one causing the issue), and thanks to the --json option we get the errors that we can filter by file, in order to only keep the ones for the current file !

This would solve the issue at stake here. The difference I see is that we are letting Flow reading the content of the file from the filesystem, instead of feeding it from context.getSourceCode().getText(). Does anyone know if there is a feature of eslint for which it could be an issue ? I heard that the IDEs use the check-contents feature to run Flow on unsaved files. Is that possible with eslint ?

What is your opinion @amilajack ?

micaste avatar Oct 16 '17 12:10 micaste

Another year has gone by, and I'm still running into this issue. I've resorted to sticking // $FlowFixMe in places where it isn't really needed by Flow. Does anyone else have a better workaround?

murrayju avatar Sep 08 '18 03:09 murrayju