Skip to content

Question: is there a way to detect an unsound assignment via public compiler API? #3523

Closed
Closed
Question: is there a way to detect an unsound assignment via public compiler API?#3523
Labels
Domain: APIRelates to the public API for TypeScriptQuestionAn issue which isn't directly actionable in code

Description

@zpdDG4gta8XKpMCd
ZpdDG4gta (zpdDG4gta8XKpMCd)
opened on Jun 16, 2015

My situation (#3067):

interface A {
    x: string;
}

interface B {
    x: string;
    y: string;
}

function copyB(value: B): B {
    return undefined;
}

var values: A[] = [];
values.map(copyB); // <-- unsound assigment

I've managed to get an AST and a type checker. I am holding a reference to the argument (copyB from above) object.

Can anyone outline the steps that need to be taken to determine that the assignment from the example is unsound using the public compiler API?

Activity

  1. Dan Quirk (danquirk)
    added
    QuestionAn issue which isn't directly actionable in code
    Domain: APIRelates to the public API for TypeScript
    on Jun 17, 2015

  2. mhegazy commented on Jun 18, 2015

    @mhegazy
    Mohamed Hegazy (mhegazy)
    on Jun 18, 2015
    Contributor

    No. That is too deep in the stack at this point and not surfeced. Jason Freeman (@JsonFreeman) any ideas?

  3. DanielRosenwasser commented on Jun 18, 2015

    @DanielRosenwasser
    Daniel Rosenwasser (DanielRosenwasser)
    on Jun 18, 2015
    Member

    I believe you'd have to extract relations that you're interested from the compiler/create your own relations and test whether types in certain positions pass those relations.

  4. mhegazy commented on Jun 18, 2015

    @mhegazy
    Mohamed Hegazy (mhegazy)
    on Jun 18, 2015
    Contributor

    the relationship check might not be sufficient, if the relation was used in overload resolution, it would be too late at this point.

  5. DanielRosenwasser commented on Jun 18, 2015

    @DanielRosenwasser
    Daniel Rosenwasser (DanielRosenwasser)
    on Jun 18, 2015
    Member

    if the relation was used in overload resolution, it would be too late at this point.

    True. This and type argument inference might complicate things.

  6. JsonFreeman commented on Jun 18, 2015

    @JsonFreeman
    Jason Freeman (JsonFreeman)
    on Jun 18, 2015
    Contributor

    You'd have to do a couple of things:

    1. Augment checkTypeAssignableTo to take a flag indicating that you only want to allow sound assignments.
    2. Add the checks that you want to fail on, guarded by this flag.
    3. Add a relation cache that stores results for sound checks, so it doesn't mix with the existing cache.
    4. Expose this function on the type checker interface.

  7. mhegazy commented on Aug 10, 2015

    @mhegazy
    Mohamed Hegazy (mhegazy)
    on Aug 10, 2015
    Contributor

    looks like the original issue has been answered. closing.

  8. Mohamed Hegazy (mhegazy)
    closed this as completedon Aug 10, 2015

  9. zpdDG4gta8XKpMCd commented on Dec 13, 2015

    @zpdDG4gta8XKpMCd
    ZpdDG4gta (zpdDG4gta8XKpMCd)
    on Dec 13, 2015
    Author

    gentlemen, can this be considered a pull request: https://github.com/aleksey-bykov/TypeScript/commit/8a8b2a46fddc0e55494c7a009f077bf499b077a7 ?

  10. zpdDG4gta8XKpMCd commented on Dec 13, 2015

    @zpdDG4gta8XKpMCd
    ZpdDG4gta (zpdDG4gta8XKpMCd)
    on Dec 13, 2015
    Author

    works:

    const enum AsNonEmpty {}

let xx : <a, r>(
    values: a[] & AsNonEmpty,
    toResult: (value: a, index: number) => r,
    folding: (result: r, value: a, index: number) => r
) => r;

let yy: <a, r>(
    values: a[],
    toResult: (value: a, index: number) => r,
    folding: (result: r, value: a, index: number) => r
) => r;

xx = yy;
    src/array.ts (38,1): Type '<a, r>(values: a[], toResult: (value: a, index: number) => r, folding: (result: r, value: a, inde...' is not assignable to type '<a, r>(values: a[] & AsNonEmpty, toResult: (value: a, index: number) => r, folding: (result: r, v...'.
    Type 'any[]' is not assignable to type 'any[] & AsNonEmpty'.
      Type 'any[]' is not assignable to type 'AsNonEmpty'.

  11. DanielRosenwasser commented on Dec 13, 2015

    @DanielRosenwasser
    Daniel Rosenwasser (DanielRosenwasser)
    on Dec 13, 2015
    Member

    That's not actually correct, since parameters are contravariant when relating functions types. yy should be assignable to xx since yy expects less of its values parameter, so it's permissible to pass in more. The fix for this is to swap s and t in each call to isRelatedTo.

    Another problem is that you're not elaborating the error that was previously checked (Types_of_parameters_0_and_1_are_incompatible).

    Rather than completely failing, you should let the function continue the same logic, but additionally report an error (with the error function, not reportError) when encountering failure relatingt to s, but not s to t. The error should be "Parameter of type {0} was related to type {1} through covariance."

    Given that there are other issues with respect to soundness, I would consider calling this warnOnParameterCovariance.

    I wouldn't be opposed to introducing this change, but we'd need to discuss it at a design meeting.

  12. zpdDG4gta8XKpMCd commented on Dec 14, 2015

    @zpdDG4gta8XKpMCd
    ZpdDG4gta (zpdDG4gta8XKpMCd)
    on Dec 14, 2015
    Author

    yy should be assignable to xx since yy expects less of its values parameter, so it's permissible to pass in more

    I am afraid the whole point is that yy should not be assignable to xx. It's what I am trying the compiler to get me protected from. I agree that passing more should be permitted, but more is what what xx is, because it's on xx side where intersection of a[] & AsNonEmpty is and such intersection means a more elaborate type.

    Another problem is that you're not elaborating the error that was previously checked.

    The thing is that the original code (very convoluted in my opinion) does not do it either. All it does is finds the first incompatible pair of parameters and returns, albeit with a customized error message rather than one set by the isRelatedTo function. Speaking of which:

    The error should be "Parameter of type {0} was related to type {1} through covariance."

    You might be right, but I don't see a reason for why we should try covariance at all. The flag I am suggesting is about doing it right which is through contravariance. In my opinion we should keep the existing error message: Types_of_parameters_0_and_1_are_incompatible. Ideally peppered by the information that isRelatedTo provides:

    Type 'any[]' is not assignable to type 'any[] & AsNonEmpty'.
      Type 'any[]' is not assignable to type 'AsNonEmpty'.

    But I am not sure how to get a piece of one error message chain and attach it to another.

    Given that there are other issues with respect to soundness, I would consider calling this warnOnParameterCovariance.

    How about noParameterCovariance?

    I am sure if design team votes for this feature you guys will make it right. For now I will stick with my version which already makes my life so much easier. All in all, thanks for considering this change. Here is the last change that separates the original parameter matching logic from the one enabled by the flag: https://github.com/aleksey-bykov/TypeScript/commit/34b8f9b5a937cb7bb471771d5c9b9c4d4f629fc8

    PS
    You don't use automatic code formatting do you? Out of curiosity what IDE and its settings should be to avoid the mess with whitespaces?

  13. JsonFreeman commented on Dec 15, 2015

    @JsonFreeman
    Jason Freeman (JsonFreeman)
    on Dec 15, 2015
    Contributor

    Aleksey-Bykov The "more elaborate type" in your description actually corresponds to "being able to pass less" in Daniel Rosenwasser (@DanielRosenwasser)'s description. So Daniel Rosenwasser (@DanielRosenwasser) is correct that the two signatures should be assignable with respect to the first parameter at least (assuming contravariance).

  14. zpdDG4gta8XKpMCd commented on Dec 15, 2015

    @zpdDG4gta8XKpMCd
    ZpdDG4gta (zpdDG4gta8XKpMCd)
    on Dec 15, 2015
    Author

    been giving it an extra though, you both are right, ugh, let me figure out
    what I am fighting with

    Aleksey-Bykov https://github.com/aleksey-bykov The "more elaborate type"
    in your description actually corresponds to "being able to pass less" in
    Daniel Rosenwasser (@DanielRosenwasser) https://github.com/DanielRosenwasser's description. So
    Daniel Rosenwasser (@DanielRosenwasser) https://github.com/DanielRosenwasser is correct that
    the two signatures should be assignable with respect to the first parameter
    at least (assuming contravariance).

    —
    Reply to this email directly or view it on GitHub
    #3523 (comment)
    .

  15. microsoft
    locked and limited conversation to collaborators on Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Domain: APIRelates to the public API for TypeScriptQuestionAn issue which isn't directly actionable in code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions