Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type narrowing should be abandoned after an operation with potential side effects #13904

Closed
JoshuaKGoldberg opened this issue Feb 6, 2017 · 4 comments
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Feb 6, 2017

TypeScript Version: 2.1.1

Code

export interface IUpdatee {
    id: string;
}

export class Container {
    public constructor(private updatee: IUpdatee) { }

    public setId(id: string) {
        this.updatee.id = id;
    }
}

export function TestContainer() {
    const updatee = {
        id: "before"
    };
    const container = new Container(updatee);

    if (updatee.id !== "before") {
        throw new Error("Updatee should stay with an initial value.");
    }

    // After this point we shouldn't be assuming updatee.id === "before"
    container.setId("after");

    // Operator '!==' cannot be applied to types '"before"' and '"after"'.
    if (updatee.id !== "after") {
        throw new Error("Updatee should receive a new value.");
    }
}

Expected behavior:

No errors, since this code is correct.

Actual behavior:

Operator '!==' cannot be applied to types '"before"' and '"after"'. thrown on line 28.

As put by @sethobrien: In the most extremely conservative compiler, it wouldn't do type narrowing on properties at all because the property getter itself could have side effects. that's probably too risk-averse to be useful. But if any other function call comes after we've narrowed a property, we should lose the narrowing info.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Feb 6, 2017
@RyanCavanaugh
Copy link
Member

But if any other function call comes after we've narrowed a property

This seems conservative to the point of annoyance. It'd be very surprising to find a property narrowing reset after calling console.log or assert, for example. I would not look forward to getting daily bug reports on that one.

I would agree that it seems reasonable to invalidate narrowings on properties of an object which has a method called on it, though. It'd fix some really "stupid" narrowings we have (Array#length specifically) and is easier to understand.

Per usual see #9998 for other discussion

@JoshuaKGoldberg
Copy link
Contributor Author

I wonder if it would be reasonable to make a dependency graph within each block scope of which objects have access to which other objects? In the above example, container has access to updatee because it's a constructor param; therefore, anything container does should have access to updatee.

@ghost
Copy link

ghost commented Jan 27, 2018

The testcases in issues and #14695 and #19638 use getters. Given that statefulness is usually the raison d'etre behind a getter in the first place, then "is this property a getter?" would be a reasonable heuristic for selectively turning off CFA/narrowing.

Anders's suggestion to silence the compiler is wrapping the property access in a function. With narrowing disabled for getters, the workaround would be to wrap it in a getter, which means (a) code that "reads" more idiomatically, and (b) eliminating the need to go edit every access to convert it into a function call instead.

@RyanCavanaugh RyanCavanaugh added Too Complex An issue which adding support for may be too complex for the value it adds and removed In Discussion Not yet reached consensus labels Feb 6, 2018
@RyanCavanaugh
Copy link
Member

Needless to say this would be a massive breaking change, and incorrectly lots of correct code an error in ways that would be rather difficult to get yourself out of without type assertions.

A proper fix would be some kind of mutability notation (similar to const methods in C++, or assume the reverse and mark which methods do have local side effects) that would have to be pervasive throughout all classes. Maybe in the far future we'll do some "immutable first" paradigm-shifting mode, but not anytime soon as a local change to narrowing behavior.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds
Projects
None yet
Development

No branches or pull requests

3 participants