plpgsql unreachable code (was BUG #1329: Bug in IF-ELSEIF-ELSE construct)

Neil Conway wrote:
> (BTW, another thing this example exposes is that we don't issue warnings
> about trivially-dead-code, such as statements in a basic block that
> follow a RETURN. This would probably be also worth doing.)

Attached is a patch that implements this. Specifically, if there are any
statements in the same block that follow a RETURN, EXIT (without
condition) or RAISE EXCEPTION statement, we issue a warning at CREATE
FUNCTION time:

create function exit_warn() returns int as $$
declare x int;
begin
     x := 5;
     loop
         x := x + 1;
         exit;
         x := x + 2;
     end loop;
     x := x + 3;
     return x;
end;$$ language 'plpgsql';
WARNING:  assignment is unreachable, due to exit near line 6
CONTEXT:  compile of PL/pgSQL function "exit_warn" near line 7

No warning is issued if check_function_bodies is false.

AFAICS there is no current infrastructure for walking a PL/PgSQL
function's parse tree, so I did this manually (which is easy enough, of
course). In the future it might be a good idea to refactor this to use
something akin to the "walker" infrastructure in the backend (for one
thing the PL/PgSQL function dumping code could use this as well).

(BTW this patch is intended for 8.1, of course.)

-Neil
--- src/pl/plpgsql/src/pl_comp.c
+++ src/pl/plpgsql/src/pl_comp.c
@@ -130,6 +130,7 @@
 static void plpgsql_HashTableInsert(PLpgSQL_function *function,
                         PLpgSQL_func_hashkey *func_key);
 static void plpgsql_HashTableDelete(PLpgSQL_function *function);
+static void check_function(PLpgSQL_function *function);

 /*
  * This routine is a crock, and so is everyplace that calls it.  The problem
@@ -152,8 +153,10 @@
 /* ----------
  * plpgsql_compile        Make an execution tree for a PL/pgSQL function.
  *
- * If forValidator is true, we're only compiling for validation purposes,
- * and so some checks are skipped.
+ * If forValidator is true, we're only compiling for validation
+ * purposes; this means we skip some checks, as well as making some
+ * additional compile-time checks that we only want to do once (at
+ * function definition time), not very time the function is compiled.
  *
  * Note: it's important for this to fall through quickly if the function
  * has already been compiled.
@@ -293,7 +296,7 @@
      * Setup error traceback support for ereport()
      */
     plerrcontext.callback = plpgsql_compile_error_callback;
-    plerrcontext.arg = forValidator ? proc_source : (char *) NULL;
+    plerrcontext.arg = forValidator ? proc_source : NULL;
     plerrcontext.previous = error_context_stack;
     error_context_stack = &plerrcontext;

@@ -595,7 +598,7 @@
     plpgsql_add_initdatums(NULL);

     /*
-     * Now parse the functions text
+     * Now parse the function's text
      */
     parse_rc = plpgsql_yyparse();
     if (parse_rc != 0)
@@ -605,7 +608,7 @@
     pfree(proc_source);

     /*
-     * If that was successful, complete the functions info.
+     * If that was successful, complete the function's info.
      */
     function->fn_nargs = procStruct->pronargs;
     for (i = 0; i < function->fn_nargs; i++)
@@ -616,12 +619,22 @@
         function->datums[i] = plpgsql_Datums[i];
     function->action = plpgsql_yylval.program;

+    /*
+     * Perform whatever additional compile-time checks we can. Note
+     * that we only do this when validating the function; this is so
+     * that (a) we don't bother the user with warnings when the
+     * function is invoked (b) we don't take the performance hit of
+     * doing the analysis more than once per function definition.
+     */
+    if (forValidator)
+        check_function(function);
+
     /* Debug dump for completed functions */
     if (plpgsql_DumpExecTree)
         plpgsql_dumptree(function);

     /*
-     * add it to the hash table
+     * Add it to the hash table
      */
     plpgsql_HashTableInsert(function, hashkey);

@@ -664,8 +677,149 @@
                    plpgsql_error_funcname, plpgsql_error_lineno);
 }

