search

getify / native-promise-only

A polyfill for native ES6 Promises as close as possible (no extensions) to the strict spec definitions.
720 stars 78 forks source link

Uncovered code as of 07-Jun-14: #2 - checkYourself #17

Closed smikes closed 10 years ago

smikes commented 10 years ago

TBQH I was a little disappointed that this didn't wind up calling wreckYourself()

Seriously, though, the if() test on line 146 is never true, so the block containing line 147 is never executed.

I am curious about this: is this a provable never-happens condition, or is it a hole in the promises-aplus-tests, or even a hole in the specification?

In any case I certainly don't advocate removing the guard yet. Eventually if we can prove self.state is never 0 at that point, then the guard and return can go, simplifying the code a bit.

getify commented 10 years ago

it a hole in the promises-aplus-tests, or even a hole in the specification?

Neither of those, per se. I think it's actually something that proper testing of the Promise(..) constructor should catch. Note: the A+ spec isn't testing the semantics of Promise(..) so it wouldn't catch stuff like this.

IIRC, that check is there for a case like:

var p = new Promise(function(resolve,reject){
   resolve(..);
   // later
   resolve(); // this second call should silently fail per A+ spec, but not
                   // tested in A+ spec tests

   // same goes for calling `reject(..)` twice.
});

In other words, the check is, if you've already called resolve(..), then state is no longer 0 and you need to silently fail.

However, later on, I added two other guards, lines 274-277 and lines 282-285, respectively, which I believe have the same effect (but for a slightly different reason).

Boiling it all down, I believe this is indeed probably a case of provably unreachable, given my implementation check on def.triggered already guarding against the only place where this could happen.

I certainly don't advocate removing the guard yet

We definitely should leave it in for now, until we get a full test suite against Promise(..), even tests that try the above stuff. Then we can remove the check and see if any of the tests fail. I highly doubt they will, but doubting and knowing are two different things at this point.

So let's make sure to revisit this later. This check is on the hot path so removing it would tweak the perf up a little bit (perhaps a lot in aggregate?), so I definitely want to once we have tests proving it's not needed. :)

smikes commented 10 years ago

Agreed, this code is no longer present, therefore not uncovered.