tc39 / ecmarkup

An HTML superset/Markdown subset source format for ECMAScript and related specifications
https://tc39.es/ecmarkup/
MIT License
222 stars 63 forks source link

False positive warning about "returning an abrupt completion" #619

Open Jack-Works opened 2 months ago

Jack-Works commented 2 months ago

it warns for the following code after upgrading from 19.1.0 to 20.0.0. It uses ReturnIfAbrupt.

Warning: spec.emu: this algorithm is declared as returning an abrupt completion, but there is no step which might plausibly return an abrupt completion

<emu-clause id="sec-iterator-step-cached" type="abstract operation">
    <h1>
    IteratorStepCached (
        _iterator_: an Iterator Record,
        _cacheGroup_: a %Map%,
    ): either a normal completion containing either an ECMAScript language value or ~not-matched~, or an abrupt completion
    </h1>
    <dl class="header">
    </dl>
    <emu-alg>
    1. Assert: _cacheGroup_ is created by CreateMatchCache and used internally for pattern-matching.
    1. If _iterator_.[[Done]] is *true*, return ~not-matched~.
    1. Let _cache_ be GetMatchCache(_iterator_, _cacheGroup_).
    1. Let _iteratedValues_ be ! Get(_cache_, *"IteratedValues"*).
    1. Let _iteratorResult_ be Completion(IteratorStep(_iterator_)).
    1. If _iteratorResult_ is an abrupt completion, set _iterator_.[[Done]] to *true*.
    1. ReturnIfAbrupt(_iteratorResult_).
    1. If _iteratorResult_ is *false*, then
        1. Set _iterator_.[[Done]] to *true*.
        1. Return ~not-matched~.
    1. Let _value_ be Completion(IteratorValue(_iteratorResult_)).
    1. If _value_ is an abrupt completion, set _iterator_.[[Done]] to *true*.
    1. ReturnIfAbrupt(_value_).
    1. Perform ! Call(<emu-xref href="#sec-array.prototype.push">%Array.prototype.push%</emu-xref>, _iteratedValues_, « _value_ »).
    1. Return _value_.
    </emu-alg>
</emu-clause>
ljharb commented 2 months ago

Should it instead say Perform ! _value _, or just prepend a ! on the Call step that references it?

bakkot commented 2 months ago

_value_ gets unwrapped by ReturnIfAbrupt already; the problem is that the new logic is failing to account for ReturnIfAbrupt and friends.

That said, I'd write the above code as

1. If _value_ is an abrupt completion, then
  1. Set _iterator_.[[Done]] to *true*.
  1. Return ? _value_.

instead of using ReturnIfAbrupt.

(Though in fact the correct thing these days is to use IteratorStepValue, which handles setting _iterator_.[[Done]] already.)

ljharb commented 2 months ago

Right, I meant, remove the ReturnIfAbrupt step and let a ! handle it in the next step.

bakkot commented 2 months ago

You can't use ! because it might actually be abrupt, at least as currently written. But yes, you can unwrap with Set _iteratorResult_ to ? _iteratorResult_., and that would also be better.

bakkot commented 2 months ago

(I actually kind of want to get rid of ReturnIfAbrupt entirely, though we'll still need IfAbruptCloseIterator and IfAbruptRejectPromise.)

Jack-Works commented 1 month ago

Can I 1. ? _value_?

bakkot commented 1 month ago

No, but you can Set _value_ to ? _value_. Or if you already know that _value_ is definitely an abrupt completion, you can Return ? _value_.

ljharb commented 1 month ago

You can also Perform ? _value_. but the assignment is probably clearer.

bakkot commented 1 month ago

We only use Perform when invoking AOs or SDOs.