Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Refactor signature relatability #6142

Merged
merged 16 commits into from
Jan 16, 2016
Merged

Conversation

DanielRosenwasser
Copy link
Member

This PR is an attempt to remove duplicated code brought in by #6075.

It is still a work in progress, since there has been a small regression in error reporting.

@sandersn and @JsonFreeman, you two might want to take a look.

@@ -3,8 +3,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
Types of parameters 'arg2' and 'arg2' are incompatible.
Type '{ foo: number; }' is not assignable to type 'Base'.
Types of property 'foo' are incompatible.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it a regression to drop these lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. I've noted that elsewhere in the PR. Mostly I'm trying to see whether this is an appropriate direction to move in. I need to figure out how to maintain this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these losses primarily because of the covariant / contravariant reversal, or something else in the change? It might be good to swap them back, just to see if it makes the errors go away.

@DanielRosenwasser
Copy link
Member Author

I tried reverting the order of source/target on parameters to recover the lost error elaborations. Unfortunately, that introduces a bug in which for the following:

function foo<T extends "foo">(f: (x: T) => T) {
    return f;
}

let f = foo((y: "foo" | "bar") => y === "foo" ? y : "foo")

We report

    let f = foo((y: "foo" | "bar") => y === "foo" ? y : "foo");
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2345: Argument of type '(y: "foo" | "bar") => string' is not assignable to parameter of type '(x: "foo") => "foo"'.
!!! error TS2345:   Type 'string' is not assignable to type '"foo"'.
!!! error TS2345:     Type '"foo" | "bar"' is not assignable to type '"foo"'.
!!! error TS2345:       Type '"bar"' is not assignable to type '"foo"'.

Which is

  1. An elaboration of the source parameter type being unassignable to the target one (see the bottom two messages)
  2. An elaboration on the source return type being unassignable to that of the target. This is because compareTypes succeeded in assignability of the target parameter type to the source parameter type, and we're still using the same elaboration stack.

I'd rather not introduce a callback to revert the elaboration stack between calls to compareTypes, so I think I'm going to try to find a way to make the current code work.

@DanielRosenwasser DanielRosenwasser force-pushed the refactorSignatureRelatability branch from df72843 to 4502213 Compare January 14, 2016 07:17
@DanielRosenwasser
Copy link
Member Author

Hey @sandersn and @JsonFreeman any other feedback? I think I've improved error baselines by elaborating in most cases except for primitives. I've also simplified the code by eliminating that elaborateErrors variable.

@@ -26,10 +26,13 @@ tests/cases/compiler/arrayAssignmentTest1.ts(70,1): error TS2322: Type 'C3[]' is
Property 'C2M1' is missing in type 'C3'.
tests/cases/compiler/arrayAssignmentTest1.ts(75,1): error TS2322: Type 'C2[]' is not assignable to type 'C3[]'.
Type 'C2' is not assignable to type 'C3'.
Property 'CM3M1' is missing in type 'C2'.
Copy link
Member

Choose a reason for hiding this comment

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

The primitive baselines seem good to simplify, but what about these? Is it OK to drop these elaborations?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, maybe I shouldn't be reviewing code while I'm sick. I didn't see that it's an insertion not a deletion.

@sandersn
Copy link
Member

Looks good to me. But as I said, I'm sick, so make sure @JsonFreeman or future-me (the not-sick one) okays it too.

@RyanCavanaugh
Copy link
Member

Reviewed the error baselines in detail and it's a great improvement. I'm not as familiar with this part of the signature checking code so I'd defer to someone else on that front -- there was nothing obvious wrong about it, though.

DanielRosenwasser added a commit that referenced this pull request Jan 16, 2016
@DanielRosenwasser DanielRosenwasser merged commit 31bbac3 into master Jan 16, 2016
@DanielRosenwasser DanielRosenwasser deleted the refactorSignatureRelatability branch January 16, 2016 23:42
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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.

5 participants