FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Strictly check callback parameters by ahejlsberg · Pull Request #18976 · microsoft/TypeScript · GitHub

Strictly check callback parameters - #18976

Merged
Anders Hejlsberg (ahejlsberg) merged 6 commits into
masterfrom
strictCallbackParameters
Oct 6, 2017
Merged

Strictly check callback parameters#18976
Anders Hejlsberg (ahejlsberg) merged 6 commits into
masterfrom
strictCallbackParameters

Conversation

Anders Hejlsberg (ahejlsberg) commented Oct 5, 2017
edited
Loading

Copy link
Copy Markdown
Member

With this PR we fix checking of the return position of callback parameters in --strictFunctionTypes mode. Previously we'd never check this position strictly (because of #15104) but now we check it strictly (i.e. contravariantly) for callback parameters of non-methods. For example:

// Compile with --strictFunctionTypes
declare let f1: (cb: (x: Animal) => Animal) => void;
declare let f2: (cb: (x: Dog) => Dog) => void;
f1 = f2;  // Error, but previously wasn't
f2 = f1;  // Error

The way the fix works is that once we've decided to check a parameter position strictly, we avoid the code path for callback parameters added in #15104. Basically, when we're checking a parameter strictly, we don't need to make exceptions for callbacks.

Fixes #18963 (at least the parts that we can fix).

Copy link
Copy Markdown
Contributor

Anders Hejlsberg (@ahejlsberg) last time we talked about disabling this code path for --strictFunctionTypes you mentioned we needed this to avoid excessive recursion for computing the variance digest, is that not an issue now?

Copy link
Copy Markdown
Member

It feels strange that the following examples undergo a regression and you lose checking under strictFunctionTypes:

interface Animal {a: any}
interface Dog extends Animal {d: any}


namespace n1 {
    class Foo {
        static f1(x: Animal): Animal { throw "wat"; }
        static f2(x: Dog): Animal { throw "wat"; };
    }
    
    declare let f1: (cb: typeof Foo.f1) => void;
    declare let f2: (cb: typeof Foo.f2) => void;
    f1 = f2;
    f2 = f1; // errors outside strictFunctionTypes, not with it.
}

namespace n2 {
    type BivariantHack<Input, Output> = { foo(x: Input): Output }["foo"];
    
    declare let f1: (cb: BivariantHack<Animal, Animal>) => void;
    declare let f2: (cb: BivariantHack<Dog, Animal>) => void;
    f1 = f2;
    f2 = f1; // errors outside strictFunctionTypes, not with it.
}

At the very least, we should have tests demonstrating the behavior, but I think we can still tighten this up.

Copy link
Copy Markdown
Member Author

Daniel Rosenwasser (@DanielRosenwasser) It's because a callback parameter check (contravariant parameters, bivariant return type) sits halfway between a strict function type check (contravariant parameters, covariant return type) and a method type check (bivariant parameters, bivariant return type). I suppose we could say that a callback parameter check occurs only if the callback type isn't declared as method (which you really have to contort yourself to do, as your example demonstrates). I think it is largely immaterial though, it never really occurs in normal code.

Copy link
Copy Markdown
Member Author

last time we talked about disabling this code path for --strictFunctionTypes you mentioned we needed this to avoid excessive recursion for computing the variance digest, is that not an issue now?

Not sure what I was talking about there, it shouldn't matter.

Copy link
Copy Markdown
Member

While I think it wouldn't be super difficult to do that, we should acknowledge these cases and add them to our test suite, with and without strictFunctionTypes.

Copy link
Copy Markdown
Member Author

Daniel Rosenwasser (@DanielRosenwasser) Mohamed Hegazy (@mhegazy) With the latest commits I've reverted back to using the callback parameter code path in --strictFunctionTypes mode as well, but with a tighter check on the return type of the callback. This means that in --strictFunctionTypes mode we'll check function types loosely only when they originate in method or constructor declarations and aren't in a callback parameter position.

Anders Hejlsberg (ahejlsberg) deleted the strictCallbackParameters branch October 6, 2017 20:36

SlurpTheo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why !callbackCheck instead of CallbackCheck.None === callbackCheck?

Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

strictFunctionTypes has different behavior with parameter types and return types

5 participants


Back | FazBrowse Home | New Git URL