--- 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();