Conditional assertions #7

Activity

1. twada commented on May 17, 2016

   

   twada

   on May 17, 2016

   Last edited by twada

   Member

   More actions

   @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)
2. falsandtru commented on May 17, 2016

   

   falsandtru

   on May 17, 2016

   More actions

   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.
3. twada commented on May 17, 2016

   

   twada

   on May 17, 2016

   Last edited by twada

   Member

   More actions

   @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 :)
4. ELLIOTTCABLE commented on May 18, 2016

   

   ELLIOTTCABLE

   on May 18, 2016

   Author

   More actions

   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?)

   👍React with 👍1Reacted by Takuto Wada
5. ELLIOTTCABLE commented on May 18, 2016

   

   ELLIOTTCABLE

   on May 18, 2016

   Author

   More actions

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

   👍React with 👍1Reacted by Takuto Wada
6. twada commented on May 20, 2016

   

   twada

   on May 20, 2016

   Member

   More actions

   @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.
7. ELLIOTTCABLE commented on May 20, 2016

   

   ELLIOTTCABLE

   on May 20, 2016

   Last edited by ELLIOTTCABLE

   Author

   More actions

   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
   #7 (comment)
8. twada commented on May 20, 2016

   

   twada

   on May 20, 2016

   Last edited by twada

   Member

   More actions

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

   👍React with 👍1Reacted by falsandtru
9. falsandtru commented on May 23, 2016

   

   falsandtru

   on May 23, 2016

   More actions

   I'm always using that style.
10. ELLIOTTCABLE commented on May 28, 2016

    

    ELLIOTTCABLE

    on May 28, 2016

    Last edited by ELLIOTTCABLE

    Author

    More actions

    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.”

    👍React with 👍1Reacted by Takuto Wada
11. twada commented on May 28, 2016

    

    twada

    on May 28, 2016

    Last edited by twada

    Member

    More actions

    @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.