Re: [PATCH] pkl,doc,testsuite: Add `assert` statement

[Jose E. Marchesi]

Hi Mohammad.


> diff --git a/libpoke/pkl-rt.pk b/libpoke/pkl-rt.pk
> index 935c4e28..43c45347 100644
> --- a/libpoke/pkl-rt.pk
> +++ b/libpoke/pkl-rt.pk
> @@ -86,6 +86,7 @@ var EC_signal        = 12;
>  var EC_io_flags      = 13;
>  var EC_inval         = 14;
>  var EC_exit          = 15;
> +var EC_assert        = 16;
>
>
>  /* Standard exceptions.  */
>  
> @@ -121,6 +122,8 @@ var E_inval
>    = Exception {code = EC_inval, msg = "invalid argument", exit_status = 1};
>  var E_exit
>    = Exception {code = EC_exit, msg = "exit", exit_status = 0};
> +var E_assert
> +  = Exception {code = EC_assert, msg = "assertion failure", exit_status = 1};

You also need to add PVM_E_ASSERT_* definitions in pvm.h.

>  /* Default exception handler.
>  
> @@ -140,6 +143,18 @@ fun _pkl_exception_handler = (Exception exception) 
> int<32>:
>     return exception.exit_status;
>    }
>  
> +fun _pkl_assert = (uint<64> cond, string msg, string lineinfo) void:
> +  {
> +    if (cond)
> +      return;
> +
> +    raise Exception {
> +      code = EC_assert,
> +      msg = "assertion failed at " + lineinfo + (msg'length ? ": " + msg : 
> ""),
> +      exit_status = 1,
> +    };
> +  }
> +

Please add a comment before _pkl_assert explaining how it is called.

> diff --git a/libpoke/pkl-tab.y b/libpoke/pkl-tab.y
> index ba3ff6e9..bc5f4e75 100644
> --- a/libpoke/pkl-tab.y
> +++ b/libpoke/pkl-tab.y
> @@ -120,6 +120,62 @@ pkl_register_arg (struct pkl_parser *parser, 
> pkl_ast_node arg)
>    return 1;
>  }
>  

Missing comment here, explaining what the function does and what it
returns.

> +static pkl_ast_node
> +pkl_make_assertion(struct pkl_parser *p, pkl_ast_node cond, pkl_ast_node msg,
> +  struct pkl_ast_loc stmt_loc)

GNU style please.  Space between assertion and (, indent the second
line, etc.

> +{
> +  pkl_ast_node id, varinit, var, call, arg_cond, arg_msg, arg_lineinfo;
> +  pkl_ast_node lineinfo, asrt;
> +  int back, over;
> +  char buf[128];
> +  const char *name = "_pkl_assert";
> +

Maybe a comment here explaining this is looking up the _pkl_assert that
is defined in the compiler run-time?

> +  id = pkl_ast_make_identifier (p->ast, name);
> +  varinit = pkl_env_lookup (p->env, PKL_ENV_NS_MAIN, name, &back, &over);
> +  if (!varinit || (PKL_AST_DECL_KIND (varinit) != PKL_AST_DECL_KIND_FUNC))
> +    {
> +      pkl_error (p->compiler, p->ast, stmt_loc,
> +                 "undefined function '%s'", name);
> +      return NULL;
> +    }
> +  var = pkl_ast_make_var (p->ast, id, varinit, back, over);
> +  PKL_AST_LOC (var) = PKL_AST_LOC (id);
> +

Maybe a comment here explaining it is mounting the `lineinfo' argument
to _pkl_assert, that follows this format FILE:LINENO:COLUMNNO ?

> +  snprintf (buf, sizeof (buf), "%s:%d:%d",
> +            p->filename ? p->filename : "<stdin>", stmt_loc.first_line,
> +            stmt_loc.first_column);

Please use asprintf instead of introducing the arbitrary limitation in
buf[128].

> +  lineinfo = pkl_ast_make_string (p->ast, buf);
> +  PKL_AST_TYPE (lineinfo) = ASTREF (pkl_ast_make_string_type (p->ast));
> +  PKL_AST_LOC (lineinfo) = stmt_loc;
> +  PKL_AST_LOC (PKL_AST_TYPE (lineinfo)) = stmt_loc;
> +

Maybe a comment here like "Mount the rest of the arguments"?

> +  arg_cond = pkl_ast_make_funcall_arg (p->ast, cond, NULL);
> +  PKL_AST_LOC (arg_cond) = PKL_AST_LOC (cond);
> +
> +  if (msg == NULL)
> +    {
> +      msg = pkl_ast_make_string (p->ast, "");
> +      PKL_AST_TYPE (msg) = ASTREF (pkl_ast_make_string_type (p->ast));
> +      PKL_AST_LOC (msg) = stmt_loc;
> +    }
> +
> +  arg_msg = ASTREF (pkl_ast_make_funcall_arg (p->ast, msg, NULL));
> +  PKL_AST_LOC (arg_msg) = PKL_AST_LOC (msg);
> +
> +  arg_lineinfo = ASTREF (pkl_ast_make_funcall_arg (p->ast, lineinfo, NULL));
> +  PKL_AST_LOC (arg_lineinfo) = PKL_AST_LOC (lineinfo);
> +

Maybe a comment here like "Mount the funcall, put it in an expression
statement and return it"?

> +  call = pkl_ast_make_funcall (
> +      p->ast, var,
> +      pkl_ast_chainon (arg_cond, pkl_ast_chainon (arg_msg, arg_lineinfo)));
> +  PKL_AST_LOC (call) = stmt_loc;
> +
> +  asrt = pkl_ast_make_exp_stmt (p->ast, call);
> +  PKL_AST_LOC (asrt) = stmt_loc;
> +
> +  return asrt;
> +}

You don't really need all these PKL_AST_LOC by the way.  If the subtree
you are mounting is correct and therefore won't trigger a compilation
error or warning then the nodes don't need to have location information.

The reason why the compiler is plagued with calls to PKL_AST_LOC for
compiler-generated nodes is that initially I had a handler that checked
for every node in the tree to have one.  Then I changed my mind and
removed it.

> @@ -1932,6 +1988,22 @@ simple_stmt:
>                      PKL_AST_LOC (PKL_AST_TYPE ($3)) = @3;
>                    PKL_AST_LOC ($$) = @$;
>                  }
> +        | ASSERT '(' expression ')'
> +                {
> +                  pkl_ast_node asrt = pkl_make_assertion (pkl_parser, $3, 
> NULL,
> +                                                          @$);
> +                  if (asrt == NULL)
> +                    YYERROR;
> +                  $$ = asrt;
> +                }
> +        | ASSERT '(' expression ',' expression ')'
> +                {
> +                  pkl_ast_node asrt = pkl_make_assertion (pkl_parser, $3, $5,
> +                                                          @$);
> +                  if (asrt == NULL)
> +                    YYERROR;
> +                  $$ = asrt;
> +                }

However, you are missing PKL_AST_LOC ($$) = @$; in the new rules you
introduce there! :)

The exp_stmt node you create in pkl_make_assertion may be in the wrong
context and therefore referred in a warning/error message.

> diff --git a/testsuite/poke.pkl/assert-4.pk b/testsuite/poke.pkl/assert-4.pk
> new file mode 100644
> index 00000000..c2cbf1a5
> --- /dev/null
> +++ b/testsuite/poke.pkl/assert-4.pk
> @@ -0,0 +1,16 @@
> +/* { dg-do run } */
> +
> +fun a = (int cond) void:
> +  {
> +    assert (1 == 1, "One is equal to one");
> +
> +    try assert (cond);
> +    catch
> +      {
> +        print ("assertion failed");
> +      }
> +  }
> +
> +/* { dg-command { a (1) } } */
> +/* { dg-command { a (0) } } */
> +/* { dg-output "assertion failed" } */

Don't you want to test that the error set in the assert exception is
correct?  You can do it with:

fun a = (int cond) void:
 {
   assert (1 == 1, "One is equal to one");

   try assert (cond);
   catch (Exception e)
   {
      print e.msg;
    }
}