Regression: Not-null assertion causes implicit any

[gcnew]

TypeScript Version: 2.7.0-dev.20171029

Code

function test(numbers: number[]) {
    let last;

    for (const n of numbers) {
        if (n % 2) {
            return n;
        }

        last = n;
    }

    return last!; // without the bang, last is inferred as `number | null`
                  // adding the bang causes implicit any
}

Workaround:
Initialise the "tracking" variable with undefined.

function test(numbers: number[]) {
    let last = undefined;

    for (const n of numbers) {
        if (n % 2) {
            return n;
        }

        last = n;
    }

    return last!; // OK
}

Expected behavior:
Should compile successfully with and without a bang.

[HarshKhatore]

Hi @mhegazy, is the issue open to take and, can someone guide me on this? Thanks.

[sandersn]

No hypotheses jump immediately to my mind. So, I would:

1. Put the example into a file.
2. make sure that tsc gives the reported error. (looks like you need noImplicitAny: true?)
3. Clone and build typescript repo.
4. Make sure that node ~/ts/built/local/tsc.js (for example) gives the same error as tsc.
5. Start debugging with node --inspect-brk ~/ts/built/local/tsc.js and attach with your favourite debugger (VS code and chrome://inspect are mine).
6. Search through the source for the no implicit any error(s) that you get and add breakpoints on them. (Note: Sourcemaps are wonky for me since we switched a dependency-based build, so you might have to put them in tsc.js instead of checker.ts)
7. Once you hit a no implicit any error, look up the stack and try to figure out why the type is any instead of number.

Report back with the stack and error location if things are still confusing. Anything to do with control flow can be complex and surprising.

[HarshKhatore]

Hi @sandersn . Thanks for the steps.
As you said, when compiling using noImplicitAny flag, the code gives the following error:
let last;

Variable 'last' implicitly has type 'any' in some locations where its type cannot be determined.

return last!;

Variable 'last' implicitly has an 'any' type.

And the same errors after compiling with local build.

Now, as you mentioned in point 5, I should debug the ~/ts/built/local/tsc.js, but that is nearly 85k loc. Am I missing something?

[sandersn]

@HarshKhatore Yeah, you'll need to search for the error message [1] you're getting and put a breakpoint there. You'll spend forever getting to the actual error otherwise.

[1] The messages are mangled like so: Variable_0_implicitly_has_type_1_in_some... Basically, you replace spaces with _ and remove other punctuation. (I think the build generates a JSON file with the mapping.)

[HarshKhatore]

@sandersn Okay, will try to do that and get back to you :)

[HarshKhatore]

Hi @sandersn, can you help with this?
command: jake local

This is an issue with gulp?

[sandersn]

@weswigham is our build expert these days. Wesley, any ideas?

Here are some things to check in the meantime.

1. jake does what jake local used to. Does that work? Does gulp work?
2. Have you done npm install -g jake and also gulp?

[HarshKhatore]

No, nothing works. Yes, I have installed both gulp and jake.
Gulp error:

[weswigham]

Hm. What npm and node versions are you using, and what command did you use to install?

[HarshKhatore]

Hi @weswigham . Node version is: 8.11.3 and npm version: 6.2.0.
I installed these using the node.js windows installer, which can be found here: https://nodejs.org/en/

[weswigham]

I meant how did you install the prerequisites in our package.json. 😄

[HarshKhatore]

simply: npm install
This should install the dev dependencies, right?

[weswigham]

Yup - I mostly just wanted to check if you were using a different package manager.

[HarshKhatore]

Hi @weswigham, any update on this?

[Bremys]

Is anyone still on it? Can I give it a shot as a first issue?

[sandersn]

@Bremys give it a shot! Looks like the last attempt was almost 2 years ago. My tips from then still apply except that source maps have been pretty reliable for me lately.

[Bremys]

@sandersn I believe I have managed to find the bug and fix it, but my understanding of the problem and its solution is pretty shallow so I am looking for some affirmation and some technical help.
Following your debug instructions I have found the error is thrown in checker.ts in the function checkIdentifier. The code that I view as relevant is:

// We only look for uninitialized variables in strict null checking mode, and only when we can analyze
// the entire control flow graph from the variable's declaration (i.e. when the flow container and
// declaration container are the same).
const assumeInitialized = isParameter || isAlias || isOuterVariable || isSpreadDestructuringAssignmentTarget || isModuleExports || isBindingElement(declaration) ||
                type !== autoType && type !== autoArrayType && (!strictNullChecks || (type.flags & (TypeFlags.AnyOrUnknown | TypeFlags.Void)) !== 0 ||
                isInTypeQuery(node) || node.parent.kind === SyntaxKind.ExportSpecifier) ||
                node.parent.kind === SyntaxKind.NonNullExpression ||
                declaration.kind === SyntaxKind.VariableDeclaration && (<VariableDeclaration>declaration).exclamationToken ||
                declaration.flags & NodeFlags.Ambient;
