Binding element alias is kept even though it is unused in declaration files

[dragomirtitian]

🔎 Search Terms

binding element alias declaration files

🕗 Version & Regression Information

• This changed between versions 4.7 and 4.8

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.2.2#code/PTAEhRyUFgCgVAygCwPYFcA2ATUA7FALqAEYCmoA1qQA5ECGuOAlgM6gBOpAtigG6lZYAMzS4AxgSYpcoFqkxYAcoQDSpGqAAUAbzx0upAFyg6GJnTYBfY7tz6jsguya4A5qEsBKUNtih-0DCWsLDwgDLkoWAAgmYWoKygaCwC8TIESOQi4pLSJGhE+EQuoOnkLEyudgRonJGI8th4hCTkVLSpoFikYhh07HQ5uGxxTEXDMkxc1BjcpLgEA1IyXQtMGHUsAJ5cxCgYAHSsAEqkQqSc4ikJSSkA7khMYkigT90UbJxnF2Kk45v351I+2EogkS1kDSUqnU1AAQvkAJIsNTtHR6AzGUzmKw2dEOFhOFzuLy+GABDikarsGRYiywYIwOrgOoAVWSzBknCpaU21HImjE6EafNA1D69gI5zYBF5vxMbFupAwGG8LBQ8SIciFODIlBoBBB2XBWrQWAwag0aLsGJMsRxPjxxgJzjcHk8xhlfJQQlt2J8fgCXJqNLt9KAA

💻 Code

// ✔ 
// Should not be kept and is removed
function shouldNotKeep ({ name: alias }: { name: string }) {
    
}

// ❌
// Alias is used in the function but not in the signature
// Should not be kept in declarations as it is an implementation detail
// symbol.isReferenced is used which checks references anywhere.
function shouldNotKeepButIsKept ({ name: alias }: { name: string }){
    return alias
}

// ✔
// Used in return type (could pe parameters types as well) so it should be kept
function shoudlKeep ({ name: alias }: { name: string }): typeof alias {
    return alias
}

🙁 Actual behavior

alias is kept in declaration files for shouldNotKeepButIsKept

🙂 Expected behavior

Either alias is removed from declaration file for shouldNotKeepButIsKept, or alias is also kept for shouldNotKeep.

Additional information about the issue

Seems like declaration emit is using isReferenced of the symbol. But this tracks usage of the symbol anywhere in the scope, we would need usage of the symbol in the function signature.

[jakebailey]

I'd need to bisect but I'm willing to bet this is on purpose for some reason; this feels vaguely like a bug I fixed a long time ago or at least reviewed...

[jakebailey]

Bisects to #41044.

[andrewbranch]

I would have thought this was more a consequence of #50779

[jakebailey]

Me too! (I couldn't remember which PR that was)

[andrewbranch]

Is this a problem? Can’t isolatedDeclarations always keep them?

[jakebailey]

Probably; I don't think there's much harm besides including an implementation detail, which is the main reason we were trying to drop them out of d.ts files. The main reason to keep them is typeof, which isolatedDeclarations won't be emitting, so it's sorta funky to not remove them.

In #50779 I seemed to think I was going to send a follow-up to improve things but clearly I never did, but the fact that this was broken by #41044 and not #50779 is very interesting.

(Also makes me think about #49627...)

[jakebailey]

I've sent a "fix" at #55678 which just continues down the the path of reverting #41044; realistically in #50779 I should have fully undone the renaming removals but I don't think I noticed the additional code path doing the same thing in declaration emit. I bet there's some test case that would break in a similar manner to what was fixed in #50779 because of this problem.

I don't know if this is ideal; obviously it'd be better to not emit renamings at all if we don't need them, but I'm not totally sure if we have the machinery in place to query "will I need this name somewhere else eventually in the emit".

[jakebailey]

Of course, an alternative is to just "do nothing" and accept that this is a limitation of the declaration emit, and that adding it when it's unused is harmless.

But as I said in #55678, if you're destructuring a parameter in your .ts files, you're probably using the parameter, so you're probably going to have it marked as used anyway, meaning you're going to get it no matter what we do without going all-in on a new reference analysis.

[dragomirtitian]

The main reason to keep them is typeof, which isolatedDeclarations won't be emitting,

@jakebailey I mean it could emit typeof if it's specifically written in source. Not sure if the type printer would ever use typeof with such a symbol.

[jakebailey]

Er, I thought we effectively found that typeof just can't be emitted with isolatedDeclarations?

[dragomirtitian]

@jakebailey If it written by the user, it will be copied like any other type. Isolated declaration will never forbit an explicit annotation. It just won't try to synthesize it. Performing the usage analysis to determine what needs to be kept to make the typeof valid is doable in isolated declarations and needs to be done anyway for other things, like imports.

I was looking at the code, and I think most of the necessary machinery to perform correct analysis is already there. I opened #55683 as a suggested fix.

[jakebailey]

needs to be done anyway for other things, like imports.

Yeah, this is what I suspected.

I'm not convinced typeof is generally safe, though; e.g. in Playground Link the TS code on the left says you can't assign anything but 0 to the prop, but the declaration file would let you assign any number. Without type checking you can't figure that out.

[dragomirtitian]

This is true, but the declarations emitted by TypeScript at present suffer from the same issue. The currently TSC generated declarations are:

export declare const x: number;
export declare const y: {
    x: typeof x;
};

Which don't account for the narrowing of x, just like a DTE would not.

If TSC decides to used the narrowed type at a later date, we can make the code above an isolated declaration error.

There will continue to be cases when either the code contains semantic errors or the code contains isolated declaration errors that are not detectable in a DTE without type info (few hopefully) that would mean a DTE successfully generates declarations that TSC would refuse to generate. This would be such a case.