Skip to content

Type guard question #1892

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
jbondc opened this issue Feb 2, 2015 · 9 comments
Closed

Type guard question #1892

jbondc opened this issue Feb 2, 2015 · 9 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead

Comments

@jbondc
Copy link
Contributor

jbondc commented Feb 2, 2015

module foo {
    export function on(event: string|Event) {
        if (typeof event === "string") {
            event = new Event(event) // (a)
            event = new Event(String(event)) // (b)
        }

        console.log(event.target) 
    }

    class Event {
        constructor(name: string) {}
    }
}

Is this a bug, known issue?

Using the latest tsc from master, get the errors:
(a) error TS2345: Argument of type 'string | Event' is not assignable to parameter of type 'string'.
(b) error TS2339: Property 'target' does not exist on type 'string | Event'.

@RyanCavanaugh RyanCavanaugh added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Feb 2, 2015
@RyanCavanaugh
Copy link
Member

Any assignment to a variable effectively "turns off" type guards on that variable.

@DickvdBrink
Copy link
Contributor

Spec part:

In the true branch statement of an 'if' statement, the type of a variable or parameter is narrowed by a type guard in the 'if' condition when true, provided the true branch statement contains no assignments to the variable or parameter

@jbondc
Copy link
Contributor Author

jbondc commented Feb 2, 2015

Hmm interesting. Thanks for the quick replies.
Intuitively in this case, I would have thought it would have narrowed back to Event.

Looks like it happens here:
https://github.com/Microsoft/TypeScript/blob/master/src/compiler/checker.ts#L4713

Will think about it some more

@RyanCavanaugh
Copy link
Member

Consider something like this:

function blah(x: string|number) {
  // Coerce numbers to string
  if(typeof x === 'number') {
    x = x.toString();
  }
  // do something with x now that it's a string
}

If we narrowed x to number, the assignment would fail because string is not assignable to number. Not a perfect example since you'd often need to cast x here anyway, but you get the idea.

@jbondc
Copy link
Contributor Author

jbondc commented Feb 2, 2015

Makes sense but feel like there should be an alternative to:

module foo {
    export function on(event: string|Event) {
        var evt: Event;
        if (typeof event === "string") {
            evt = new Event(event)
        } else {
            evt = event
        }
       }
       class Event {
           constructor(name: string) {}
       }
}

It's a pretty common pattern in JS.

@NN---
Copy link

NN--- commented Feb 3, 2015

@jbondc Your code compiles well in the current TS 1.4:
You can see it in the playground .

@jbondc
Copy link
Contributor Author

jbondc commented Feb 3, 2015

@RyanCavanaugh So widening on assignment 'By Design' is good

This is really a different feature of adding a check at the end of an if block statement and keeping track of type assignments. Some ~ pseudo-code:

module foo {
    export function on(event: string|Event) {
        if (typeof event === "string") {
            event = new Event(String(event))
            // on assignment, event type widens to 'string|Event' (good)
            // *but* keep track of assigned type Event
            /* guardTypeInfo.typeAssigns.push(Event) */
        }
        // checker.ts 
        // If all types in the set 'string|Event' have guard assignments of the same type, 
        // *narrow* to the assigned type
        /*
            if(guardTypeInfo) {
               var countAssign = guardTypeInfo.typeAssigns.length;
               if(guardTypeInfo.noElseClause && guardTypeInfo.unionType.types.length -1 == countAssign) {
                   var inferMissingType = ...
                   guardTypeInfo.typeAssigns.push(inferMissingType);
                   countAssign++
                }

                if(countAssign ==  guardTypeInfo.unionType.types.length) {
                    // If all types in 'guardTypeInfo.typeAssigns' are the same
                    narrowType =   guardTypeInfo.typeAssigns[0];
               }
            }
       */
        // event type is narrowed to 'Event'
        console.log(event.target) 
    }

    class Event {
        constructor(name: string) {}
    }
}

A couple more examples:

function blah(x: string|number) {
  if(typeof x === 'number') {
      x = x.toString();
      // x widens to 'string|number 
      // x is assigned a type 'string'
  }
  // end of if block
  // infer that missing else clause contains a guard assignment of type of 'string'
  // the number of assigned types (2) [string,string] == union types (2) [number,string]     

   // narrow the type to 'string' since assignments the same [string,string]
}

function blah(x: string|number) {
  if(typeof x === 'number') {
      x = x.toString();
      // x widens to string|number 
      // x is assigned a type 'string'
  }  elseif(typeof x === 'string') {
  }

   // narrow the type to 'string' since assignments the same [string,string] 
}

function blah(x: string|number|boolean) {
  if(typeof x === 'number') {
      x = x.toString();
      // x widens to string|number|boolean 
      // x is assigned a type 'string'
  }
   // number of assignments (2) [string,string] < (3) [string|number|boolean]
   // type x is still string|number|boolean
}

function blah(x: string|number|boolean) {
  if(typeof x === 'number') {
      x = true;
      // x widens to string|number|boolean 
      // x is assigned a type 'boolean'
  }  elseif(typeof x === 'string') {
      x = false;
      // x widens to string|number|boolean 
      // x is assigned a type 'boolean'
  }
  // end of if block
  // infer that missing else clause contains a guard & assignment of type of 'boolean'
  // the number of assigned types (3) [boolean|boolean|boolean] == union types (3) [string,number,boolean]

   // narrow the type to 'boolean' since assignments the same [boolean,boolean,boolean]
}

function blah(x: string|number|boolean) {
  if(typeof x === 'number') {
      // x narrows to number
  }  elseif(typeof x === 'string') {
      x = false;
      // x widens to string|number|boolean 
      // x is assigned a type 'boolean'
  }
  // end of if block
  // infer that missing else clause contains a guard & assignment of type of 'boolean'
  // the number of assigned types (3) [number,boolean,boolean] == union types (3) [string,number,boolean]

   // type x is still string|number|boolean since assignments not the same [number,boolean,boolean]
}

Using a nullish guard:

function blah(x: string|number|boolean) {
  if(x == null) {
     // x narrows to void type
     // no assignments
      x = null // ok
      x = void 0 // ok
  } elseif(typeof x === 'number') {
      // x narrows to number
  }  elseif(typeof x === 'string') {
       // x narrows to string
  } else {
       // x narrows to boolean
   }
  // end of if block
  // the number of assigned types (3) == union types (3) [string,number,boolean] 
   // type x is still string|number|boolean since assignments not the same [number,string,boolean]
   x = null // ok
   x = void 0 // ok
}

Using type constraints (related to #185):

function blah(x: string|number|boolean) {
  if(x == null) {
     // x narrows to void type
     // adds constraint x!null!undefined
      x = null // error
      x = void 0 // error
  } elseif(typeof x === 'number') {
      // x narrows to number!null!undefined
  }  elseif(typeof x === 'string') {
       // x narrows to string!null!undefined
  } else {
       // x narrows to boolean!null!undefined
   }
   // type x is string|number|boolean & !null!undefined
     x = null // error
     x = void 0 // error
}

Does this make sense? Can start with a partial patch.

@fdecampredon
Copy link

see #1769

@jbondc
Copy link
Contributor Author

jbondc commented Feb 4, 2015

So I have a patch for if(foo == null) #1806 but the other part is much harder with current code.
This issue is now more related to #1764

@mhegazy mhegazy closed this as completed Mar 24, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead
Projects
None yet
Development

No branches or pull requests

6 participants