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

Incorrect circularity detection #33191

Closed
danvk opened this issue Sep 2, 2019 · 15 comments
Closed

Incorrect circularity detection #33191

danvk opened this issue Sep 2, 2019 · 15 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@danvk
Copy link
Contributor

danvk commented Sep 2, 2019

TypeScript Version: 3.6.2

Search Terms:

  • 7022
  • directly or indirectly in its own initializer

Code

function extent(nums: number[]) {
  let result: [number, number] | null = null;
  for (const num of nums) {
    if (!result) {
      result = [num, num];
    } else {
      const [oldMin, oldMax] = result;
      result = [Math.min(num, oldMin), Math.max(num, oldMax)];
    }
  }
  return result;
}

Expected behavior:

No error.

Actual behavior:

'oldMin' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022)
'oldMax' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022)

I'm really struggling to see where the circularity comes in. The type of result is shown as [number, number] in that branch, so I'd assume destructuring it would result in two number types.

Removing the destructuring and using array accesses is equivalent but produces no error:

function extent(nums: number[]) {
  let result: [number, number] | null = null;
  for (const num of nums) {
    if (!result) {
      result = [num, num];
    } else {
      result = [Math.min(num, result[0]), Math.max(num, result[1])];  // ok
    }
  }
  return result;
}

Playground Link: link

Related Issues:

@mprobst
Copy link
Contributor

mprobst commented Oct 21, 2019

here's another example from zone (source file wtf.ts):

  function shallowObj(obj: {[k: string]: any} | undefined, depth: number): any {
    if (!obj || !depth) return null;
    const out: {[k: string]: any} = {};
    for (const key in obj) {
      if (obj.hasOwnProperty(key)) {
        let value: any = obj[key];
        switch (typeof value) {
          case 'object':
            const name = value && value.constructor && (<any>value.constructor).name;
            value = name == (<any>Object).name ? shallowObj(value, depth - 1) : name;
            break;
          case 'function':
            value = value.name || undefined;
            break;
        }
        out[key] = value;
      }
    }
    return out;
  }

Gives:

zone.js/lib/zone-spec/wtf.ts:134:13 - error TS7022: 'value' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.

134         let value = obj[key];
                ~~~~~

@s0
Copy link
Contributor

s0 commented Mar 14, 2020

Another example i've just come across, TypeScript version 2.8.3:

function testFunction() {
  let state: { value: number } | null = null;
  for (let i = 0; i < 10; i++) {
    const previousState = state;
    //    ^^^^^^^^^^^^^ error: 'previousState' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022)
    state = null;

    const value = i === 0 ? 0 : previousState?.value;
    //    ^^^^^ error: 'value' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.ts(7022)

    if (value !== undefined) {
      state = { value: value + 1 }
    }
  }
  console.log('Test Function:', state?.value);
}

When putting in explicit type annotations, it produces a new error:

function testFunction() {
  let state: { value: number } | null = null;
  for (let i = 0; i < 10; i++) {
    const previousState: { value: number } | null = state;
    state = null;

    const value: number | undefined = i === 0 ? 0 : previousState?.value;
    //   Property 'value' does not exist on type 'never'.ts(2339): ^^^^^
    if (value !== undefined) {
      state = { value: value + 1 }
    }
  }
  console.log('Test Function:', state?.value);
}

@JoostK
Copy link
Contributor

JoostK commented Jan 27, 2021

Here's another case, but then with a while loop:

interface Node {
  next: Node | null;
}

function test(node: Node) {
  let currentNode: Node | null = node;
  while (currentNode !== null) {
    const nextNode = currentNode.next;
    //    ~~~~~~~~ error TS7022: 'nextNode' implicitly has type 'any' because it does not have a type annotation
    //                           and is referenced directly or indirectly in its own initializer.
    currentNode = nextNode;
  }
}

Playground link

@tonivj5
Copy link

tonivj5 commented Jan 27, 2021

BTW, using @JoostK's example, if you removes the asignation, error goes away... 😆

