Global variables in local functions are not control flow analysed #8472

Activity

1. kitsonk commented on May 5, 2016

   

   Kitson Kelly (kitsonk)

   on May 5, 2016

   Contributor

   More actions

   Essentially a dupe of #8720, #8367 and #7662 in that type guards get reset within functions, because the function body cannot be certain that is the only situation it will be invoked.

   You could easily invoke test() any number of places outside of the scope of the current static analysis... You are assume that a non-module file is the "whole" of the programme, TypeScript cannot make that assumption.
2. tinganho commented on May 5, 2016

   

   Tingan Ho (tinganho)

   on May 5, 2016

   ContributorAuthor

   More actions

   You could easily invoke test() any number of places outside of the scope of the current static analysis

   Though you can make the compiler check all those places for local functions and private methods.

   If it wheren't private method or local functions then I would agree.

   Here is the related class issue:

   class A {
     private p: string | undefined;
     public a() {
       if (this.p) {
         this.b()
       }
     }
     private b() {
       this.p.charAt(1); // no error because it is checked in this.a()
     }
   }
3. tinganho commented on May 5, 2016

   

   Tingan Ho (tinganho)

   on May 5, 2016

   Last edited by tinganho

   ContributorAuthor

   More actions

   Just to extend my explanation:

   The below code:

   let str: string | undefined;
   if (str) {
       test();
   }
   
   function test() {
       str.charAt(1); // error can be undefined
   }

   Could have been rewritten as:

   let str: string | undefined;
   if (str) {
       str.charAt(1);
   }

   Or

   class A {
     private p: string | undefined;
     public a() {
       if (this.p) {
         this.b()
       }
     }
     private b() {
       this.p.charAt(1); // no error because it is checked in this.a()
     }
   }

   Could be rewritten as:

   class A {
     private p: string | undefined;
     public a() {
       if (this.p) {
         this.p.charAt(1); 
       }
     }
   }

   These examples sort of entails that local functions/private methods are just inlined code blocks and therefore should be CFA:ed.
4. kitsonk commented on May 5, 2016

   

   Kitson Kelly (kitsonk)

   on May 5, 2016

   Contributor

   More actions

   There is a big different between private and global variables. While at runtime there is no such thing as private at design time, you are contracting that no one outside of the "knowable" code is going to touch a private variable. But maybe I am missing something and the TypeScript team will elucidate I am sure.
5. tinganho commented on May 5, 2016

   

   Tingan Ho (tinganho)

   on May 5, 2016

   ContributorAuthor

   More actions

   Kitson Kelly (@kitsonk)

   I think the compiler should consider whatever I declared it to be. Not based on what it is on runtime. If you try to access a declared private method publicly, you get a compile error anyway.

   FWIW the local function below is private on runtime:

   function f() {
     function local() {
     }
   }

   It also depends on your definitions of private. What I considered private on my OP, was for functions, it should be file scope private and every scope underneath it. And for class methods it is class scope private.
6. mhegazy commented on May 5, 2016

   

   Mohamed Hegazy (mhegazy)

   on May 5, 2016

   Contributor

   More actions

   this issue is actually a duplicate/flip side proposal of #8353. you can find more details about why we can not do this in #8353 (comment), and more discussion in the design meeting notes at #8388.

   where as it is theoretically possible to do some of these checks, by inlining function bodies in calls, or combining the control flow gram of all call sites as we enter a declaration, it would be a significant investment and a significant additional complexity tot he compiler logic that does not seem to warrant the value.
7. Mohamed Hegazy (mhegazy)

   closed this as completedon May 5, 2016
8. Mohamed Hegazy (mhegazy)

   added

   DuplicateAn existing issue was already createdAn existing issue was already created

   on May 5, 2016
9. tinganho commented on May 6, 2016

   

   Tingan Ho (tinganho)

   on May 6, 2016

   ContributorAuthor

   More actions

   I hope you can consider doing the checks inside functions and methods. It is really a big source of unwraps ! for me. It is amazing how the CFA works, though I consider it having a big hole and not being feature complete if it doesn't consider functions and methods.

   I preferably don't want to think that every refactor may introduce !. In an ideal world people shouldn't need to use !.
10. microsoft

    locked and limited conversation to collaborators on Jun 19, 2018