dsherret / ts-morph

TypeScript Compiler API wrapper for static analysis and programmatic code changes.
https://ts-morph.com
MIT License
4.95k stars 195 forks source link

method.getSignature().getReturnType().getText() returns union type instead of type alias in 10.x #943

Open AlCalzone opened 3 years ago

AlCalzone commented 3 years ago

I am using ts-morph to auto-generate documentation for a project of mine and just upgraded from ts-morph from 9.1.0 to 10.0.1.

Describe the bug

Version: 10.0.1

Prior to the upgrade, this method had an inferred return type of

Promise<{ currentValue: Maybe<boolean>; targetValue: boolean | undefined; duration: Duration | undefined; } | undefined>

whereas after the update, the return type gets printed as

Promise<{ currentValue: boolean | ("unknown" & { __brand: boolean; }); targetValue: boolean | undefined; duration: Duration | undefined; } | undefined>

where currentValue gets printed with the union type behind Maybe<boolean>.

I have verified with git bisect that the culprit is indeed not the update to TypeScript 4.2, but the ts-morph update. I didn't find anything regarding this change in the changelog, so I'm assuming it is not intended.

To Reproduce

git clone https://github.com/zwave-js/node-zwave-js
git checkout 6ffcf0f0104cbfb1c1af793b2a8fc291eb171c9d
yarn install && yarn run docs:generate
# you should see no changes here
git checkout 3373caa84e16c78999101fefbc82f4dd4b49f774
yarn install && yarn run docs:generate
# docs/api/CCs/BinarySwitch.md changes as described above

Expected behavior

No changes to docs/api/CCs/BinarySwitch.md like before the update.

dsherret commented 3 years ago

@AlCalzone this functionality is all from the compiler API so this was because of TS 4.2, likely the "Smarter Type Alias Preservation" change: https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/#smarter-type-alias-preservation

It looks like in this case it's not doing a great job. How does it display in your editor when your editor is using TS 4.2?

AlCalzone commented 3 years ago

The editor is displaying a third variant, which looks like it resolves the original type to its union constituents but doesn't go further after that: grafik

Guess its not a bug in ts-morph then - feel free to close.

dsherret commented 3 years ago

That's more what I would expect (because of the || false). I wonder if this is just some type format flags thing that needs to be provided to an arg of getText(...) in order to get BrandedUnknown<boolean>.

dsherret commented 3 years ago

@AlCalzone Maybe try providing ts.TypeFormatFlags.UseAliasDefinedOutsideCurrentScope to the second argument of type.getText()

AlCalzone commented 3 years ago

I can reproduce on the TS playground that this regressed in 4.2.2: https://github.com/microsoft/TypeScript/issues/43031 4.1.5 correctly preserves the alias Maybe<boolean> which has boolean as a union constituent.

No dice with UseAliasDefinedOutsideCurrentScope sadly.

dsherret commented 3 years ago

@AlCalzone hmmm... must be a way considering the language service shows that.

I think though that boolean | BrandedUnknown<boolean> is correct because false in || false is the boolean type without a brand. I think it was a bug before.

AlCalzone commented 3 years ago

But Maybe<boolean> is boolean | ("unknown" & { __brand: boolean }), which includes boolean itself without a brand.

I could get to the type the language service prints by changing the scope for getText(), but that is still unnecessarily complicated.

dsherret commented 3 years ago

@AlCalzone oh, you are right! I was lazily/incorrectly only looking at the Brand type.