Conditional assertions

[ELLIOTTCABLE]

So, at the moment, using babel-plugin-unassert, I can't write an assertion within a conditional:

if (options.class) assert(root instanceof options.class)

---

> BABEL_ENV=production ./build.sh
TypeError: ./giraphe.es6.js: Property consequent of IfStatement expected node to be of a type ["Statement"] but instead got null
    at Object.validate (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-types/lib/definitions/index.js:115:13)
    at Object.validate (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-types/lib/index.js:552:9)
    at NodePath._replaceWith (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-traverse/lib/path/replacement.js:214:7)
    at NodePath._remove (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-traverse/lib/path/removal.js:60:10)
    at NodePath.remove (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-traverse/lib/path/removal.js:31:8)
    at /Users/ec/Documents/Code/.local/giraphe/node_modules/babel-traverse/lib/path/lib/removal-hooks.js:40:12
    at NodePath._callRemovalHooks (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-traverse/lib/path/removal.js:51:9)
    at NodePath.remove (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-traverse/lib/path/removal.js:25:12)
    at PluginPass.visitor.CallExpression.enter (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-plugin-unassert/index.js:57:37)
    at newFn (/Users/ec/Documents/Code/.local/giraphe/node_modules/babel-traverse/lib/visitors.js:310:19)

So, I understand the dangers of messing with code around the actual call to the assert; but this particular one is really, really egregious (assertions occur alone behind a conditional all the time!)

Here's my thought for how to deal with it:

1. By default, if assertion-statement being removed is the sole child of a conditional branch, then remove that conditional branch, and insert the condition-expression as a bare statement (incase it was written to have any side-effects):

   if (foo(bar))
     assert(widget);
   # becomes
   foo(bar);

2. And provide an option to entirely remove conditionals whose only contents are an assertion (i.e. “I am acknowledging that I know that I need to write my conditional-assertions to have no side-effects, or to include a second statement within their body if they do.”

If this is welcome, I'm happy to write a few failing test-cases and pull-request them. (=

[twada]

@ELLIOTTCABLE Thank you for reporting and good catch!

I agree with you that some conditionals have side-effects and it's risky to remove them. How about changing them to EmptyStatements?

if (foo(bar))
   assert(widget);

becomes

if (foo(bar))
   ;

or in babel

if (foo(bar))
   undefined;

If this is welcome, I'm happy to write a few failing test-cases and pull-request them. (=

Yeah thanks! Would you send issue and PR to babel-plugin-unassert? (Since unassert is esprima/escodegen based and with a bit tricky implementation)

[falsandtru]

Also:

$ echo '1 ? assert(0) : 0' | unassert
1 ? assert(0) : 0;
$ echo '1 && assert(0)' | unassert
1 && assert(0);

It would be better to be removed or throws semantic errors also in these cases for consistency. Otherwise, users cannot predict unassert behaviors.

[twada]

@falsandtru as is now unassert removes assertions if and only if it is a statement (its parent is an ExpressionStatement) since some libraries (e.g. jQuery #4) use its original assert function for not standard (for the name assert we imagine) way.

In your case, ConditionalExpression, author would be using assert as a original function that returns value and the code would be an assignment.

var foo = bar ? assert(0) : 0

unassert shouldn't remove it in such a case.

The easiest way to detect which call is to remove is, the case when assert call is a single expression. Not in conditionals, No return values.

By the way, you must be using unassert-cli and I'm glad to see it's so useful for one-liner explanation :)

[ELLIOTTCABLE]

So, I've put … rather a lot of thought into this today; and I've come to the following conclusion:

There's no good way to do this by default.

I think, whatever you do, it's going to be dangerously surprising in a not-inconsequential number of edge-cases. In situations like this, the only good user-experience, is a combination of requiring intervention, and educating the user; so here's my current suggestion:

1. Implement three options in the underlying library, from which the user can choose for their project with a configuration option:
   • “Wrap:” assert(foo) in problematic context → retain context, and insert configured replacement for the assert itself (console.log('Disabled assert:foo'), as a random, terrible example?)
   • “Retain:” assert(foo) in problematic context → discard the assert, replace with EmptyStatement or void 0-style expression, as appropriate, but retain context (if side-effects are desired)
   • “Discard:” assert(foo) in problematic context → discard the assert and parent-statement (or enclosing expression, in an expression-context)
2. And, crucially, make it a fatal error (compile/transpile-time) for any of those examples to occur before the user has configured that.

I know that sounds drastic, but I really think it's necessary for a pleasant UX: unassert needs to establish with the user a contract as to how it will handle asserts as early as possible. A good starting-point is mentioning some of this in the README (yes, I do think this is that important of an issue :P, it's a very common pattern.), followed by an error if they don't follow the README for their particular interface, and fail to configure one of the above behaviours.

