Regression: failure when comparing two values of the same enum type

[DirtyHairy]

TypeScript Version: 2.0.2

Code

Unfortunately, I have not been able to construct a reduced testcase, so I prepared a branch in the failing project to demonstrate the issue. Please checkout

https://github.com/DirtyHairy/6502.ts

and switch to the branch ts-broken. Doing npm install and grunt initial will demonstrate the issue. After that, the failing compile can be retriggered via grunt ts.

Expected behavior:

Project builds.

Actual behavior:

Using tsc v2.0.2
src/machine/stella/Board.ts(139,16): error TS2365: Operator '!==' cannot be applied to types 'ExecutionState.boot' and 'ExecutionState.fetch'

The code used to compile fine before 2.0.2 . The failing code fragment is this function:

    boot(): Board {
        let cycles = 0,
            cpuCycles = 0;

        this.reset();

        if (this._cpu.executionState !== CpuInterface.ExecutionState.boot)
            throw new Error("Already booted!");

        while (this._cpu.executionState !== CpuInterface.ExecutionState.fetch) {
            this._cycle();

            cycles++;
            if (this._subClock === 0) {
                cpuCycles++;
            }
        }

        this.cpuClock.dispatch(cpuCycles);
        this.clock.dispatch(cycles);
        return this;
    }

The corresponding types are declared in this snippet:

interface CpuInterface {

    // [...]

    executionState: CpuInterface.ExecutionState;

    // [...]

}

module CpuInterface {
    export const enum ExecutionState {
        boot, fetch, execute
    }

    // [...] 

}

I think the problem was introduced when enum values were split into different types, and I suspect that two issues are at work here:

1. The assertion leads the compiler to infer the type of this._cpu.executionState to be CpuInterface.ExecutionState.boot. However, calling this.cycle() will mutate the execution state.
2. The compiler deems the two sides of !== to be of different type and rejects the comparision.

After introducing a manual cast

while (this._cpu.executionState as CpuInterface.ExecutionState !== CpuInterface.ExecutionState.fetch) {

the code compiles fine

However, as I said, I have not been able to construct a reduced test case, so some other aspect of my code seems to contribute to the compiler's confusion 😏

[sandersn]

It looks like control flow is tripping you up, although I'm not sure yet whether if it's working as intended. Basically, after the line if (this._cpu.executionState !== CpuInterface.ExecutionState.boot) throws an error, control flow analysis narrows executionState can't be anything but boot. So the comparison at the beginning of the while is always false.

I know that while loops are more complicated than if statements in that they try to understand any type guards inside the loop body. But I don't think that this._cycle() is recognized as something that could alter this._cpu. I'll have to confirm by checking the code, though.

If that's the case, then perhaps control flow analysis should know when it is narrowing a property of this and understand that method calls on this can also change the value of the property.

[sandersn]

Yes, I'm pretty sure that this feature is working as designed for now. I observed that adding an obvious narrowing inside the body of the loop also gets rid of the error, like this:

        while (this._cpu.executionState !== CpuInterface.ExecutionState.fetch) {
            if (this._cycle()) {
                this._cpu.executionState = CpuInterface.ExecutionState.fetch;
            }

            cycles++;
            if (this._subClock === 0) {
                cpuCycles++;
            }
        }

Since this._cycle(): void, the if body never even executes, but control flow analysis doesn't catch that, so it adds fetch back to the list of states that executionState could have.

For a real workaround, I'd recommend not breaking the CpuInterface abstraction and instead adding methods like CpuInterface.hasBooted and CpuInterface.isFetching and using those instead of directly checking the contents of this._cpu.executionState.

[mhegazy]

This looks like the same issue as #10613. I have a similar explanation in #10613 (comment)

[DirtyHairy]

Hi @sandersn !

Thanks for the feedback! As this is a performance critical part of the emulator (the very inner loop, executed at ~3-4 MHz), I prefer checking the state via direct comparision over a function call at the price of loosening the abstraction. However, I am fine with either of the two other workarounds ;)

Is this part of control flow analysis going to be refined in a future release? The way JS and TS are designed, any function call might have side effects that mutate this._cpu, so the assumptions made by the compiler in this situation seem rather brittle to me ;)

Cheers
-Christian

[sandersn]

Yes, control flow is still being improved as we observe the ways it fails. Lots of tweaks and fixes are going into 2.1 already, for example.