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

(v0.70) Missing type annotation when using array methods with exported function #6151

Closed
chrisirhc opened this issue Apr 17, 2018 · 29 comments
Closed

Comments

@chrisirhc
Copy link

chrisirhc commented Apr 17, 2018

On flow v0.70, array methods / generic methods seem to be broken. This only happens when the function is exported, and uses an array method. Marking a return type of any[] makes flow pass.

Reproduction here:

// @flow
function noErrors() {
  return [].map(b => b);
}

export function noErrors1() : any[] {
  return [].filter(b => b);
}

export function hasErrors() {
  // Errors out below with "Missing type annotation"
  return [].map(b => b);
}

export function hasErrors2() {
  // Errors out below with "Missing type annotation"
  return [].filter(b => b);
}

Errors:

12:   return [].map(b => b);
             ^ Missing type annotation for `U`.
17:   return [].filter(b => b);
             ^ Missing type annotation for `T`.
@chrisirhc chrisirhc changed the title Missing type annotation when using array methods with exported function (0.70) Missing type annotation when using array methods with exported function Apr 17, 2018
@chrisirhc chrisirhc changed the title (0.70) Missing type annotation when using array methods with exported function (v0.70) Missing type annotation when using array methods with exported function Apr 17, 2018
@apsavin
Copy link
Contributor

apsavin commented Apr 17, 2018

Why use any? You can use something like Array<string>.

See the changelog:

New missing annotation errors might be reported.

@lll000111
Copy link
Contributor

lll000111 commented Apr 17, 2018

Not an error, quite the opposite actually (it's quite deliberate).

You export a function — Flow expects you to document the function's return type. Your function has no return type annotation. I mean, the error message even tells you!

And use * instead of any, it's counter-productive and there is no reason to turn off type checking, instead let Flow autodetect it. Even better of course if you specify the actual type. Even if Flow can figure it out, human readers (incl. yourself two weeks later) appreciate the documentation a type annotation provides.

Fixed.

@villesau
Copy link
Contributor

@lll000111 The error message doesn't tell anything about missing return type. It says that Missing type annotation for 'U' How can you even know what is U without diving into flow core libdefs?

@lll000111
Copy link
Contributor

lll000111 commented Apr 17, 2018

@villesau

You say:

The error message doesn't tell anything about missing return type

The error message says

Missing type annotation for U.

I'd say "missing annotation" is quite clear. There is a missing annotation. You look at the function and you see — there is no annotation! You check your parameters and the return type, that's all that can be (and that must be) annotated. There is no ambiguity here. Okay, the "U"... so what? You don't need it. All you need is "Missing annotation". Ignore generic components. While it could be improved of course, I think a type system requires a bit of prerequisite knowledge, this isn't aimed at average users. This tool sure has more than enough unsolved issues (that nobody on the owner side even acknowledges, they seem to ignore this entire public "issues" section as far as I can tell), in comparison I'd put this error message quality issue at the bottom of the list.

@villesau
Copy link
Contributor

@lll000111 It asks you to annotate your U, something related to your map or filter, it doesn't say anything return type annotation related. That's the point. Flow rarely requires you to annotate your return type which doesn't make it obvious. When I tried to update to latest Flow, I couldn't even see the benefit of having to explicitly type it: Previously Flow was able to infer the type just fine. At least all the cases I tested. But that's maybe not strictly related to this issue.

@lll000111
Copy link
Contributor

lll000111 commented Apr 17, 2018

@villesau The key is the return statement. "Missing annotation" is only applicable to function signatures and variables.

In this context. related: #2667

Okay okay okay. By all means, let's have a better error message.

@vicapow
Copy link
Contributor

vicapow commented Apr 17, 2018

This seems like inconsistent behavior. Either all exports should require explicit annotations or non should as long as the implicit types are valid.

@kennethtruong
Copy link

I'm pretty sure something seems wrong with exporting anything using an array method.

Given this example https://flow.org/try

// @flow

const someArray: Array<string> = ['hello'];

// Error Here
const a = {
  someValue: someArray.map((x) => x)
  //         ^ Missing type annotation for `U`.
};

// No Errors Here
const b = {
  someValue: someArray
}

export const c = {
  a: a,
  b: b
};

@iainbeeston
Copy link
Contributor

I'm seeing this too. You don't need to wrap the array in an object, as you can see here. Happens in 0.7.0, but not 0.6.9. Adding a type annotation before exporting gets rid of the error

@schumannd
Copy link

So where do 'U', 'T' or 'S' come from? Are they just randomly generated letters? In that case it would be very helpful to replace them with 'exported function return type' or something like that.

@mrkev
Copy link
Contributor

mrkev commented Apr 23, 2018

@schumannd this for map, this for filter.

It's technically doing things as it should: you're returning an Array or an Array and it needs a type annotation for the type parameters of the returned array; the problem is it's a little cryptic since the user didn't directly define map or filter, so they wouldn't know what U or T are.

@schumannd
Copy link

schumannd commented Apr 24, 2018

Well yes, flow is throwing an error and thus doing things as it should.
If, however, the error message is not understandable, then there is definitely room for improvement.
Also, what are these letters? Are they the names of the parameters of map and filter? What about 'S'?

I am strongly in favor of doing something about this. Operations on arrays are among the most common things to appear in any code. These cryptic error messages will cause a lot of new users trouble.

@jfirebaugh
Copy link

I'm not sure it technically is doing the right thing. Return type annotations are not required on all exported methods -- that's easy to verify. So what's special about methods that use array generics?

jfirebaugh added a commit to mapbox/mapbox-gl-js that referenced this issue May 1, 2018
Not upgrading past that due to facebook/flow#6151.
jfirebaugh added a commit to mapbox/mapbox-gl-js that referenced this issue May 2, 2018
Not upgrading past that due to facebook/flow#6151.
jfirebaugh added a commit to mapbox/mapbox-gl-js that referenced this issue May 2, 2018
Not upgrading past that due to facebook/flow#6151.
jfirebaugh added a commit to mapbox/mapbox-gl-js that referenced this issue May 2, 2018
Not upgrading past that due to facebook/flow#6151.
jfirebaugh added a commit to mapbox/mapbox-gl-js that referenced this issue May 2, 2018
Not upgrading past that due to facebook/flow#6151.
@amypellegrini
Copy link

I don't understand this entirely, but if you do import * as React from "react"; instead of import React from "react"; flow doesn't complain anymore.

@JasonSooter
Copy link

@amypellegrini I ended up finding the same conclusion. I still don't completely understand but here is a screenshot that somewhat confirms it

components___flow

https://flow.org/en/docs/react/components/

@peeyush-ad2games
Copy link

We already have import * as React from 'react' in our file but it's still giving the error.

@JasonSooter
Copy link

@peeyush-ad2games What version of flow are you using?

@peeyush-ad2games
Copy link

@functionalStoic we are using 0.78.0

@woniesong92
Copy link

I also ran into the same issue. What's the status of this?

@innokenty
Copy link

So it's a great discussion, but how do we compile the code like that?

@amypellegrini
Copy link

@innokenty the usual setting is with Babel and Webpack, shout out if need more details.

gnprice pushed a commit to zulip/zulip-mobile that referenced this issue Sep 21, 2018
Flow v0.75 (which we'll upgrade to with RN v0.55) requires an
explicit annotation on types in "input position" -- which
includes the type parameter to `Array<>` -- in module exports.

See discussion in these issue threads (the closest thing to
documentation that exists, as is typical for Flow):
  facebook/flow#6828
  facebook/flow#6151
@kevindice
Copy link

kevindice commented Nov 30, 2018

I hope this will help someone out: map is typed as a generic. You want to provide something in the place of U here, as in someArray.map<U>(someMapperFunction) where someMapperFunction is of type T => U.

An example:

dictToOptions = (options: {[string]: string}) => {
  const mapper = (key: string): React.Node => (
    <option key={key} value={options[key]}>{options[key]}</option>
  )
  return Object.keys(options).map<React.Node>(mapper)
}

@luislhl
Copy link

luislhl commented Jan 28, 2019

I'm trying to upgrade my version to 0.91.0 and also having problems with map functions.

Maps like this are causing me the errors:

  renderDescriptions() {
    return this.props.products.map((item, index) => (
      <ol
        key={`description-${index}`}
      >
        {this.renderDescriptionItem(item.descriptions)}
      </ol>
    ));
  }

Changing to this solves the problem:

  renderDescriptions() {
    const descriptions: React.Element<'ol'>[] = this.props.products.map((item, index) => (
      <ol
        key={`description-${index}`}
      >
        {this.renderDescriptionItem(item.descriptions)}
      </ol>
    ));

    return descriptions;
  }

But i'm still curious about something.

There is a Medium post by Flow about this change, in which they state:

Note that builtin generic types like Map, Set, and Promise are not affected. Builtin types are merged early in the analysis, so Flow already complains about missing annotations when their instances reach exports.

So why are we getting errors in built-in map functions, like the one above?

My this.props.products variable is a built-in Array.

@kevindice
Copy link

@luislhl In your first example...

Flow knows that .map() always returns an array but doesn't know the type of the elements of that array. It could be anything. This information needs to either be explicitly provided like this this.props.products.map<React.Node>( ... or provided in a way that Flow can infer this information.

In your second code block, you specify the type of descriptions as being Array<React.Element>, so Flow can infer that this.props.products.map( ... will return this type.

@resgar
Copy link

resgar commented May 21, 2019

With an array of ObjectType:

type ObjectType = {
  id: number,
};

using .map<ObjectType>, I got the following error:

property id is missing in React.Element [1] but exists in ObjectType [2] in the return value.

I fixed this error using: .map<{}>

@goodmind
Copy link
Contributor

I think if you use $ReadOnlyArray it doesn't require annotation

@radar
Copy link

radar commented Sep 10, 2019

I ran into this issue today with the following code:

 configuredFlags() {
    return this.props.availableFlags.filter((flag: string) => {
      return this.initialFlagStatus(flag) !== 'default';
    });
  }

The solution was to define a return type for the configuredFlags function:

 configuredFlags(): Array<string> {
    return this.props.availableFlags.filter((flag: string) => {
      return this.initialFlagStatus(flag) !== 'default';
    });
  }

@danielo515
Copy link

Today I ran on this weird error.

I had this:

type Response = {
  viewer: { paymentRequests: $ReadOnlyArray<PaymentRequest> },
}

export default (companyID: string) => {
  const { data, ...info } = useQuery<Response>(  )
  return {
    ...info,
    data: {
      paymentRequests:  data.viewer.paymentRequests
            .slice()
            .sort(byPayDateDesc)
    },
  }
}

And I was getting this error:

Missing type annotation for T. Tis a type parameter declared in read-only array type [1] and was implicitly instantiated at call of methodslice [2].Flow(InferError)

Luckly I was able to properly "Cast" by doing this:

    data: {
      paymentRequests: data
        ? (data.viewer.paymentRequests
            .slice()
            .sort(byPayDateDesc): PaymentRequest[])
        : data,
    },

But I don't understand why flow was not capable of inferring what was already declared, slice should not change the data in any way, and it doesn't allow you to provide parameters, so this was kind of a dead end until I came to this funny cast.

@SamChou19815
Copy link
Contributor

We now requires annotation on exported stuff unconditionally.

@SamChou19815 SamChou19815 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests