28911 - change Diagnostic.code to 'string | number'

[a-tarasyuk]

Fixes #28911

[ajafff]

Should the same change be made to DiagnosticWithLinePosition and DiagnosticRelatedInformation?

[a-tarasyuk]

@ajaff ok, will add changes to DiagnosticWithLinePosition, DiagnosticRelatedInformation.

[DanielRosenwasser]

The unfortunate thing here is that we're going to cause a few bugs where people create a Set<number | string> and end up using .has on the wrong type. But I think this is fine.

[DanielRosenwasser]

Hey @amcasey, can VS handle non-numeric diagnostic codes?

[amcasey]

@DanielRosenwasser, I believe so, but I'll confirm.

Edit: Yes, it can.

[amcasey]

@DanielRosenwasser I misspoke - Roslyn can accept strings but TS deserializes them as int so old versions of VS will break (probably by dropping these diagnostics).

But we can definitely have newer versions of VS opt into receiving string error codes.

[DanielRosenwasser]

Sounds like we'd need to change our API to also support a new configuration option on to say that the client can support string error codes. Even if we decided to add this functionality, we would need to keep numeric error codes around f o r e v e r.

As terrible as it sounds, it might be better to return a new property on Diagnostic objects.

[DanielRosenwasser]

@weswigham

[sheetalkamat]

@DanielRosenwasser Your suggestion doesnt sound terrible. Infact i like having optional friendlyName property sounds good. That way even we could add the names to error codes (instead of numbers) and editors can decide to use them over number if present to displayed. We should keep errorCode required so that editors not handling the additional optional property can show something that user can search about.

[weswigham]

Friendly human readable short names are good. Although question, if we go there: Can multiple messages share a short name (so, eg, we can group all the excess property variant messages under one "name")? I don't really see why not.

[a-tarasyuk]

I think this PR has to be closed.

[minestarks]

@DanielRosenwasser just to clarify on #28959 (comment) I feel like a configuration option isn't necessary if we simply add a new optional friendlyName field - errors that support friendly names would always populate the friendlyName and old editors would just ignore it.