Add related error spans for getter/setters with different types

[DanielRosenwasser]

Now that we support multiple related spans for errors (#10489, #22789, #24548), we'd like to improve an existing error message.

Currently, we provide a diagnostic for a pair of get/set accessor's types not matching:

Code:

let x = {
  get foo() { return 100; }
  set foo(value: string): { }
}

Current error:

'get' and 'set' accessor must have the same type.

We'd like to give a better error message. For example:

Primary span:

A 'get-' and 'set-' accessor must have the same type, but this 'get' accessor has the type '{0}'.

Related span:

The respective 'set' accessor has the type '{0}'.

[bashdx]

Hello guys,

if you could point me in the correct direction (where do I find the error messages) I will take a look at it.
Thank you and regards
bashdx

EDIT:

If I got it right it revolves around src/compiler/diagnosticMessages.json - there you have a bunch of JS objects like

"'get' and 'set' accessor must have the same type.": { "category": "Error", "code": 2380 },

The build will generate the appropriate JS in for example tsc.js?
How do you inject the type information in there if you had format parameter in the string?

[mhegazy]

The issue in the OP is about adding additional information to the error message. We have recently allowed an error message to include child messages (to add clarifications), we call these related spans. See #25359 for a similar change.

[bashdx]

Thank you for your reply. I built the compiler and tried the following with your code snippet from above:
`$ node tsc.js --target "ES5" test_accessors.ts
test_accessors.ts:4:3 - error TS2322: Type '100' is not assignable to type 'string'.

4 return 100;
~~~~~~~~~~~

test_accessors.ts:6:26 - error TS1005: '{' expected.

6 set foo(value: string):{}
~
`

I can't get error 2380 to be invoked. Could you please provide a fitting code snippet. Using a class instead of an object resulted in a similar result.

Thank you and regards

[mhegazy]

Try this instead:

let x = {
    get foo(): number { return 100; },
    set foo(value: string) { }
}

[bashdx]

That did it! I am getting closer - could you please tell me, how to create a "blank" node? The current result looks like this (please ignore the unrelated error message about generators - this was solely for testing). Where would I create the message for the related span?

built/local/test_accessors.ts:2:9 - error TS2380: 'get' and 'set' accessor must have the same type, but this 'get' accessor has the type 'number'.

2     get foo(): number { return 100; },
          ~~~

  built/local/test_accessors.ts:3:9
    3     set foo(value: string) { }
              ~~~
    Generators are not allowed in an ambient context.

built/local/test_accessors.ts:3:9 - error TS2380: 'get' and 'set' accessor must have the same type, but this 'get' accessor has the type 'string'.

3     set foo(value: string) { }
          ~~~

  built/local/test_accessors.ts:2:9
    2     get foo(): number { return 100; },
              ~~~
    Generators are not allowed in an ambient context.

[mhegazy]

not sure why you are getting the error Generators are not allowed in an ambient context...

[bashdx]

This was just a random error message I picked to test the behavior of my changes.

[mhegazy]

Ahh.. take a look at #25377 then.

[mhegazy]

To add a new error message, you need to edit https://github.com/Microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json

see 4459730#diff-e119cc28df9582f9e1ca0d41720ea142

[bashdx]

The rest of the output can stay like this? This very format? Also it seems the error handler is invoked twice, while the former aims for the getter (inteded) and the latter aims for the setter, but uses the getter error message. I will check it out ASAP.

[bashdx]

I'd appreciate feedback if everything went fine, please.
Find pull request here.

[RyanCavanaugh]

Open to ideas here that have some concrete improvement

[acagastya]

There is no restriction on get set in ECMA specification, so adhering to the standard, shouldn't this restriction be lifted? I mean "'get' and 'set' accessor must have the same type".

[sirian]

setters and getters MAY HAVE different types

get zIndex(): string
set zIndex(value: string | number | null | undefined)

Another example:

class MY_URL {
    get query(): Record<string, string>
    set query(value: string | Record<string, string | number | null |undefined>);
}

const url = new MY_URL();
url.query = "foo=1&bar=2" // string setter
url.query = {foo: 1, bar: "2"} // Record<string, ...> setter

assert("1" === url.query.foo && "2" === url.query.bar)

@RyanCavanaugh

[martinheidegger]

To me its a reasonable limitation that getter/setter should not have different type but this should imo. be handled by a linter rather than the compiler.

What I have been thinking is that this might be related to the writeonly proposal #21759. Maybe having a type declaration such as the following could help to get a mental model for how getters/setters have different types:

interface IMY_URL {
  readonly query: Record<string, string>
  writeonly query: string | Record <string, string | number | null | undefined>
}

Another real-life example I have for this is the sketch api which does C mappings and has properties that can be set with partial objects but the getters will return filled-out instances.

[acagastya]

@martinheidegger that limitation is not per ECMA standards and only serves as additional gatekeeping.

[szmarczak]

I completely agree with @acagastya and @sirian. Different setters/getters are very useful when doing normalization.

/cc @sindresorhus