Skip to content

Interface property not enforcing requirement for both getter and setter when implemented on a class #45517

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
frank-weindel opened this issue Aug 19, 2021 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@frank-weindel
Copy link

frank-weindel commented Aug 19, 2021

Bug Report

A property specified in an interface is not enforcing the requirement that both a getter and setter should exist when the interface is implemented on a class. See code example below...

🔎 Search Terms

interface getter setter accessor mutator property

🕗 Version & Regression Information

Seems to be an issue all the way from the nightly build back to 3.3.3 (as far as I can go back in the playground)

  • This is the behavior in every version I tried (in Playground), and I reviewed the FAQ for entries about interfaces, getters, and setters.

⏯ Playground Link

Playground link with relevant code

💻 Code

interface IMyInterface {
  property1: string; // Intuitively this should imply, when implemented, that it is both settable and gettable, regardless of the means
}

class MyClassWithSetterOnly implements IMyInterface { // <--- Expect error, because property1 is missing a getter (Callout A)
  private _property1: string = 'default';
  set property1(v: string) {
    this._property1 = v;
  }
}

class MyClassWithGetterOnly implements IMyInterface { // <--- Expect error, because property1 is missing a setter (Callout B)
  private _property1: string = 'default';
  get property1(): string {
    return this._property1;
  }
}

🙁 Actual behavior

  • The implements clause on Callout A's line above does not produce an error
  • The implements clause on Callout B's line above does not produce an error

🙂 Expected behavior

  • The implements clause on Callout A's line should produce an error similar to:
Class 'MyClassWithSetterOnly' incorrectly implements interface 'IMyInterface'.
  Property 'property1' is missing in type 'MyClassWithSetterOnly' but required in type 'IMyInterface'.ts(2420)
  • The implements clause on Callout A's line should produce an error similar to:
Class 'MyClassWithGetterOnly' incorrectly implements interface 'IMyInterface'.
  Property 'property1' is missing in type 'MyClassWithGetterOnly' but required in type 'IMyInterface'.ts(2420)

Understanding that missing half of a getter/setter set might produce a slightly different error for easier debugging and fix automation.

Alternative expectation

An alternative expectation may be for TypeScript to not allow a setter/getter to exist alone in any context (class, interface, object literal, etc). However, I imagine this would be very troubling for developers.

@RyanCavanaugh
Copy link
Member

The type system consistently treats properties only in terms of their "read" side; writes are always possibly-unsound due to e.g. the absence of a setter, or because the property type of the object is more specific than the property type specified by a reference to the object.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Aug 19, 2021
@jcalz
Copy link
Contributor

jcalz commented Aug 19, 2021

Looks like a duplicate of #36575 and/or #13347. It has always been the case that you can assign a readonly property to a mutable one and vice versa (so the "getter only" behavior is as designed). Since there is no concept of writeonly, I guess that properties with only set accessors are silently promoted to full read-write properties in the type system, so assignments between those are also just allowed. I don't think this is a great state of affairs, but there's not much to be done here I fear.

@jcalz
Copy link
Contributor

jcalz commented Aug 19, 2021

@RyanCavanaugh Do you think that the current behavior of setter-only accessors is as intended?

class Foo {
  set prop(x: string) { }
  // no definite assignment error for prop
}
const f = new Foo();
f.prop.toUpperCase(); // no error when reading f.prop 
// runtime error, f.prop is undefined

If so I guess I'll leave this alone. If not maybe I'll open a separate issue.

Playground

@frank-weindel
Copy link
Author

frank-weindel commented Aug 19, 2021

@RyanCavanaugh Thanks for the response. Does "Working as Intended" also mean "Working Correctly" in this case? Does that mean this will never be improved because of some fundamental type theory principle that TypeScript implements? Certainly the above scenario is in some way detectable. And I can't see why a developer wouldn't want to be informed at compile time of the runtime error and type inconsistency that occurs.

const setterOnly = new MyClassWithSetterOnly() as IMyInterface;
const getterOnly = new MyClassWithGetterOnly() as IMyInterface;

console.log(setterOnly.property1); // Property returns `undefined` which is incompatible with `string`!
console.log(getterOnly.property1);

setterOnly.property1 = 'value';
getterOnly.property1 = 'value'; // Runtime [ERR]: Cannot set property property1 of # which has only a getter 

Playground

@jcalz Thanks! I also just found #21759 regarding the suggestion of a writeonly keyword.

@frank-weindel
Copy link
Author

frank-weindel commented Aug 19, 2021

Other related but closed writeonly issues I've uncovered looking through #21759

However I'm not sure if the existence of writeonly is needed for the interface enforcement I'm calling out here. I'd expect by default a property in an interface is both read/write capable, and hence should enforce that either the property implemented by class/object literal is either 1) a direct value field, 2) or a type identical set of getter & setter

@jcalz
Copy link
Contributor

jcalz commented Aug 19, 2021

Dropping a reference to ESLint's accessor-pairs linter rule here for anyone who wants to catch setter-only properties.

@RyanCavanaugh
Copy link
Member

Without a concept of writeonly we don't have any way to detect that Foo should have an error (I agree it should, in principle).

Possibly with variant getters/setters we could consider synthesizing a get(): never accessor, but that doesn't fix very much because a never-returning getter would (correctly) be considered OK.

Fundamentally there's no type system difference between

class A implements HasM {
  set m(v) { }
}

and

class A implements HasM {
  get m() { throw new Error("ugh"); }
  set m(v) { }
}

so it's difficult to detect these via the normal assignability relationship. We'd need a separate additional pass on classes to validate that implemented properties didn't only have setters, but this costs cycles and is a fairly difficult mistake to make as a programmer.

@frank-weindel
Copy link
Author

@RyanCavanaugh Thanks for the explanation! That makes sense to me.

I think it's easier than you say to make the mistake. I have a case where I have an interface that some UI component implements. The setters update something visually in the component. Implementors of the interface may only opt to create setters since that's the minimum needed to get everything to work/compile. Later on, a user of the interface may opt to actually get one of those properties, and eventually find out later on at runtime that a particular implementation of the interface did not implement the getter. Implementations of this interface may exist in separate repositories adding to the complexity of getting the implementor to fix the issue. It would obviously be way more preferred if this was caught by the type checker.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

4 participants