interface Node {
  next: Node | null;
}

function test(node: Node) {
  let currentNode: Node | null = node;
  while (currentNode !== null) {
    const nextNode = currentNode.next; // No error
   // currentNode = nextNode; // Uncomment it, and error comes back
    console.log(nextNode);
  }
}

@JoostK
Copy link
Contributor

JoostK commented Jan 27, 2021

BTW, using @JoostK's example, if you removes the asignation, error goes away... 😆

Removing the assignment removes the circular nature of currentNode, so that's expected. It ends up being circular due to the involvement of flow typing.

@MichaelMitchell-at
Copy link

A new one in 4.7.0-beta #48708

@vassudanagunta
Copy link

The error also goes away if you simply move just the initialization out of the loop:

function extentOne(nums: number[]) {
  let result: [number, number] | null = null;
  let oldMin, oldMax;
  for (const num of nums) {
    if (!result) {
      result = [num, num];
    } else {
      [oldMin, oldMax] = result;
      result = [Math.min(num, oldMin), Math.max(num, oldMax)];
    }
  }
  return result;
}

It also goes away if you remove null from the type:

function extentTwo(nums: number[]) {
  if (nums.length === 0) {
    return null
  }
  let result: [number, number] = [nums[0], nums[0]];
  for (const num of nums) {
    if (!result) {
      result = [num, num];
    } else {
      const [oldMin, oldMax] = result;
      result = [Math.min(num, oldMin), Math.max(num, oldMax)];
    }
  }
  return result;
}

Playground link

@danvk
Copy link
Contributor Author

danvk commented Feb 28, 2024

I went to take a look at this old issue today, only to find that @babakks had recently fixed it in #56753. Nice! 🎉

The example in the original issue already works on the TS 5.4-beta playground and the fix should be out in the final 5.4 release in the next few weeks.

@danvk danvk closed this as completed Feb 28, 2024
@JoostK
Copy link
Contributor

JoostK commented Feb 28, 2024

Unfortunately there's still a limitation for #33191 (comment), which continues to report an error (it's a scenario without binding elements). Interestingly, #33191 is quite similar but that one has been resolved in 5.0, so perhaps my case falls under #42529 (comment).

@danvk
Copy link
Contributor Author

danvk commented Feb 28, 2024

@JoostK maybe open up a new issue, then? I don't see why your code from #33191 (comment) should be flagged as circular.

@RyanCavanaugh
Copy link
Member

It is circular

  • What's the type of nextNode?
  • Depends on what the type of its initializer is
  • The initializer is currentNode.next. What's the type of the next property?
  • Depends on the possibly-seen inhabitants of the union. What are the types from the inbound control flows?
  • One inbound flow is from the assignment to currentNode from nextNode in the following line
  • What's the type of nextNode?
  • Hey that's the question we're asking

@danvk
Copy link
Contributor Author

danvk commented Feb 28, 2024

  • Depends on the possibly-seen inhabitants of the union. What are the types from the inbound control flows?
  • One inbound flow is from the assignment to currentNode from nextNode in the following line

What feels funny about this is that there's a declared type for next (Node | null). So couldn't this fall back to using that declared type, rather than raising a circularity error while trying to get a more precise one? Or would that open a different can of worms?

@RyanCavanaugh
Copy link
Member

I recommend reading #45213 for a long discussion of the practicality of "falling back"

@babakks
Copy link
Contributor

babakks commented Feb 29, 2024

@danvk I'm glad that fix partially helped with this issue.

@ahejlsberg
Copy link
Member

This issue is effectively a design limitation as @RyanCavanaugh describes here. The "fix" in #56753 is more of an accident because the logic in that PR suppresses circularity errors in manner that isn't consistent. We're reverting that logic in #59183 and the design limitation continues to exist.

@ahejlsberg ahejlsberg added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Bug A bug in TypeScript labels Jul 9, 2024
@ahejlsberg ahejlsberg removed this from the Backlog milestone Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests