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

Cast is allowed between property and lone getter/setter #36575

Closed
TimvdLippe opened this issue Feb 2, 2020 · 5 comments
Closed

Cast is allowed between property and lone getter/setter #36575

TimvdLippe opened this issue Feb 2, 2020 · 5 comments
Assignees
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.
Milestone

Comments

@TimvdLippe
Copy link
Contributor

TypeScript Version: 3.7.5

Search Terms: getter, setter, checkjs

Code

cast.js

class Interface {
  constructor() {
    /** @type {!Array<number>} */
    this.prop;
  }
}

// This should (and does!) error, as they have different types
const cast = /** @type {!Interface} */ ({
  prop: 5
});

// This should error, as the prop is missing
const castWithoutProp = /** @type {!Interface} */ ({});

// This should error, as it has no correspnding getter
const castWithoutGetter = /** @type {!Interface} */ ({
  /** @param {!Array<number>} newValue */
  set prop(newValue) {}
});

// This should error, as it has no correspnding setter
const castWithoutSetter = /** @type {!Interface} */ ({
  /** @return {!Array<number>} */
  get prop() {
    return [];
  }
});

// This should (and does!) pass, as it has both the setter and the getter
const castWithSetterAndGetter = /** @type {!Interface} */ ({
  /** @param {!Array<number>} newValue */
  set prop(newValue) {},
  /** @return {!Array<number>} */
  get prop() {
    return [];
  }
});

tsconfig.json sets checkjs and allowjs to true.

Expected behavior:
Casts with missing getters/setters should error. Note that I did find #21759 which states that TS has no concept of writeonly. However, since omitting a setter also does not error, I concluded that checkjs semantics are probably different. Therefore I deduced that this is an issue with checkjs, rather than a semantics difference in .ts.
Actual behavior:
No errors on any cast that misses a getter/setter.

Playground Link: File cast.js in https://codesandbox.io/s/typescript-playground-export-sg90u

Related Issues: #21759

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript checkJs Relates to checking JavaScript using TypeScript Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels Feb 4, 2020
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 3.9.0 milestone Feb 4, 2020
@sandersn sandersn removed Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". checkJs Relates to checking JavaScript using TypeScript Help Wanted You can do this labels Mar 17, 2020
@sandersn
Copy link
Member

Note that this is not Javascript-specific -- it applies to Typescript as well:

class Interface {
    prop = [1,2]
}
const cast = { prop: 5 } as Interface
const castwoProp = {} as Interface
const castwoGetter = { set prop(newvvalue: number[]) { } } as Interface
const castwoSetter = { get prop(): number[] { return [] } } as Interface
const castWSetterGetter = {
    get prop(): number[] { return [] },
    set prop(newv) { }
} as Interface

Casts in TS are allowed between any types that are assignable to each other, so it's fine to cast {} as Interface and new Interface() as {} but not "s" as number.

If you try the stricter check of assignability:

class Interface {
    prop = [1,2]
}
const cast: Interface = { prop: 5 }
const castwoProp: Interface = {}
const castwoGetter: Interface = { set prop(newvvalue: number[]) { } }
const castwoSetter: Interface = { get prop(): number[] { return [] } }
const castWSetterGetter: Interface = {
    get prop(): number[] { return [] },
    set prop(newv) { }
}

You get an error for {} but not the subsequent ones. (The equivalent declaration in JS is:

/** @type {Interface} */
const cast = {}

I personally think it's fine for a cast to ignore problems with accessors vs properties. I'm not so sure about assignment. This is an area where the old 1.8 spec might explain what the intended behaviour is and (maybe) why.

@sandersn sandersn changed the title Checkjs does not type check getter/setter types Cast is allowed between property and lone getter/setter Mar 17, 2020
@sandersn sandersn added the Needs Investigation This issue needs a team member to investigate its status. label Mar 17, 2020
@sandersn
Copy link
Member

So here's the spec, 3.11.4, which says that a source S is assignable to a target T when, for all properties N and M:

M is a property [of T] and S has an apparent property N where

  • M and N have the same name,
  • the type of N is assignable to that of M,
  • if M is a required property, N is also a required property, and
  • M and N are both public, M and N are both private and originate in the same declaration, M and N are both protected and originate in the same declaration, or
  • M is protected and N is declared in a class derived from the class in which M is
    declared

The rest of the spec doesn't distinguish between accessors and properties (it calls them "member accessors" and "member variables", and calls them together "properties"), except to forbid accessors from overriding properties.

Currently, I'm not sure whether this should be tagged "Design Limitation" or "Working as Intended". In the former case, we could think about how to fix it without breaking existing programs.

@sandersn sandersn modified the milestones: TypeScript 3.9.0, Backlog Mar 18, 2020
@sandersn sandersn added Design Limitation Constraints of the existing architecture prevent this from being fixed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. and removed Needs Investigation This issue needs a team member to investigate its status. labels Mar 18, 2020
@sspi
Copy link

sspi commented May 7, 2021

I have a similar regression. Simple Testcase:

class A {
	nodeByField: Node;

	nodeByFunction(): Node {return null}

	get nodeByGetter(): Node {return null}
}

class AForElements extends A {
}

interface AForElements extends A {
	nodeByField: Element

	nodeByFunction(): Element

	nodeByGetter: Element //TS2610: 'nodeByGetter' is defined as an accessor in class 'A', but is overridden here in 'AForElements' as an instance property.
}

The class AForElements is only used for Element (that extends Node). So I want to explicitly cast in AForElements my field method and getter. This test case works perfectly in TS 3.7.3 but fails in newer TS versions (even in 4.3.0beta) only for the getter (see the msg in comment).

I don't find an other way for casting the getter. I missed something?
Allowing something like:

interface AForElements extends A {
	get nodeByGetter(): Element
}

would be a solution?

Note 1: in the issue title, I suppose it is "none" and not "lone"?
Note 2: I'm not sure if it is exactly the same issue, I can open a new one if not...

@sspi
Copy link

sspi commented May 7, 2021

Yes it's exactly what I'm waiting for. Sorry for bothering, my mistake is that I work in Webstorm and it does not recognize this new syntax yet! (and for the other errors in your playground, my TsConfig is in "strictNullChecks": false mode)
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.
Projects
None yet
Development

No branches or pull requests

5 participants