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

why does initialization narrow type? #8513

Closed
zpdDG4gta8XKpMCd opened this issue May 7, 2016 · 26 comments · Fixed by #8548
Closed

why does initialization narrow type? #8513

zpdDG4gta8XKpMCd opened this issue May 7, 2016 · 26 comments · Fixed by #8548
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented May 7, 2016

nightly build

"compilerOptions": {
        "module": "amd",
        "moduleResolution": "classic",
        "noEmitOnError": true,
        "allowUnreachableCode": false,
        "allowUnusedLabels": false,
        "outDir": "./built",
        "noResolve": true,
        "noImplicitAny": true,
        "noImplicitReturns": true,
        "forceConsistentCasingInFileNames": true,
        "noFallthroughCasesInSwitch": true,
        "target": "es5"
        // "strictNullChecks": true
    }
export type Optional<a> = Some<a> | None;

export interface None { readonly none: string; }
export interface Some<a> { readonly some: a; }

export const none : None = { none: '' };

export function isSome<a>(value: Optional<a>): value is Some<a> {
    return 'some' in value;
}

export function fn<r>(): void {
    let result: Optional<r> = none;
    isSome(result)
        ? result.some
        : undefined
}

image

@malibuzios
Copy link

malibuzios commented May 7, 2016

(Tested on 1.9.0-dev.20160507)

I believe what is happening here is similar to this much simpler example:

let x: number | string = 1234;

if (typeof x === "string") {
    x; // x has type 'nothing'
}

x was declared with the union type number | string, and was assigned a number. Flow analysis has correctly predicted that its actual type right before entering the if (typeof x conditional was number.

The program then went to test if its type was string and a narrowing was applied through the guard. Since flow analysis started with the assumption it was a number, the option of it being a either a number or a string in the body of the conditional had been already discarded, and the type that remained was the empty union type nothing.

In the example you gave, this function was used as a user-defined guard for the target type Some<T> and the source union Some<T> | None:

export function isSome<T>(value: Some<T> | None): value is Some<T> {
   //..
}

And the guard was applied in a similar circumstance:

let result: Some<T> | None = None; // Flow analysis now assumes the type is 'None'

if (isSome(result)) { // The option of it being 'None' is removed here
    result; // 'result' gets type 'nothing'
}

@zpdDG4gta8XKpMCd
Copy link
Author

you might be right, but the flow analysis should not ignore the explicit type annotations result: Some<T> | None

@malibuzios
Copy link

malibuzios commented May 7, 2016

The idea here, I believe, is that despite the fact that flow analysis uses unsound assumptions and is not really 'perfect' in its predictions (e.g. it doesn't currently account to assignment of captured variables by called functions), it is still somehow 'loyal' to its own idea of the execution of code. It predicted that the if block would never be executed so it used the nothing type there instead of string (which I believe it could and would also be technically correct).

let x: number | string = 1234; // flow analysis narrowed from 'number | string' to 'number'

if (typeof x === "string") { // flow analysis narrowed from 'number' to 'nothing'
    x; // x has type 'nothing'
}

My understanding is that nothing is used as a signifier for an 'error' state, to 'alert' the programmer when something went wrong from the point of view of the 'unsound' type system, not necessarily in run-time - where the if block could have actually been entered and executed perfectly fine, and x would indeed have the type string (I'm not necessarily referring to this particular example, though).

I have also tried to observe and suggest improvements for flow analysis in an even more 'extreme' state: where nothing is even assumed as the beginning point, more in this issue.

Edits: rephrased a bit.

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented May 7, 2016

i understand your reasoning, i am questioning the premise of the apparent fact that despite being explicitly said that result at the point of its declaration can be either Some<r> or None the flow analysis ignores it and fails short immediately

@mhegazy
Copy link
Contributor

mhegazy commented May 7, 2016

as @malibuzious noted, this is behaving as expected.

let result: Optional<r> = none;

beahaves as:

let result: Optional<r> = none;
result; // Some<r> | None | undefined
result = none;
result; // None

To achieve the result you want case the value, i.e.:

let result = <Optional<r>>none;

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label May 7, 2016
@mhegazy mhegazy changed the title why doesn't this code typecheck? why does initialization narrow type? May 7, 2016
@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented May 7, 2016

this is just rediculous

favoring let result = <Optional<r>>none over let result: Optional<r> = none... how can i unsee it

@zpdDG4gta8XKpMCd
Copy link
Author

i hope there will be a flag to turn the flow analysis off because its' going to break 75% of our code base

@zpdDG4gta8XKpMCd
Copy link
Author

we all understand that let result = <Optional<r>> none is semantically different (and not in a good way) from let result: Optional<r> = none, don't we?

@mhegazy
Copy link
Contributor

mhegazy commented May 7, 2016

i hope there will be a flag to turn the flow analysis off because its' going to break 75% of our code base

so is this the pattern you are using?

let result: Optional<r> = none;

initalizeResult();

if (isSome()) {
 .... 
}

See related discussion in #8381

@zpdDG4gta8XKpMCd
Copy link
Author

no, result here is a accumulator value for a fold/reduce like routine, so
we start it with a "zero" value (none in this case) then we step into a
loop where the accumulation happens and immediately hit this problem.

I intentionally left the original code outside to make it clean cut.
On May 7, 2016 4:04 PM, "Mohamed Hegazy" [email protected] wrote:

i hope there will be a flag to turn the flow analysis off because its'
going to break 75% of our code base

so is this the pattern you are using?

let result: Optional = none;

initalizeResult();
if (isSome()) {
....
}

See related discussion in #8381
#8381


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8513 (comment)

@mhegazy
Copy link
Contributor

mhegazy commented May 7, 2016

I do not see the issue then. Can you post a more elaborated sample of the pattern you use?

@ahejlsberg
Copy link
Member

@Aleksey-Bykov Consider this example:

// Compile with --strictNullChecks
let x: string | number = "abc";
let len = x.length;  // Ok, x is known to be a string
x = 5;
let y = x * 10;  // Ok, x is known to be number
x = true;  // Error, boolean not assignable to string | number

Note that the type annotation specifies the upper type bound that must always be true for values assigned to the variable, but the whole purpose of control flow based type analysis is to track the more specific actual type of the variable. That's what is happening above. The compiler isn't ignoring anything.

If I understand you correctly, you're arguing that the first line should be an error? (i.e. that the compiler should ignore the assigned type and pretend that the current value is possibly a number.)

@myitcv
Copy link

myitcv commented May 8, 2016

@ahejlsberg the "change" here is that previously the following would have been an error:

// From Playground (pre control flow)
let x: string | number = "abc";
let len = x.length;   // error: property 'length' does not exist on type 'string | number'

If anyone relied on/expected this to be an error before, it is no longer an error with control flow analysis as implemented and hence is breaking to my mind.

Have to agree with @Aleksey-Bykov (assuming we are unhappy with the same thing?!). I declare types on variables to help enforce things (i.e. expect errors in certain situations) as much as I look to ask "what can I do?"

If I declare a variable as type string | number:

let x: string | number;

then I expect the method/property set to be the intersection of string and number

Is there any reason control flow analysis has not been put behind a flag to turn it on?

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented May 8, 2016

@ahejlsberg, sorry for my complaints, my apologies, I should say that the flow analysis is realy awesome!

It has a hard part though:

As we just saw, there is no way (without using type assertions) to initialize a variable of an extended type (compared to the type of the initializing value) anymore, so the flow analysis

  • brings a breaking change, although I have to admit it's not a very common pattern in our code (7 places that broke in over 1500 files)
  • behaves unintuitively (at least to me) and might puzzle developers

To keep in constructive I wish I knew how to solve the above problem without using the type assertions.

@mhegazy here is the original function that breaks

export function foldFew<p extends string, a, r>(
    values: Pinned<p, a>,
    toResult: (value: a, pin: p) => r,
    folding: (result: r, value: a, pin: p) => r
): bo.Optional<r> {
    let result: bo.Optional<r> = bo.noneFrom('No values.');
    for (var key in values) {
        let pin = <p>key;
        let next : bo.Optional<r> = bo.someFrom(
            bo.isSome(result)
                ? folding(
                    result.some,
                    _peek(values, pin),
                    pin
                )
                : toResult(
                    _peek(values, pin),
                    pin
                )
        );
        result = next;
    }
    return result;
}

@ivogabe
Copy link
Contributor

ivogabe commented May 8, 2016

I think that the code in the loop is analyzed twice, first with only the flow path from before the loop. Then the only type of result is None. See #8404 for more details.

@ahejlsberg I think this is another argument to separate type analysis from error reporting. Or do you have a different solution?

@ahejlsberg
Copy link
Member

ahejlsberg commented May 8, 2016

Here is a simple example that shows the issue:

let cond: boolean;
function foo() {
    let x: string | number = 0;
    x;  // number
    while (cond) {
        x;  // number, then string | number
        x = typeof x === "string" ? x.slice() : "abc";
        x;  // string
    }
}

It currently errors on x.slice() with the error Property 'slice' does not exist on type 'nothing' because, on entry to the loop, the type of x is known to be number. Once the flow back from the loop is analyzed the full possible type of string | number appears, and the error goes away again.

Generally it is the case that more capabilities emerge when a union type is narrowed. For example, narrowing string | number to number makes more methods available. However, as things stand now, going from number to nothing effectively takes everything away. Perhaps this argues that nothing should be more like any because, technically, the nothing type is an indication that the code will never execute and thus the code could never be the source of errors.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels May 8, 2016
@ahejlsberg ahejlsberg self-assigned this May 8, 2016
@ahejlsberg ahejlsberg added this to the TypeScript 2.0 milestone May 8, 2016
@myitcv
Copy link

myitcv commented May 8, 2016

@ahejlsberg

Generally it is the case that more capabilities emerge when a union type is narrowed.

Apologies if I've missed the relevant conversation, but what is the motivation for narrowing a union type? Does it need to be done, or is it one option amongst many in answering how to deal with union types in control flow analysis?

As mentioned above I find it very counterintuitive; I'm also less clear on the benefits it brings vs the explicit approach of switching based on type.

There is also the question on how the spec will be reworded to explain the property and method set available on variable of a union type... I suspect this will be tricky to word, not least to understand (on top of everything else).

I would also flag that this approach with union types doesn't appear to be consistent with the following:

interface A {
    DoIt(): string;
}

class C1 {
    DoIt(): string {
        return "test";
    }

    C1Meth(): number {
        return 5;
    }
}

class C2 {
    DoIt(): string {
        return "trial";
    }

    C2Meth(): number {
        return 4;
    }
}

let x: A;
x = new C1();
x.C1Meth();  // error: Property 'C1Meth' does not exist on type 'A'

@ivogabe
Copy link
Contributor

ivogabe commented May 8, 2016

@myitcv I think that the best use case would be the following:

function test(x: string | string[]) {
  if (typeof x === "string") x = [x];
  // x: string[]
}

This narrowing is not necessary for control flow analysis, but you can do this with no extra effort. It should, aside from this issue, give more accurate types so I don't a big problem with it. I've used it a lot lately and I even didn't need type casts any more.

When non union types are narrowed, you can get unexpected behavior in various cases, for instance:

let x: (string | number)[];
x = [42]; // x: number[]
x.push(""); // Would be an error if x was narrowed

let y: { z?: boolean };
y = {};
// y: {}
y.z = true; // Error: z does not exist on {}

@DanielRosenwasser
Copy link
Member

Perhaps this argues that nothing should be more like any because, technically, the nothing type is an indication that the code will never execute and thus the code could never be the source of errors.

@ahejlsberg so would the following code not give an error at all?

function f(x: number) {
    if (typeof x === "string") {
        // I'm assuming 'x' has type 'nothing' here,
        // so is this valid?
        x.length;
    }
}

@mhegazy
Copy link
Contributor

mhegazy commented May 8, 2016

@ivogabe the example you shared is not accurate. In both cases, assignments to x and y do not change the type because of contextual types. so both cases are not errors.

@ahejlsberg
Copy link
Member

@myitcv @ivogabe Another important motivating example is:

function f(s?: string) {
    // s is of type string | undefined
    if (!s) {
        s = "";
    }
    // s is of type string in remainder of function
}

Without control flow based type analysis we'd force you to write casts or redundant type guards.

@DanielRosenwasser In your particular example we perform no control flow analysis for x because it is of a primitive type and always definitely assigned--we effectively know that x can never be anything but a number (in well behaved code) so we don't spend time analyzing for other types. But consider this example instead:

function f(x: string | number) {
    if (typeof x !== "string" && typeof x !== "number") {
        x.length;  // Error?
    }
}

If we say that nothing behaves like any then you wouldn't get an error above. You're basically in the land of "defensive coding" at that point because it can never happen according to the type annotations. We currently behave as if x had type {}, but you could argue behaving like x had type any is a reasonable choice.

@DanielRosenwasser
Copy link
Member

On the contrary, I feel like if I have a type that's narrowed to nothing at any point, I'm writing nonsensical code and I'd want to know about it. Perhaps I've made a mistake in some code and branched incorrectly - knowing that I'm not writing anything useful might hint to me that I'm on the wrong path. It's kind of like how I'd want to know if I have unreachable code.

Are there any useful scenarios where the final calculated type (i.e. the type at its fixed point of analysis) at a position is nothing?

@RyanCavanaugh
Copy link
Member

At the very least any use of x at that point should be considered an implicit any (because that's what it is). But {} (or even some poisoned type that isn't assignable to anything, because you might end up passing this var to a thing that accepts any) seems preferable.

@myitcv
Copy link

myitcv commented May 9, 2016

@ivogabe @ahejlsberg in both of those situations the code can be rewritten to the arguably more readable:

function test(xa: string | string[]) {
    let x: string[];
    if (typeof xa === "string") {
        x = [xa];
    } else {
        x = xa;
    }
}

function f2(sa?: string) {
    let s = "";
    if(sa != undefined) {
        s = sa;
    }
}

To re-consider the original example:

let x: string | number;

// ....

console.log(x.length);  // error ???

what do we lose by not having narrowing? The two examples you presented, for sure, but I believe they can (and should) be rewritten more explicitly for the sake of clarity in any case. Anything else?

@ahejlsberg did you also see my question about the interface example above?

@ahejlsberg
Copy link
Member

@myitcv Yes, it is possible to rewrite code to satisfy the type checker, but we're trying to correctly analyze as many common patterns as possible. I think it is beyond doubt that the pattern I showed above is very common and with --strictNullChecks it would fail if it wasn't for union type narrowing.

The question of narrowing non-union types on assignment is one that we're still thinking about. I gets somewhat more complicated because, as @ivogabe's example demonstrates, when optional properties are involved, the assigned type may actually have fewer members than the declared type. One possible mitigation is to use an intersection of the declared type and the assigned type:

let a: { x?: number, y?: number };
a = { x: 1 };
a;  // Type { x: number, y: number | undefined };

Here, the assignment would produce the intersection type { x?: number, y?: number } & { x: number }, which effectively corresponds to { x: number, y: number | undefined }, i.e. we'd remove undefined from the type of x but not loose track of y.

@myitcv
Copy link

myitcv commented May 9, 2016

@ahejlsberg

I think it is beyond doubt that the pattern I showed above is very common

I very much defer to your experience in terms of the prevalence of such a pattern, but, yes, I can see it being a common pattern for people to declare functions like the following (re-including here for the sake of clarity):

function f(s?: string) {
   // write code here to deal with optionality of s
}

Where I disagree is that the "solution" should be to narrow (or indeed in the example presented by @ivogabe); not least because people can very easily be explicit as demonstrated above.

Principal reason being that this "solution" creates a further problem of its own, namely the cognitive dissonance between the declared type and what it then possible because of the narrowing.

If I see the following:

let x: string | number;

then I expect the following to be an error:

console.log(x.length);

The problem created by narrowing union types is that instead the expectation needs to be adjusted to "well, it depends"

So my position here is that I think the choice to narrow union types makes code:

  • less readable
  • less easy to write
  • less easy to maintain

Those are the main benchmarks I would be looking towards to assess what to do in this situation. Given the two use cases put forward have simple and explicit alternative solutions, they don't win me over when it comes to my primary objection.

Of course this doesn't even consider the complexity of implementation, which is not something I have exposure to but is equally as important. I would guess the current choice is more complicated, more edge cases etc?

The question of narrowing non-union types on assignment is one that we're still thinking about

I presented the interface as an example of where the approach on narrowing is currently not consistent. For the avoidance of doubt, I'm against narrowing in this situation too for the same reason as above.

All the above is, of course, just an opinion, and steered only by an overarching goal of making TypeScript more readily adopted and understood by many. It goes without saying that as a moderately heavy user of TypeScript I'm very grateful for the work that you and the rest of the team put into its continued development.

@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label May 11, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants