Typescript: Poor deprecated messages - types should almost never be displayed.

[DanielRosenwasser]

/** @deprecated - this is bad advice */
declare function crossTheStreams(): void

crossTheStreams()

Expected: 'crossTheStreams' is deprecated.
Actual: '(): void' is deprecated

Notice the type in place of the entity name, notice the lack of a period at the end.

[sandersn]

Note: This should normally happen only with overloads. It's not great with overloads either, so probably the overload message should be '{0}' with type '{1}' is deprecated. instead of putting a type into '{0}' is deprecated

[AnyhowStep]

I assume this isn't as simple as using signature.declaration.name.escapedText?

Like, signature.declaration could be JSDocSignature.
How would you get the function name from a JSDocSignature?

[DanielRosenwasser]

This should normally happen only with overloads.

Only if not every overload is deprecated. If all overloads are deprecated, showing the type doesn't help.

I assume this isn't as simple as using signature.declaration.name.escapedText?

It is better to use one of the various helpers for printing out the expression when a name is available. We've done this in a couple of places where we try to special case this when the invoked expression is an identifier/property access.

[DanielRosenwasser]

tryGetPropertyAccessOrIdentifierToString is one

[DanielRosenwasser]

getCalledExpressionName is also similar, but is weird in that it'll give a partial result if the expression gets a bit too complex (e.g. foo().bar.baz will give you bar.baz).

Probably what you want is just to do something even more basic where you try grabbing the name.text off a property access, the text off an Identifier, and stop there. Not sure of any other helper functions that do this, but I'm sure this isn't the only place.

[AnyhowStep]

I see getNameOfDeclaration, too

[DanielRosenwasser]

The declaration name isn't always useful because it might be anonymous (it might just be a call signature, it might come from a default import). The most relevant name for this error message is the one that is being used locally.

Something useful might be to provide a related span linking back to where the deprecation comes from, so that would link users back to the relevant declaration.

The declaration was marked as deprecated here.

[AnyhowStep]

I have no clue what I'm doing but I tried something like this _{(I should remove the "lol")},

        function checkDeprecatedSignature(signature: Signature, node: CallLikeExpression) {
            if (signature.declaration && signature.declaration.flags & NodeFlags.Deprecated) {
                const suggestionNode = getDeprecatedSuggestionNode(node);
                const deprecation = getJSDocDeprecatedTag(signature.declaration);
                if (deprecation == undefined) {
                    errorOrSuggestion(/*isError*/ false, suggestionNode, Diagnostics._0_is_deprecated, signatureToString(signature));
                } else {
                    const diagnostic = createDiagnosticForNode(
                        suggestionNode, 
                        Diagnostics._0_is_deprecated, 
                        signatureToString(signature) + "lol"
                    );
                    addRelatedInfo(
                        diagnostic,
                        createDiagnosticForNode(deprecation, Diagnostics.The_implementation_signature_is_declared_here)
                    );
                    suggestionDiagnostics.add({ ...diagnostic, category: DiagnosticCategory.Suggestion });
                }
            }
        }

And got something like this,

(I know it's the wrong diagnostic lol)

---

I'm surprised getJSDocDeprecatedTag() works for this,

[DanielRosenwasser]

(I should remove the "lol"),

But it's so fun!

Yeah, I don't know what the code currently looks like, but on the node, you should be able to getInvokedExpression and get the text using any of the methods I described above.

[AnyhowStep]

I noticed this doesn't produce the deprecation notice on foo(blah)

getCalledExpressionName seems to be in src/services/navigationBar.ts and not exported.

---

Huzzah!

---

Regarding overloads, this is pretty good. (ignore the eslint red squigglies)

[AnyhowStep]

I think I've got it down,

[AnyhowStep]

@DanielRosenwasser
Is it normal for Symbol.declarations to be a non-empty array but for valueDeclaration to be undefined?

I assumed valueDeclaration was just a shorthand for declarations[0].

This happens in tests/cases/fourslash/jsdocDeprecated_suggestion1.ts

Smaller repro,

//a.ts
/** @deprecated */
export function bar() {

}

/** @deprecated */
export type QW = 3;

//b.ts
import { bar, QW } from './a';

The import of bar is fine. It has valueDeclaration set.

However, QW has declarations.length == 1 but valueDeclaration == undefined.

---

[AnyhowStep]

It also seems like tryGetPropertyAccessOrIdentifierToString() doesn't like namespaces + optional chains.

In tests/cases/fourslash/jsdocDeprecated_suggestion1.ts,
the line containing foo?.[[|"faff"|]]?.() trips up tryGetPropertyAccessOrIdentifierToString().

Minimal repro,

export namespace foo {
    /** @deprecated */
    export function faff() { }
}

foo?.["faff"]?.();

---

If you replace the namespace with a variable, it works.

---

Adding this seems to fix it,

        else if (isElementAccessExpression(expr)) {
            const baseStr = tryGetPropertyAccessOrIdentifierToString(expr.expression);
            if (baseStr !== undefined && isPropertyName(expr.argumentExpression)) {
                return baseStr + "." + getPropertyNameForPropertyNameNode(expr.argumentExpression);
            }
        }

---

Unfortunately... It fails in this case,

[Kingwl]

Is it normal for Symbol.declarations to be a non-empty array but for valueDeclaration to be undefined?
I assumed valueDeclaration was just a shorthand for declarations[0].

Basically, As the name of valueDeclaration, Only existed when some symbol could be an value

[AnyhowStep]

It trips me up, though, because I see it declared as type Declaration, not Declaration|undefined.

It also doesn't make sense for a symbol to exist without a declaration, in my head.

[Kingwl]

First value declaration means first declaration of value

You can also find all declarations(includes valueDeclaration) in the declarations field

[AnyhowStep]

I see I had the wrong idea of what valueDeclaration was. It's conditionally set.