facebook / flow

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

Pure functions #2715

Open CGamesPlay opened 7 years ago

CGamesPlay commented 7 years ago

This is a feature request for marking functions as not having any side effects, aka being pure.

Flow reports the following safe code as unsafe:

/* @flow */
type T = { func: ?(number) => void };
function f() { return 7; }
function g(val: T) {
  if (val.func) {
    val.func(f());
  }
}

The reasoning for this is because Flow has no idea who else has a reference to val, and the call to f might modify the value of val.func, so the previous non-null assertion is invalidated.

It would be nice to be able to mark functions as pure. Flow could assert that:

As a straw man syntax for this, I suggest pure before the return type in the type definition:

type APureFunction: () => pure void
function aPureFunction(): pure void { }

I don't think this is a duplicate of #18 because that focuses on data immutability, which is different from function purity.

bradennapier commented 6 years ago

Just ran into this as well. Especially when it comes to using %checks value as it means that every time you use any %checks function you have to redo every single refine you have already done.

For example, when I do something like this:

function isObjLiteral<+O>(obj: O): boolean %checks {
  return obj != null && typeof obj === 'object' && !Array.isArray(obj);
}

Any properties underneath a call to this is invalid

image

whereas if I put that on the bottom it is fine. Meaning I would have to refine the fact p.d exists every single property which is crazy inefficient.

image

jcready commented 6 years ago

@bradennapier it seems like predicate functions (using %checks) only cause previous refinements to be invalidated if they are called in a different statement from the original refinement (Try Flow)

const isArr = (x: mixed): boolean %checks => Array.isArray(x)
const isObj = (x: mixed): boolean %checks => typeof x === 'object' && x !== null && !isArr(x)
const isStr = (x: mixed): boolean %checks => typeof x === 'string'
const isNum = (x: mixed): boolean %checks => typeof x === 'number'
const isBool = (x: mixed): boolean %checks => typeof x === 'boolean'

const test = (x: { [string]: mixed }): string => {
  if (typeof x.foo === 'string') {
    if (typeof x.bar === 'string') {
      return x.foo // passes
    }
  }

  // Predicate functions cause refinement invalidations if in different statements
  if (typeof x.foo === 'string') {
    if (isStr(x.bar)) {
      return x.foo // fails
    }
  }

  if (isStr(x.foo)) {
    if (isStr(x.bar)) {
      return x.foo // fails
    }
  }

  if (isStr(x.foo)) {
    if (typeof x.bar === 'string') {
      return x.foo // passes
    }
  }

  // Both refinements in the same statement do not cause refinement invalidation
  if (typeof x.foo === 'string' && isStr(x.bar)) {
    return x.foo // passes
  }

  if (isStr(x.foo) && isStr(x.bar)) {
    return x.foo // passes
  }

  return ''
}
bradennapier commented 6 years ago

Yeah, which is the problem. "Pure" functions as a concept would relieve this as you are essentially promising Fllow that no mutations or side effects will occur when a given function is called, so it should not invalidate refinements when called.

I would think %checks would default to being "pure" since its a simple boolean operation whereas there would be a way to mark standard functions as pure (perhaps jsut with %pure in place of %checks).

jcready commented 6 years ago

But do you not find it odd that the refinements are not invalidated if you just put the function call in the same statement?

if (typeof x.foo === 'string' && isStr(x.bar)) {
  return x.foo // passes
}
bradennapier commented 6 years ago

Yeah, I think that this may explain a few of the bugs going on right now with refinement actually. Blindly adding some refinement topics here for interlinking

3918 #4556 #4874 #4981 #5016 #5312

AlexJWayne commented 6 years ago

5441 also

kevinbarabash commented 5 years ago

This issue seems to be focused on side-effects that mutate data somewhere. Pure functions in general aren't supposed to have any side-effects, e.g. throwing exceptions, logging to the console, drawing to the canvas, etc. I think we should only mark functions as mutating or not.

rattrayalex commented 5 years ago

I would support a %pure annotation that allowed throwing exceptions and/or logging to the console, though drawing to the canvas (or DOM) should not happen in a "pure" function IMO.

For me the value is less about preserving refinements, and more about readable code: I want to know which call trees can mutate state/the world (eg; write to the dom, send network requests) so that I can clearly identify the ones which can, and isolate them. Keeping a high percentage of code non-mutative helps keep systems understandable and safe, and without a feature like this, it's hard to do.

As a bit of an aside, I consider there to be "read-purity" and "write-purity". So far we've discussed "write-purity" (not mutating other state) but not "read-purity" (not accessing state outside of function params). I'd also like to see a way of annotating functions as read-pure, so that I know that they don't rely on information I don't provide them directly. It'd be nice to have a distinction, though, since I might want a program flow like "read data through I/O, but do not mutate" => "process data without I/O" => ''write to I/O, but do not read" and would like to guarantee the nature of these steps

kevinbarabash commented 5 years ago

Totally agree that tracking global mutations leads to code that easier to understand and maintain. console.log output might be an important thing to track for those people working on CLI applications. Having some way to configure what kinds of mutations a person cares about might be useful.

rattrayalex commented 5 years ago

Agreed, being able to configure whether you care about things like logging would be ideal!