Skip to content

Allow functions to be printed structurally in declaration emit even when they have symbols#21203

Merged
weswigham merged 2 commits into
microsoft:masterfrom
weswigham:allow-structural-function-output
Jan 16, 2018
Merged

Allow functions to be printed structurally in declaration emit even when they have symbols#21203
weswigham merged 2 commits into
microsoft:masterfrom
weswigham:allow-structural-function-output

Conversation

@weswigham
Copy link
Copy Markdown
Member

@weswigham weswigham commented Jan 16, 2018

#18860 introduced a break in a few of our RWC tests (mobx, one more) when it swapped to aggressively using typeof for the reproduction of function types (in order to be more like the type baseline output), but never introduced a fallback to the structural version of the type if the type wasn't accessible for declaration emit. This adds that fallback (and a flag to control it).

@weswigham weswigham requested review from a user, mhegazy and rbuckton January 16, 2018 19:18
Comment thread src/compiler/checker.ts Outdated
if (symbol.flags & SymbolFlags.Class && !getBaseTypeVariableOfClass(symbol) && !(symbol.valueDeclaration.kind === SyntaxKind.ClassExpression && context.flags & NodeBuilderFlags.WriteClassExpressionAsTypeLiteral) ||
symbol.flags & (SymbolFlags.Enum | SymbolFlags.ValueModule) ||
shouldWriteTypeOfFunctionSymbol()) {
shouldWriteTypeOfFunctionSymbol() && !(context.flags & NodeBuilderFlags.UseStructuralFallback && isSymbolAccessible(symbol, context.enclosingDeclaration, SymbolFlags.Value, /*computeAliases*/ false).accessibility !== SymbolAccessibility.Accessible)) {
Copy link
Copy Markdown

@ghost ghost Jan 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A helper similar to isTypeSymbolAccessible would be nice to avoid writing .accessibility !== SymbolAccessibility.Accessible.
Also, why not just put the && part inside of shouldWriteTypeOfFunctionSymbol?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll move it into shouldWriteTypeOfFunctionSymbol.

else {
errorNameNode = declaration.name;
const format = TypeFormatFlags.UseTypeOfFunction | TypeFormatFlags.WriteDefaultSymbolWithoutName |
const format = TypeFormatFlags.UseTypeOfFunction | TypeFormatFlags.UseStructuralFallback | TypeFormatFlags.WriteDefaultSymbolWithoutName |
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we're using TypeFormatFlags.UseStructuralFallback in all the same places we already use TypeFormatFlags.UseTypeOfFunction. Can they be combined?

Copy link
Copy Markdown
Member Author

@weswigham weswigham Jan 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They could be, but then external consumers lose the choice of allowing the structural fallback or not. I'm erring on the side of being more configurable here, since they are seperate behaviors. This way allows something like a quickfix to try to build a node using typeof with no fallback, see if there's an error, then decide not to offer the fix, for example.

@weswigham weswigham merged commit 154c614 into microsoft:master Jan 16, 2018
@weswigham weswigham deleted the allow-structural-function-output branch January 16, 2018 20:37
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants