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

Type information should be preserved with polymorphic this #23548

Closed
DavidANeil opened this issue Apr 19, 2018 · 8 comments
Closed

Type information should be preserved with polymorphic this #23548

DavidANeil opened this issue Apr 19, 2018 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@DavidANeil
Copy link

When the return type of a function is the this type, uses of that function should preserve the type information of the left hand operand.

See example:

class TestClass {
  constructor(public member: number) { }

  fakeCopy() {
    return this;
  }

  copy() {
    return new TestClass(this.member);
  }
}

const tester: Readonly<TestClass> = new TestClass(5);

tester.member = 6; // This properly errors.
tester.copy().member = 6; // This properly does not error.
tester.fakeCopy().member = 6; // This should error.

In this example fakeCopy returns a TestClass; it would be more intuitive if fakeCopy would return typeof tester which would be Readonly<TestClass>

@ghost
Copy link

ghost commented Apr 19, 2018

Duplicate of #10725? Readonly just makes the properties readonly, but has no effect what happens when you call a method.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 19, 2018
@RyanCavanaugh
Copy link
Member

Not to mention it'd be flat-out broken if ReadOnlyArray<T>#slice returned a ReadOnlyArray<T> instead of an Array<T>.

@DavidANeil
Copy link
Author

@RyanCavanaugh, I would assume that a function like slice would not be returning the this type.

@DavidANeil
Copy link
Author

@andy-ms, This suggestion is not particular to Readonly, I just used it as an example where type information can be added on top of a variable, but then not preserved when using a function that returns this.

It seems intuitive to me that if a function is returning this then I should neither gain nor lose type information by using it.

@RyanCavanaugh
Copy link
Member

It doesn't seem to make sense for the this type to "know" what its surrounding context is, because there's no way for the implementation of a this-returning function to correctly adhere to whatever the transform of its parent represents.

@DavidANeil
Copy link
Author

This would be a large change, and would need its own proposal, but what do you think of a function that uses the this value to, in addition of having the implicit type of this as the surrounding class, having a check at any call site of the function that the typeof lefthandoperand is usable as the this in the given function.

Maybe an example would illustrate:

class TestClass {
  member = 5;

  // current and proposed typechecker: fakeCopy passes checks
  fakeCopy() {
    return this;
  }

 // current and proposed typechecker: mutatingFunction passes checks (member is not marked readonly).
  mutatingFunction() {
    this.member = 6;
  }
}

const tester: Readonly<TestClass> = new TestClass();

// current typechecker: function 'mutatingFunction' exists on typeof tester. Valid call.
// proposed typechecker: TestClass#mutatingFunction(this: typeof tester) fails because property member is readonly.  Invalid call.
tester.mutatingFunction();

// current typechecker: tester.fakeCopy() returns a TestClass.
// proposed typechecker: tester.fakeCopy() passes typechecking because TestClass#fakeCopy(this: typeof tester) passes.  Inferred return type is `typeof tester`
let copy = tester.fakeCopy();

This would fix both my problem and the need for a DeepReadonly type that protects against calling mutating functions that others seem to have.

Thoughts?

@RyanCavanaugh
Copy link
Member

We've had lots of issues that boil down to "Implement C++-style const correctness"; start at #12

@DavidANeil
Copy link
Author

I am closing this in favor of #23554

@microsoft microsoft locked and limited conversation to collaborators Jul 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

2 participants