const initialType = assumeInitialized ? (isParameter ? removeOptionalityFromDeclaredType(type, declaration as VariableLikeDeclaration) : type) :
                type === autoType || type === autoArrayType ? undefinedType :
                getOptionalType(type);
            const flowType = getFlowTypeOfReference(node, type, initialType, flowContainer, !assumeInitialized);
// A variable is considered uninitialized when it is possible to analyze the entire control flow graph
// from declaration to use, and when the variable's declared type doesn't include undefined but the
// control flow based type does include undefined.
if (!isEvolvingArrayOperationTarget(node) && (type === autoType || type === autoArrayType)) {
            if (flowType === autoType || flowType === autoArrayType) {
                if (noImplicitAny) {
                         /* ERROR thrown here */
                    error(getNameOfDeclaration(declaration), Diagnostics.Variable_0_implicitly_has_type_1_in_some_locations_where_its_type_cannot_be_determined, symbolToString(symbol), typeToString(flowType));
                    error(node, Diagnostics.Variable_0_implicitly_has_an_1_type, symbolToString(symbol), typeToString(flowType));
                    }
                    return convertAutoToAny(flowType);
                }
            }

The two scenarios (with and without bang operator) first differ in their assumeInitialized evaluation, as intended.
What I think is not intended is initialType being evaluated to any type with the bang operator in contrast to being correctly evaluated as undefined without it. So my solution is:

// We only look for uninitialized variables in strict null checking mode, and only when we can analyze
// the entire control flow graph from the variable's declaration (i.e. when the flow container and
// declaration container are the same).
const isInitialized =  isParameter || isAlias || isOuterVariable || isSpreadDestructuringAssignmentTarget || isModuleExports || isBindingElement(declaration) ||
                                 type !== autoType && type !== autoArrayType && (!strictNullChecks || (type.flags & (TypeFlags.AnyOrUnknown | TypeFlags.Void)) !== 0 ||
                                 isInTypeQuery(node) || node.parent.kind === SyntaxKind.ExportSpecifier) ||
                                 declaration.kind === SyntaxKind.VariableDeclaration && (<VariableDeclaration>declaration).exclamationToken ||
                                 declaration.flags & NodeFlags.Ambient; 
const assumeInitialized = isInitialized || node.parent.kind === SyntaxKind.NonNullExpression; 
const initialType = isInitialized? (isParameter ? removeOptionalityFromDeclaredType(type, declaration as VariableLikeDeclaration) : type) :
                         type === autoType || type === autoArrayType ? undefinedType :
                         getOptionalType(type);

That is, split assumeInitialized to two expressions whether it really is initialized (isInitialized) or you can only assume it is through the bang syntax. initialType should only consider whether isInitialized is true. Using this code, it ran as expected.

My questions are so:

1. Is my implementation of isInitialized correct? My fear is missing some edge cases, more particularly, are there guards that behave similarly to the bang operator and should move from isInitialized to assumeInitialized?
2. I am having technical issues with editing the repository due to its size and it is a real bottleneck in my ability to contribute. Are there known quick fixes in VSCode to deal with a project this big? Tried to remove Intellisense completely but didn't manage to do it.
3. I understood I need to add some tests but didn't find a guide on how to do so, I'll appreciate if you point me at that direction.

[sandersn]

1. asserts guards now behave like the bang operator. I'm not sure you can detect them here or only in control flow. There might be some other cases I'm not thinking of².
2. I think VSCode turned on semantic highlighting recently, and it doesn't work well with checker.ts. Try turning that off? I personally use emacs, which has slower completions but better keystroke responsiveness.
3. Start with https://github.com/microsoft/TypeScript/blob/master/CONTRIBUTING.md#running-the-tests. Once you run all the tests, you'll see which fail because of your changes and you can copy+modify them to make new ones.

A couple of additional notes:

1. assumeInitialized is calculated the same way in 2 or 3 places throughout the compiler. You'll need to update them all to be the same.
2. I like your explanation but the code is so complex that I can't diff it without diff highlight. You should definitely create a PR at this point because it'll be easier to discuss with multiple people. (There are about 3-4 of us that know this code well enough to help.)

[Bremys]

Thanks for the reply, I'll create the PR and move the discussion there.
I'll check out the semantic highlighting and run the tests.

[Zzzen]

(There are about 3-4 of us that know this code well enough to help.)

The type checker is really complex, is there any public resources to learn about it? @sandersn

[aminpaks]

This is a very old issue I don't get the same error anymore or at least this works now what would be the solution?

function testInitializer(numbers: number[]) {
    let last; // any

    for (const n of numbers) {
        if (n % 2) {
            return n;
        }

        last = n; // assigning 'number' to 'any' works of course
    }

    return last; // still 'any' so whats the issue now?
}

cc @sandersn @weswigham

[eric-hu]

@aminpaks I still see the error in the Typescript playground with the 4.1.3 (current latest version). Your code snippet is missing the non-null assertion (return last; instead of return last!;).

[aminpaks]

@eric-hu ya I see. I'll see what I can do.

[nicolas377]

It seems this popped up at #14020 (issue #11463).