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

Object is possibly null inside a closure defined within a const constraint #36193

Open
benallfree opened this issue Jan 15, 2020 · 15 comments
Open
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@benallfree
Copy link

benallfree commented Jan 15, 2020

Search: Object is possibly null, narrowed type, type constraint, closure

Code

const foo: string | null = 'bar'
if (foo) {
    function baz() {
        console.log(foo.length)  // Object is possibly 'null'
    }
}

Expected behavior:

Compiles

Actual behavior:

const foo: string | null
Object is possibly 'null'

Playground Link:

Playground

Related Issues:

#12113, #33319, #9998

Discussion:

By design, type narrowings are reset inside closures. This makes sense, but there is one case where better assumptions can be made: when the closure is defined inside a type constraint block AND the type narrowing is based on a const value. Even if called from async, the closure is still referencing a const and was created after a suitable type constraint occurred.

Thoughts?

@benallfree
Copy link
Author

benallfree commented Apr 8, 2021

Actually now I think this issue should be closed because the behavior is correct. Consider this:

let foo: string | null = 'bar'
const delegate: { baz: ()=>void }: { baz: ()=>{} }
if (foo) {
    function baz() {
        console.log(foo.length)  // Object is possibly 'null'
    }
    delegate.baz = baz
}
foo = null
delegate.baz() // runtime failure

edit April 24: this is a bad example, foo = null is not possible because foo is const

@johnsoncodehk
Copy link

johnsoncodehk commented Apr 22, 2021

In principle, TypeScript can check whether the function is allocated. (Not sure should I open a feature request.)

if (foo) {
    function baz() {
        console.log(foo.length)  // Object is 'string' | 'null'
    }
    delegate.baz = baz
}
if (foo) {
    function baz() {
        console.log(foo.length)  // Object is 'string'
    }
    // no assign for baz
}

Or design a keyword to prevent the function allocated.

if (foo) {
    static function baz() {
        console.log(foo.length)  // Object is 'string'
    }
    delegate.baz = baz // error: function can't be allocated because function is static
}

@benallfree
Copy link
Author

benallfree commented Apr 24, 2021

Hi @johnsoncodehk I think Typescript behaves correctly already. Consider this:

if(foo) {
   function baz() {
      console.log(foo.length)  // Object is possibly null
    }
    setTimeout(baz,100)
}

Since there is no way to guarantee that foo is defined at the time baz() is called in all cases, Typescript is correct to catch that it might be null even though foo was defined when baz() was created.

@johnsoncodehk
Copy link

johnsoncodehk commented Apr 24, 2021

@benallfree in this case baz is assign to a function param, so foo is possibly null, but if baz do not have any assign in the closure, but only have synchronize calling, foo will not be null.

But I just found a other case so we can't do that:

if(foo) {
  baz(); // foo is not null here
  foo = null
  baz(); // foo is null here but no type error
  function baz() {
     console.log(foo.length)
   }
}

@benallfree
Copy link
Author

@johnsoncodehk Right. Then do you agree also that this issue should be closed?

@johnsoncodehk
Copy link

Yes, I do. :)

@benallfree
Copy link
Author

Closing per discussion.

@benallfree
Copy link
Author

benallfree commented Apr 24, 2021

Sorry, reopening, I think I might be confusing myself. @johnsoncodehk this issue is about a const constraint, so the value of foo should never change. I think this issue may be valid after all.

const foo: string|null = 'bar' // I think this maybe should be type narrowed to 'string'
if(foo) { // TS should know that `foo.length` is always valid here
}

So maybe the real question is why const foo:string|null='bar' isn't type narrowed to foo:string?

@benallfree benallfree reopened this Apr 24, 2021
@johnsoncodehk
Copy link

johnsoncodehk commented Apr 27, 2021

This issue seem to have 2 problems now.

In you comment, it seem not a closure type narrowing problem, but is a problem of type picking priority in function.

const foo: string | null = 'bar'
function baz() {
  console.log(foo.length) // Object is possibly 'null'
}

In this case function preference to string | null not 'bar' even foo is const, I believe this is design to select the most initial definition.


(remove this part because updated before comment)

@benallfree
Copy link
Author

@johnsoncodehk foo = null is not allowed because foo is const.

@johnsoncodehk
Copy link

johnsoncodehk commented Apr 27, 2021

So our consensus is under this 2 conditions, variables type narrowing in closure function is feasible?

  1. function has no any assigned.
  2. variables is const.

@benallfree
Copy link
Author

function has no any assigned.

@johnsoncodehk Can you provide an example please?

variables is const

Yes this does seem to be an issue. Try this playground sample:

const foo: string | null = 'bar'

const baz1 = () => {
  console.log(foo.length) // OK (narrows to string)
}

const baz2 = function () {
  console.log(foo.length) // OK (narrows to string)
}

function baz3() {
  console.log(foo.length) // Error (possibly null)
}

console.log(foo.length) // OK (narrows to string)

@johnsoncodehk
Copy link

johnsoncodehk commented Apr 27, 2021

@johnsoncodehk Can you provide an example please?

Never mind just let we focus on your problem to avoid confusion.

Yes this does seem to be an issue. Try this playground sample:

Because baz3 will hoisting, when you put foo to bottom, all function has consistency behavior.

const baz1 = () => {
  console.log(foo.length) // Error (possibly null)
}

const baz2 = function () {
  console.log(foo.length) // Error (possibly null)
}

function baz3() {
  console.log(foo.length) // Error (possibly null)
}

const foo: string | null = 'bar'

@benallfree
Copy link
Author

Ah, function hoisting! I did not know this. From SO:

In Javascript, function statements are scoped within the containing function (or globally if there is no containing function), they're not scoped within an if or else or loop block.

In TS we can still know that the const assignment means that foo narrows to a string and that foo.length is always available. Do you agree that TS could handle this better? Or am I still not understanding?

@cefn
Copy link

cefn commented Feb 26, 2022

I believe typescript could handle this better. I encountered the problem with a real-world example like this, which I have simplified so it can live in a playground ...

type UrlString = `http${string}`;

interface Item {
  message: string;
}

interface Config {
  endpointUrl?: UrlString;
}

async function doSave(item: Item, endpointUrl: UrlString) {
  const response = await fetch(`${endpointUrl}/item`, {
    method: "POST",
    headers: {
      "Content-Type": "application/json",
    },
    body: JSON.stringify(item),
  });
  if (response.status !== 200) {
    throw new Error("Save unsuccessful");
  }
  return item;
}

function createHandle(config: Config) {
  const { endpointUrl } = config;
  if (!endpointUrl) {
    throw new Error(`Cannot create handle without endpointUrl config`);
  }

  // FUNCTION BASED APPROACH
  async function save(item: Item) {
    // compilation error Type 'undefined' is not assignable to type '`http${string}`'
    return await doSave(item, endpointUrl);
  }

  return {
    save,
  };
}

I have no choice but for the config object to have endpointUrl initially undefined, simply because of the framework I am using (it has to be populated at runtime before creating the 'handle' above). Hence the need for a runtime check during creation.

It is interesting that if I replace the later lines from // FUNCTION BASED APPROACH with the following, then the potential for endpointUrl to be undefined disappears, which could be a workaround for some cases...

// METHOD BASED APPROACH
  return {
      async save(item:Item){
          // no compilation error
          return await doSave(item, endpointUrl); 
      }
  };
}

However, this also suggests to me that typescript type narrowing inference is not doing its job correctly for the case that a function is defined within a closure over a const which is known (by earlier-executed runtime code) to be set. If it can handle the method case, then it should handle the function case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants