void and undefined confusion with incremental clutz #695

Activity

1. rkirov

   added

   incremental clutz

   on Feb 16, 2018
2. evmar commented on Feb 16, 2018

   

   evmar

   on Feb 16, 2018

   Last edited by evmar

   Contributor

   More actions

   let x = someMyI.foo();
   

   is legal JS code at runtime, and probably the intended use of this API, so I think returning void is not what we want.

   Rather, I think this is just another case of inference where we should add an annotation on the subclass.
3. rkirov commented on Feb 16, 2018

   

   rkirov

   on Feb 16, 2018

   ContributorAuthor

   More actions

   If the input is:

   /** @interface */
   var MyI;
   
   /** @return {string | undefined} */
   MyI.prototype.foo = function() {};
   
   /** @constructor */
   var MyClassThatImplementsI;
   
   /** @return {undefined} */
   MyClassThatImplementsI.prototype.foo = function() {
     // no return;
   }
   
   exports.MyI = MyI;
   exports.MyClassThatImplementsI = MyClassThatImplementsI;

   we still emit:

   declare namespace ಠ_ಠ.clutz.module$exports$bare$reexport {
     class MyClassThatImplementsI extends MyClassThatImplementsI_Instance {
     }
     class MyClassThatImplementsI_Instance {
       foo ( ) : void ;
     }
     interface MyI {
       foo ( ) : string | undefined ;
     }
   }
   declare module 'goog:bare.reexport' {
     import alias = ಠ_ಠ.clutz.module$exports$bare$reexport;
     export = alias;
   }

   Because, we convert undefined in a return position to void to be more TypeScript-y. Maybe that was a mistake, and we should just emit undefined uniformly :/
4. rkirov commented on Feb 16, 2018

   

   rkirov

   on Feb 16, 2018

   ContributorAuthor

   More actions

   And to answer my question - "why this works in total mode" - it works because with plainly @override closure reports the interface type (not the inferred type).
5. LucasSloan commented on Feb 23, 2018

   

   LucasSloan

   on Feb 23, 2018

   Contributor

   More actions

   One way to solve this would be to add void to methods that have type unions that include undefined. Thus

   /** @return {string | undefined} */
   MyI.prototype.foo

   Would be translated to:

   declare interface I {
     f(): string | undefined | void;
   }

   And implementing that interface with a void method would be fine.
6. rkirov commented on Feb 24, 2018

   

   rkirov

   on Feb 24, 2018

   Last edited by rkirov

   ContributorAuthor

   More actions

   I have reservations because of this situation. Users might already have fn that accept string | undefined and when going through closure they will get assignability errors, that cannot trace back to the original .js source.

   declare interface I {
     f(): string | undefined | void;
   }
   
   function someOtherF(x: string | undefined) {
   
   }
   
   function f(x: I) {
     someOtherF(x.f());  // type error here
   }
7. rkirov commented on Mar 21, 2018

   

   rkirov

   on Mar 21, 2018

   ContributorAuthor

   More actions

   Two other answers here are:

   • swallow error in users' .ts code with 'any'
   • replace '@OverRide' with an explicit type annotation, because TypeScript does not have anything like @OverRide. If one redefines a method, they have to retype all the types (otherwise they are any).

   I am leaning towards the second answer.
8. evmar commented on Mar 21, 2018

   

   evmar

   on Mar 21, 2018

   Contributor

   More actions

   Fundamentally if you have the JS equivs of these in different compilation units:

   class A { foo(): A|undefined { ... } }
   
   class B { /** @override */ foo() { return undefined; } }
   

   Then there's no way we can rely on inference to pick the proper return type of foo for B. That is an argument for any I guess, separate from the void return problem.
9. rkirov commented on Mar 21, 2018

   

   rkirov

   on Mar 21, 2018

   Last edited by rkirov

   ContributorAuthor

   More actions

   Sorry, I am not following, isn't the correct type output:

   class A {
      foo(): A|undefined {...}
   }
   class B extends A {
      foo(): undefined;
   }

   This is valid TS, a subclass can change the return type as long it is a subtype (functions are covariant in the return position).
10. evmar commented on Mar 21, 2018

    

    evmar

    on Mar 21, 2018

    Contributor

    More actions

    Ah you are right. It's only if foo has no return statement and we infer void where this will break.
11. rkirov commented on Mar 21, 2018

    

    rkirov

    on Mar 21, 2018

    ContributorAuthor

    More actions

    Yep, which means one answer could be add 'return undefined', which has the same downside that some of these cases are in libraries that oppose changes just so that they are consumed by clutz.