facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.08k stars 1.86k forks source link

Type annotations break type safety #1262

Closed aldendaniels closed 8 years ago

aldendaniels commented 8 years ago

Warns (Correct):

const noop = (n) => n;
const num:number = noop('NOT A NUMBER');
// "String. This type is incompatible with number"

Passes (Incorrect):

const noop = (n:any) => n;
const num:number = noop('NOT A NUMBER');
// "Found 0 errors"
samwgoldman commented 8 years ago

This isn't a bug. By annotating n as any, you're bypassing the type checker. any is explicitly designed to allow unsafe casting, which is happening here. See more in the documentation.

Maybe you wanted to use a polymorphic function type?

/* @flow */
const noop = <T>(n: T): T => n;
const num: number = noop('NOT A NUMBER');
test.js:3
  3: const num: number = noop('NOT A NUMBER');
                         ^^^^^^^^^^^^^^^^^^^^ string. This type is incompatible with
  3: const num: number = noop('NOT A NUMBER');
                ^^^^^^ number
aldendaniels commented 8 years ago

@samwgoldman - Thanks for the quick reply!

So basically (there's probably a more accurate way of saying this), Flow's type inference is disabled at the boundary of annotated functions.

I realize that a polymorphic function would cover this trivial example, but it wouldn't have helped with the real function I was dealing with. Flow's ability to intelligently perform static type inference is impressive to say the least. It seams a pity that this intelligence is turned off for annotated functions.

My use-case involved the usage of Object.assign. Flow understands Object.assign and warns against invalid property assignments. Great! But if the assignment occurs inside an annotated function and operates on parameter variables, Flow trusts the annotation and lets the non-type-safe behavior slide.

The internal $shape type you pointe me to in #1263 will solve that use case, but not all use cases. Does Flow (or should Flow) allow users to describe a type annotation which adds constraints without disabling Flow's normal type inference?

samwgoldman commented 8 years ago

So basically (there's probably a more accurate way of saying this), Flow's type inference is disabled at the boundary of annotated functions.

I'm not sure I would say that the type inference is "disabled." Inference, by definition, comes in when annotations are absent. When annotations are present, inference just doesn't apply. Again, this is by definition, not design.

Maybe I'm just moving goalposts here, though. It seems like you have some useful feedback here, and I want to make sure I'm not losing you in semantics. Ignoring the above for a moment, let's address the rest of your comment.

Flow understands Object.assign and warns against invalid property assignments. Great! But if the assignment occurs inside an annotated function and operates on parameter variables, Flow trusts the annotation and lets the non-type-safe behavior slide.

I wouldn't say this is an issue with annotations in themselves, but I do agree that, currently, there are some types which Flow can infer for which there is no possible annotation. Object.assign is a good example of that.

/* @flow */
type T = { foo: string, bar: number };
let t: T = Object.assign({}, { foo: "" }, { bar: 0 });

The above code is totally acceptable to Flow, but it wouldn't be possible to write a module that provides this function. You could write a local function, because local functions don't require annotations, but we do require annotations at module boundaries, so the annotation expressivity issue comes into play there.

One of my mid-term goals with Flow is to provide more type-level constructs to provide type safety to idiomatic JS patterns which aren't currently expressible. I don't think there is a quick/easy fix for this, but the easiest way for you and the rest of the community to help is to provide examples, to help guide development.

Until these new features roll out, I'm happy to help you work around any issues you come across.

aldendaniels commented 8 years ago

You've exactly understood my concern - and articulated it better than I could have. Flow annotations cannot express some type-level constructs which the Flow type checker is able to infer. An object merge (Object.assign) is one such example.

Certainly new annotations could be added for common cases. Flow could add a type annotation describing the merging of multiple objects. Theoretically, Flow could perform static validation to help ensure that object merging is occurring as described. This would be great. Short of automated theorem proving, however, the extent and flexibility of such type annotations will be limited.

For a given function, the type annotation describes a subset of what could be described about the function's inputs and return value. Take this contrived function:

function prepForSerialize(o: Object) : Object {
  let ret = {};
  for (let key in o) {
    if (o.hasOwnProperty(key) && o[key] instanceof RegExp) {
       ret[key] = `/${o[key].source}/`;
    }
  }
  return ret;
}

type MyType = {r: Object};
let thing:MyType = {r: /abc/};
thing = prepForSerialize(thing);

A sophisticated type inference model could, in theory, realize that the above code will result in thing.r being a string instead of an object, which is an error. The type annotation (Object) is true and useful, but it's incomplete and is insufficient to reveal the error.

Is it true, then, that "Inference, by definition, comes in when annotations are absent"? In the above example, couldn't one infer the error on the last line while also respecting the prepForSerialize() type annotation? Over my head here, so I hope I'm making sense.

Until these new features roll out, I'm happy to help you work around any issues you come across.

Thank you! I really appreciate your help.