+/*
+ * Emit a warning that the statement 'unreach' is unreachable, due to
+ * the effect of the preceding statement 'cause'.
+ */
+static void
+report_unreachable_stmt(PLpgSQL_stmt *unreach, PLpgSQL_stmt *cause)
+{
+    /*
+     * XXX: adjust the line number that is emitted along with the
+     * warning message. This is a kludge.
+     */
+    int old_lineno = plpgsql_error_lineno;
+    plpgsql_error_lineno = unreach->lineno;

+    elog(WARNING, "%s is unreachable, due to %s near line %d",
+         plpgsql_stmt_typename(unreach),
+         plpgsql_stmt_typename(cause),
+         cause->lineno);
+
+    plpgsql_error_lineno = old_lineno;
+}
+
 /*
+ * Given a list of PL/PgSQL statements, perform some compile-time
+ * checks on them.
+ *
+ * Note that we return as soon as we have emitted "unreachable"
+ * warnings for a given sequence of statements. So that given:
+ *
+ *    EXIT; EXIT; EXIT
+ *
+ * we will see the first "EXIT", issue warnings for the second and
+ * third unreachable EXIT statements, and then return, so that we
+ * don't issue bogus "unreachable" warnings when we see the second
+ * EXIT.
+ *
+ * XXX: currently we walk the PL/PgSQL execution tree by hand. It
+ * would probably be worth refactoring this to use something akin to
+ * the tree walker infrastructure in the backend.
+ */
+static void
+check_stmts(PLpgSQL_stmts *stmts)
+{
+    int i;
+
+    for (i = 0; i < stmts->stmts_used; i++)
+    {
+        PLpgSQL_stmt *stmt = stmts->stmts[i];
+        int j;
+
+        switch (stmt->cmd_type)
+        {
+            case PLPGSQL_STMT_RETURN:
+                for (j = i + 1; j < stmts->stmts_used; j++)
+                    report_unreachable_stmt(stmts->stmts[j], stmt);
+
+                return;
+            case PLPGSQL_STMT_EXIT:
+                {
+                    /*
+                     * If the EXIT statement has a conditional, it is
+                     * not guaranteed to exit the loop, so don't issue
+                     * a warning.
+                     */
+                    PLpgSQL_stmt_exit *exit_stmt = (PLpgSQL_stmt_exit *) stmt;
+                    if (exit_stmt->cond == NULL)
+                    {
+                        for (j = i + 1; j < stmts->stmts_used; j++)
+                            report_unreachable_stmt(stmts->stmts[j], stmt);
+
+                        return;
+                    }
+                }
+                break;
+            case PLPGSQL_STMT_RAISE:
+                {
+                    PLpgSQL_stmt_raise *raise_stmt = (PLpgSQL_stmt_raise *) stmt;
+                    /*
+                     * Only RAISE EXCEPTION (converted to elog_level =
+                     * ERROR by the parser) will exit the current
+                     * block.
+                     */
+                    if (raise_stmt->elog_level == ERROR)
+                    {
+                        for (j = i + 1; j < stmts->stmts_used; j++)
+                            report_unreachable_stmt(stmts->stmts[j], stmt);
+
+                        return;
+                    }
+                }
+                break;
+            case PLPGSQL_STMT_BLOCK:
+                {
+                    PLpgSQL_stmt_block *block_stmt = (PLpgSQL_stmt_block *) stmt;
+                    check_stmts(block_stmt->body);
+
+                    if (block_stmt->exceptions)
+                    {
+                        for (j = 0; j < block_stmt->exceptions->exceptions_used; j++)
+                            check_stmts(block_stmt->exceptions->exceptions[j]->action);
+
+                        return;
+                    }
+                }
+                break;
+            case PLPGSQL_STMT_IF:
+                check_stmts(((PLpgSQL_stmt_if *) stmt)->true_body);
+                check_stmts(((PLpgSQL_stmt_if *) stmt)->false_body);
+                break;
+            case PLPGSQL_STMT_LOOP:
+                check_stmts(((PLpgSQL_stmt_loop *) stmt)->body);
+                break;
+            case PLPGSQL_STMT_WHILE:
+                check_stmts(((PLpgSQL_stmt_while *) stmt)->body);
+                break;
+            case PLPGSQL_STMT_FORI:
+                check_stmts(((PLpgSQL_stmt_fori *) stmt)->body);
+                break;
+            case PLPGSQL_STMT_FORS:
+                check_stmts(((PLpgSQL_stmt_fors *) stmt)->body);
+                break;
+            case PLPGSQL_STMT_DYNFORS:
+                check_stmts(((PLpgSQL_stmt_dynfors *) stmt)->body);
+                break;
+            default:
+                /* do nothing */
+                break;
+        }
+    }
+}
+
+/*
+ * Issue warnings about various ill-advised constructs in the function
+ * body. We don't do very many compile-time checks at the moment, but
+ * a few is better than none...
+ */
+static void
+check_function(PLpgSQL_function *function)
+{
+    check_stmts(function->action->body);
+}
+
+/*
  * Fetch the argument names, if any, from the proargnames field of the
  * pg_proc tuple.  Results are palloc'd.
  */
--- src/test/regress/expected/plpgsql.out
+++ src/test/regress/expected/plpgsql.out
@@ -2002,3 +2002,111 @@
 DETAIL:  Key (f1)=(2) is not present in table "master".
 drop function trap_foreign_key(int);
 drop function trap_foreign_key_2();
+--
+-- issue warnings about unreachable code
+--
+create function exit_warn() returns int as $$
+declare x int;
+begin
+    x := 5;
+    loop
+        x := x + 1;
+        exit;
+        x := x + 2;
+    end loop;
+    x := x + 3;
+    return x;
+end;$$ language 'plpgsql';
+WARNING:  assignment is unreachable, due to exit near line 6
+CONTEXT:  compile of PL/pgSQL function "exit_warn" near line 7
+create function exit_no_warn() returns int as $$
+declare x int;
+begin
+    x := 5;
+    loop
+        x := x + 5;
+        exit when x > 20;
+        x := x - 1;
+    end loop;
+    x := x + 3;
+    return x;
+end;$$ language 'plpgsql';
+create function return_warn() returns int as $$
+declare
+    a int;
+    b int;
+begin
+    a := 3;
+    b := 2;
+    if a > b then
+        return 5;
+        begin
+            return 10;
+        end;
+    end if;
+    return 15;
+end;$$ language 'plpgsql';
+WARNING:  block variables initialization is unreachable, due to return near line 8
+CONTEXT:  compile of PL/pgSQL function "return_warn" near line 9
+create function return_warn2() returns int as $$
+begin
+    return 10;
+    return 15;
+    return 20;
+end;$$ language 'plpgsql';
+WARNING:  return is unreachable, due to return near line 2
+CONTEXT:  compile of PL/pgSQL function "return_warn2" near line 3
+WARNING:  return is unreachable, due to return near line 2
+CONTEXT:  compile of PL/pgSQL function "return_warn2" near line 4
+create function raise_warn() returns int as $$
+declare x int;
+begin
+    x := 10;
+    begin
+        raise exception 'some random exception';
+        return x;
+    exception
+        when RAISE_EXCEPTION then NULL;
+    end;
+
+    begin
+        raise exception 'foo';
+    exception
+        when RAISE_EXCEPTION then
+            return x;
+            x := x + 1; -- unreachable
+    end;
+
+    begin
+        raise notice 'not an exception';
+        return x;
+    end;
+end;$$ language 'plpgsql';
+WARNING:  return is unreachable, due to raise near line 5
+CONTEXT:  compile of PL/pgSQL function "raise_warn" near line 6
+-- note that we only want to emit warnings at CREATE FUNCTION time, not
+-- when the function is invoked.
+SELECT exit_warn();
+ exit_warn
+-----------
+         9
+(1 row)
+
+SELECT return_warn();
+ return_warn
+-------------
+           5
+(1 row)
+
+SELECT return_warn2();
+ return_warn2
+--------------
+           10
+(1 row)
+
+SELECT raise_warn();
+ raise_warn
+------------
+         10
+(1 row)
+
--- src/test/regress/sql/plpgsql.sql
+++ src/test/regress/sql/plpgsql.sql
@@ -1746,3 +1746,87 @@

 drop function trap_foreign_key(int);
 drop function trap_foreign_key_2();
+
+--
+-- issue warnings about unreachable code
+--
+create function exit_warn() returns int as $$
+declare x int;
+begin
+    x := 5;
+    loop
+        x := x + 1;
+        exit;
+        x := x + 2;
+    end loop;
+    x := x + 3;
+    return x;
+end;$$ language 'plpgsql';
+
+create function exit_no_warn() returns int as $$
+declare x int;
+begin
+    x := 5;
+    loop
+        x := x + 5;
+        exit when x > 20;
+        x := x - 1;
+    end loop;
+    x := x + 3;
+    return x;
+end;$$ language 'plpgsql';
+
+create function return_warn() returns int as $$
+declare
+    a int;
+    b int;
+begin
+    a := 3;
+    b := 2;
+    if a > b then
+        return 5;
+        begin
+            return 10;
+        end;
+    end if;
+    return 15;
+end;$$ language 'plpgsql';
+
+create function return_warn2() returns int as $$
+begin
+    return 10;
+    return 15;
+    return 20;
+end;$$ language 'plpgsql';
+
+create function raise_warn() returns int as $$
+declare x int;
+begin
+    x := 10;
+    begin
+        raise exception 'some random exception';
+        return x;
+    exception
+        when RAISE_EXCEPTION then NULL;
+    end;
+
+    begin
+        raise exception 'foo';
+    exception
+        when RAISE_EXCEPTION then
+            return x;
+            x := x + 1; -- unreachable
+    end;
+
+    begin
+        raise notice 'not an exception';
+        return x;
+    end;
+end;$$ language 'plpgsql';
+
+-- note that we only want to emit warnings at CREATE FUNCTION time, not
+-- when the function is invoked.
+SELECT exit_warn();
+SELECT return_warn();
+SELECT return_warn2();
+SELECT raise_warn();