(Once the user has established which one they wish to use, then it's up to them to maintain that; and it becomes something unassert doesn't have to worry about anymore. i.e. if they choose ‘discard’, and then have conditionals with side-effects … well, that's on them. Y'know what I'm saying?)

[ELLIOTTCABLE]

(Clarification: The reason I think this is such a big deal, is because we're literally talking about code differences between development/testing and production. That's a pretty serious opportunity for subtle bugs, if Enormous Effort isn't expended educating the developer as exactly what will change.)

[twada]

@ELLIOTTCABLE Thank you for clarification. unassert requires more documentation. Right.

unassert removes assert calls that look like assertions in terms of Assertion (software development) - Wikipedia.

In terms of ECMAScript AST, a single CallExpression wrapped by ExpressionStatement. That is not assignment, not an argument of other calls, etc.

Your case, if statement without curly braces, is a few edge case that CallExpression is not wrapped by ExpressionStatement. CallExpression is a immediate child of IfStatement.

if (foo(bar))
   assert(widget);

parsed AST of code above is not the same as code below

if (foo(bar)) {
   assert(widget);
}

In a few cases like that, I'll go with your "Retain" strategy. It's a bugfix.

BTW, It's nice to have to fail first at compile time. So I'll consider implementing it too.

Thanks.

[ELLIOTTCABLE]

Well, I don't see it as the same. This is pretty important, because runtime-heavy tasks can occur in that conditional; and conditional assertions (distinct from ‘assertions that happen to be inside a conditional’) can be heavy like that.

Here's a real-world example of my own: I've a directed-cyclic-graph consuming library; and when exercised by my tests, I want something like the following:

if (node.isSomeComplicatedGraphState)
   assert(node.isSomeOtherComplicatedState())

Such methods already exist, but have a high runtime-complexity: say the actual method under test is O(n²)-complex; with the assertions, it's O(2n + n²) (which is fine) … but if I'm depending on unassert to production-ify my code, it's Very Bad if half of that assertion gets left in (so now my production method is O(n + n²)).

Now, some engines will optimize-away an EmptyStatement conditional that they can statically-analyze to have no side-effects … but not all, and not often, and almost certainly not in these problematic cases I'm mentioning (you think Graph::isSomeComplicatedGraphState is going to be an easily-analyzed inlineable? I doubt it :P)

This is a really common pattern in assertion-based development (I seem to remember a really neat blog-post that broke the experience down into two real-world 'flavours ' of assertions; the runtime-simple, and the runtime-complex, like this one); and I don't think it's out-of-line to ask for better support! :O On Thu, May 19, 2016 at 8:48 PM Takuto Wada notifications@github.com wrote:

@ELLIOTTCABLE https://github.com/ELLIOTTCABLE Thank you for clarification. unassert requires more documentation. Right.

unassert removes assert calls that look like assertions in terms of Assertion (software development) - Wikipedia https://en.wikipedia.org/wiki/Assertion_(software_development).

In terms of ECMAScript AST, a single CallExpression wrapped by ExpressionStatement. That is not assignment, not an argument of other calls, etc.

Your case, if statement without curly braces, is a few edge case that CallExpression is not wrapped by ExpressionStatement. CallExpression is a immediate child of IfStatement.

if (foo(bar)) assert(widget);

parsed AST of code above is not the same as code below

if (foo(bar)) { assert(widget); }

In a few cases like that, I'll go with your "Retain" strategy. It's a bugfix.

BTW, It's nice to have to fail first at compile time. So I'll consider implementing it too.

Thanks.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/twada/unassert/issues/7#issuecomment-220497759

[twada]

IMO, conditional assertions is NOT common.

Writing assertion with conditional is error prone practice and is not effective, performant. Both Assertion programing paradigm and Design by Contract paradigm do not encourage you to write assertions in conditionals. They (including me, ) use conditional expression inside assertions.

assert(node.isSomeComplicatedGraphState && node.isSomeOtherComplicatedState());

[falsandtru]

I'm always using that style.

[ELLIOTTCABLE]

The problem is, that example will fail: sure, you can short-circuit the runtime complexity of the second function, but at the cost of that assertion actually failing, when the point is that it shouldn't occur at all when node.isSomeComplicatedGraphState isn't the case:

assert(node.isSomeComplicatedGraphState() && node.isSomeOtherComplicatedState())
       |    |                             |
       |    false                         false
       Object{}

And again, as above, I realize I can wrap if (state) assert(other.state) into a function, and then assert that function … but that runs into one of two problems, every time:

1. If I preform the actual assert() within the body of the function, then we've come full-circle to the same problem of the complicated ‘should this assertion even occur’ still occurring at runtime,

   function assertIfIsANode(){ if (node.isSomeComplicatedGraphState()) /* noop */; }
   
   function hotLoopFunction(){
     assertIfIsANode()
     // ...
   })

2. … but if I assert() the function-call itself, then power-assert becomes useless, as it receives no metadata whatsoever about the actual assertion:

   function ifIsANode(){ if (node.isSomeComplicatedGraphState())
     return node.isSomeOtherState(); }
   
   function hotLoopFunction(){
     assert(ifIsANode())
     // ...
   })

… and in both situations, this means writing external support code for my assertions: in the worst case, each assert() will now need a multi-line function written somewhere else, maintained somewhere else, and explained somewhere else. (In this example, it probably means exporting some new, unofficial API on the node type, y'know?)

I suspect I'm simply not making my case well, because … it genuinely seems extremely obvious to me that this is Really Important?

---

Edit: To be clear, I understand the value of convention-over-configuration; and I also think the behaviour you already implemented / described is the best default behaviour. I disagree with your assertion (ha ha) that this isn't a problem; but if you understand that this is a real thing, that real people need, then we're on the same page … and I totally understand prioritizing the usual case, along with the maintenance-cost of adding configuration options to unassert, over this relatively edge-case situation.

tl;dr I'd be much more understanding if I heard “I won't implement this because it's not within the goals of unassert” instead of “This isn't a real problem, you're using assertions wrong.”

[twada]

@ELLIOTTCABLE Thank you for your suggestion again.

First, I apologize that I misunderstood your intention of what "conditional" means. Now I understood. Yeah it's a real problem sometimes.

I'd be much more understanding if I heard “I won't implement this because it's not within the goals of unassert” instead of “This isn't a real problem, you're using assertions wrong.”

Second. I apologize that my explanation is not a nice one. It's not a fair one. I'm so sorry for that.

Now I'm going to find the way to solve this problem. First I implement your "Retain" strategy since it contains a bugfix to be released soon.

Then I'll tackle with conditional assertions in my way. So please stay tuned.