unreachable code for function declarations

[timruffles]

Function declarations are not flow friendly. Since they're hoisted to the top of the scope people use them to keep helpers separate from what the function is 'doing', with access to its params e.g:

/* @flow */

function foo(a) {

  return helper() + 1;

  function helper() {
    return a * a;
  }
}

gives

$ flow check
./test.js:5:3,22:
unreachable code

[avikchaudhuri]

Interesting observation. We should support this idiom.

[rmg]

This was the first (and only!) error reported on my very first run of flow.

[tommilimmot]

Would be nice to have this supported!

[XrXr]

+1

[karelbilek]

+1

[ghengeveld]

+1

In fact this idiom is recommended in John Papa's AngularJS styleguide: https://github.com/johnpapa/angular-styleguide#function-declarations-to-hide-implementation-details

[davidmason]

I am happy to have a try at implementing this in my spare time.

@avikchaudhuri Can you point me to the right area in the code that I should be looking if I want to make this sort of change?

[samwgoldman]

@davidmason I am pretty sure you want to look at the code that processes statements in the type inference code.

For a bit of background, check out the Abnormal module.

A heavy handed approach would be to actually iterate through the remaining statements, and only complain about unreachable code if there are any non-hoisted statements (like VariableDeclaration and FunctionDeclaration—there might be more that hoists).

[davidmason]

@samwgoldman thanks, I'll take a look at those.

(like VariableDeclaration and FunctionDeclaration—there might be more that hoists).

I'll also try to check whether a VariableDeclaration is part of an assignment. The variable declaration is hoisted, but the assignment part of the statement will still never run so should still be flagged as unreachable code.

[avikchaudhuri]

Yeah this strategy should work.

[samwgoldman]

@davidmason Are you working on this?

[davidmason]

@samwgoldman yes, had to study up a bit on OCaml. I made some changes, I just need to get the build working to check if it works.

I'm on Fedora 20. I did:

yum install ocaml elfutils-libelf-devel

[]$ ocamlc -v
The OCaml compiler, version 4.00.1
Standard library directory: /usr/lib64/ocaml

[]$ yum info ocaml
Installed Packages
Name        : ocaml
Arch        : x86_64
Version     : 4.00.1
Release     : 3.fc20
Size        : 28 M
Repo        : installed
From repo   : fedora
Summary     : OCaml compiler and programming environment
URL         : http://www.ocaml.org
License     : QPL and (LGPLv2+ with exceptions)
Description : OCaml is a high-level, strongly-typed, functional and object-oriented
            : programming language from the ML family of languages.
            : 
            : This package comprises two batch compilers (a fast bytecode compiler
            : and an optimizing native-code compiler), an interactive toplevel system,
            : parsing tools (Lex,Yacc,Camlp4), a replay debugger, a documentation generator,
            : and a comprehensive library.

[]$ yum info elfutils-libelf-devel
Loaded plugins: langpacks, refresh-packagekit
Installed Packages
Name        : elfutils-libelf-devel
Arch        : x86_64
Version     : 0.161
Release     : 6.fc20
Size        : 26 k
Repo        : installed
From repo   : updates
Summary     : Development support for libelf
URL         : https://fedorahosted.org/elfutils/
License     : GPLv2+ or LGPLv3+
Description : The elfutils-libelf-devel package contains the libraries to create
            : applications for handling compiled objects.  libelf allows you to
            : access the internals of the ELF object file format, so you can see the
            : different sections of an ELF file.

[]$ make
mkdir -p bin
tar czf bin/flowlib.tar.gz lib
echo "const char* const BuildInfo_kRevision = \"$(git rev-parse HEAD)\";" > hack/utils/get_build_id.gen.c
ocamlbuild -ocamlc "ocamlopt   -ccopt -DNO_LZ4"\
    src/embedded/flowlib_elf.o hack/heap/hh_shared.o hack/utils/realpath.o hack/third-party/inotify/inotify_stubs.o hack/utils/nproc.o hack/hhi/hhi_elf.o hack/utils/get_build_id.gen.o hack/utils/get_build_id.o
Finished, 16 targets (16 cached) in 00:00:00.
ocamlbuild  -no-links  -I src/commands -I src/common -I src/dts -I src/embedded -I src/parser -I src/parsing -I src/server -I src/stubs -I src/typing -I hack/deps -I hack/dfind -I hack/globals -I hack/heap -I hack/parsing -I hack/procs -I hack/search -I hack/socket -I hack/stubs -I hack/third-party/avl -I hack/third-party/core -I hack/utils -I hack/third-party/inotify -I hack/fsnotify_linux -lib unix -lib str -lflags "src/embedded/flowlib_elf.o hack/heap/hh_shared.o hack/utils/realpath.o hack/third-party/inotify/inotify_stubs.o hack/utils/nproc.o hack/hhi/hhi_elf.o hack/utils/get_build_id.gen.o hack/utils/get_build_id.o -cclib -l -cclib elf   " src/flow.native
+ /usr/bin/ocamlc.opt -c -w A -warn-error A -w -3-27 -w -27 -w -4-6-29-35-44-48 -w -3 -I hack/utils -I src/parsing -I src/commands -I src/embedded -I src/typing -I src/dts -I src/common -I src/server -I src/stubs -I src/parser -I hack/dfind -I hack/parsing -I hack/search -I hack/socket -I hack/deps -I hack/fsnotify_linux -I hack/stubs -I hack/procs -I hack/heap -I hack/globals -I hack/third-party/inotify -I hack/third-party/core -I hack/third-party/avl -o hack/utils/utils.cmo hack/utils/utils.ml
File "hack/utils/utils.ml", line 360, characters 17-21:
Error: Unbound value |>
Command exited with code 2.
Compilation unsuccessful after building 7 targets (6 cached) in 00:00:00.
make: *** [build-flow] Error 10

[samwgoldman]

@davidmason Great! It looks like you need a newer version of OCaml. The Flow source code uses |>, the reverse application operator, which is only available in OCaml since 4.01 (you have 4.00). Let me know if you have any more issues after you upgrade to 4.01 (or 4.02).

[davidmason]

@samwgoldman that got it compiling. I'll just need to iterate a bit on what I have and get the behaviour right.

[davidmason]

I've stalled a little on this - just had several really busy weeks and weekends. Looks to be calming down now though, so I'll aim to make the time this weekend and get it finished.

[davidmason]

@samwgoldman @avikchaudhuri Do I need to do something in the PR after agreeing to the CLA to have it pick that up?

[samwgoldman]

Fixed in 84ae36d05049f31b1089c1fa33060a2b3ea4b8f8