Skip to content

Make getter-only member non-assignable to getter+setter member #27538

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

Closed
3 of 4 tasks
igelbox opened this issue Oct 4, 2018 · 5 comments
Closed
3 of 4 tasks

Make getter-only member non-assignable to getter+setter member #27538

igelbox opened this issue Oct 4, 2018 · 5 comments
Labels
Duplicate An existing issue was already created

Comments

@igelbox
Copy link
Contributor

igelbox commented Oct 4, 2018

Search Terms

getter, setter

Suggestion

Prevent types with getter-only attributes (read-only behavior) to be assignable to types with setter attributes (read-write) behavior. This pretty suits the structural type system.

Use Cases

In 3.2.0-dev we're able to assign a read-only instance to a read-write typed variable and then try to assign those read-only attributes. However, this code will raise an error in the strict-mode runtime.
So, it would be better to catch this error during the compile-time.

Examples

// A simplified example:
class ReadOnly {
  get x() { return 0; }
}
class ReadWrite extends ReadOnly {
  get x() { return 0; }
  set x(v: number) { }
}

const s: ReadWrite = new ReadOnly(); // Actual: Ok, Expected: Error
s.x = 1; // Will cause `TypeError: setting getter-only property "x"` error in strict-mode runtime


// A real-world issue:
function f<T extends ReadOnly>(v: T) {
  if (v instanceof ReadWrite) {
    v.x = 2; // Fails with `[ts] Cannot assign to 'x' because it is a constant or a read-only property.`
    // it's because checker.ts#getNarrowedType() returns the original T type, just because isTypeAssignableTo(type, candidate).
  }
}

Well, the proposal it to fail on the following assignment:

const s: ReadWrite = new ReadOnly(); // Should be an error

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript / JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. new expression-level syntax)
@igelbox
Copy link
Contributor Author

igelbox commented Oct 4, 2018

Also, I tried to implement (by modifying tsc) this non-assignable behavior for the readonly attributes. However, I faced with a trouble with the existing code, e.g.:

interface I {
  method(): void;
}
class C implements I {
  readonly method = () => {};
}

function doAction(v: I) {}

doAction(new C()); // Works in 3.2.0-dev, but would've been broken
// if I'd implemented this feature for `readonly` attributes as well.

@ghost
Copy link

ghost commented Oct 4, 2018

Duplicate of #8496

@ghost ghost marked this as a duplicate of #8496 Oct 4, 2018
@ghost ghost added the Duplicate An existing issue was already created label Oct 4, 2018
@igelbox
Copy link
Contributor Author

igelbox commented Oct 4, 2018

@Andy-MS from my perspective, this's a kind of duplicate. Because it would be great to fix both issues (getter+setter and readonly). But, due to backward compatibility reasons and due to interfaces don't support readonly attribute now, I don't believe we'll be able to fix the readonly behavior in a short perspective.
However, we can fix this particular issue and leave #8496 for more investigation. Moreover, I can open PR today or tomorrow (but I still need to check the backward compatibility somehow, e.g. try to compile some huge TS project).

What do you think?

@ghost
Copy link

ghost commented Oct 4, 2018

These are the same problem though -- TypeScript considers a readonly property (with only a getter) assignable to one that isn't (with a getter and setter).

@typescript-bot
Copy link
Collaborator

This issue has been marked as a duplicate and has seen no activity in the last day. It has been closed for automatic house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

2 participants