Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor declaration emitter into declaration transformer #21930

Merged
merged 21 commits into from
Mar 16, 2018
Merged

Refactor declaration emitter into declaration transformer #21930

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
62ee91d
Refactor declaration emitter into declaration transformer
weswigham Jan 18, 2018
9f42eb8
Merge branch 'master' into declaration-emit-refactor
weswigham Feb 15, 2018
4c639b8
Merge branch 'master' into declaration-emit-refactor
weswigham Feb 21, 2018
3adcbc7
Slight cleanup from code review feedback
weswigham Feb 21, 2018
9e34a3f
Merge branch 'master' into declaration-emit-refactor
weswigham Feb 26, 2018
db8e239
Merge branch 'master' into declaration-emit-refactor
weswigham Feb 27, 2018
7844547
Merge branch 'master' into declaration-emit-refactor
weswigham Mar 1, 2018
d1359ee
Merge branch 'master' into declaration-emit-refactor
weswigham Mar 3, 2018
165e6bb
Incorporate fix for new test
weswigham Mar 3, 2018
b84cbe4
Merge branch 'master' into declaration-emit-refactor
weswigham Mar 8, 2018
63923e2
Merge branch 'declaration-emit-refactor' of github.com:weswigham/Type…
weswigham Mar 8, 2018
e8a64bd
Merge branch 'master' into declaration-emit-refactor
weswigham Mar 9, 2018
d67ddc1
Merge branch 'master' into declaration-emit-refactor
weswigham Mar 15, 2018
a2f1a8a
Swaths of PR feedback
weswigham Mar 15, 2018
036c65a
Merge public methods
weswigham Mar 15, 2018
289e3e5
Per-file output
weswigham Mar 15, 2018
08ffcc8
Preserve input import ordering more often
weswigham Mar 15, 2018
cb69963
Unify jsdoc comment start detection under more lenient rule
weswigham Mar 15, 2018
1a699b8
Merge branch 'per-file-output' into declaration-emit-refactor
weswigham Mar 15, 2018
f83bc81
Move to per-file transformations to reduce the memory that msut be re…
weswigham Mar 15, 2018
6d729cf
Fix typo
weswigham Mar 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 23 additions & 19 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3614,10 +3614,6 @@ namespace ts {
isExternalModuleAugmentation(node.parent.parent);
}

function literalTypeToString(type: LiteralType) {
return type.flags & TypeFlags.StringLiteral ? '"' + escapeString((<StringLiteralType>type).value) + '"' : "" + (<NumberLiteralType>type).value;
}

interface NodeBuilderContext {
enclosingDeclaration: Node | undefined;
flags: NodeBuilderFlags | undefined;
Expand Down Expand Up @@ -3678,7 +3674,7 @@ namespace ts {
return symbolName(symbol);
}

function isDeclarationVisible(node: Declaration): boolean {
function isDeclarationVisible(node: Declaration | AnyImportSyntax): boolean {
if (node) {
const links = getNodeLinks(node);
if (links.isVisible === undefined) {
Expand Down Expand Up @@ -25258,6 +25254,7 @@ namespace ts {

function isImplementationOfOverload(node: FunctionLike) {
if (nodeIsPresent((node as FunctionLikeDeclaration).body)) {
if (isGetAccessor(node) || isSetAccessor(node)) return false; // Get or set accessors can never be overload implementations, but can have up to 2 signatures
const symbol = getSymbolOfNode(node);
const signaturesOfSymbol = getSignaturesOfSymbol(symbol);
// If this function body corresponds to function with multiple signature, it is implementation of overload
Expand Down Expand Up @@ -25397,30 +25394,33 @@ namespace ts {
}
}

function writeTypeOfDeclaration(declaration: AccessorDeclaration | VariableLikeDeclaration, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: EmitTextWriter) {
function createTypeOfDeclaration(declaration: AccessorDeclaration | VariableLikeDeclaration, enclosingDeclaration: Node, flags: NodeBuilderFlags, tracker: SymbolTracker, addUndefined?: boolean) {
declaration = getParseTreeNode(declaration) as typeof declaration;
// Get type of the symbol if this is the valid symbol otherwise get type at location
const symbol = getSymbolOfNode(declaration);
let type = symbol && !(symbol.flags & (SymbolFlags.TypeLiteral | SymbolFlags.Signature))
? getWidenedLiteralType(getTypeOfSymbol(symbol))
: unknownType;
if (type.flags & TypeFlags.UniqueESSymbol &&
type.symbol === symbol) {
flags |= TypeFormatFlags.AllowUniqueESSymbolType;
flags |= NodeBuilderFlags.AllowUniqueESSymbolType;
}
if (flags & TypeFormatFlags.AddUndefined) {
if (addUndefined) {
type = getOptionalType(type);
}
typeToString(type, enclosingDeclaration, flags | TypeFormatFlags.MultilineObjectLiterals, writer);
return nodeBuilder.typeToTypeNode(type, enclosingDeclaration, flags | NodeBuilderFlags.MultilineObjectLiterals, tracker);
}

function writeReturnTypeOfSignatureDeclaration(signatureDeclaration: SignatureDeclaration, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: EmitTextWriter) {
function createReturnTypeOfSignatureDeclaration(signatureDeclaration: SignatureDeclaration, enclosingDeclaration: Node, flags: NodeBuilderFlags, tracker: SymbolTracker) {
signatureDeclaration = getParseTreeNode(signatureDeclaration) as typeof signatureDeclaration;
const signature = getSignatureFromDeclaration(signatureDeclaration);
typeToString(getReturnTypeOfSignature(signature), enclosingDeclaration, flags | TypeFormatFlags.MultilineObjectLiterals, writer);
return nodeBuilder.typeToTypeNode(getReturnTypeOfSignature(signature), enclosingDeclaration, flags | NodeBuilderFlags.MultilineObjectLiterals, tracker);
}

function writeTypeOfExpression(expr: Expression, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: EmitTextWriter) {
function createTypeOfExpression(expr: Expression, enclosingDeclaration: Node, flags: NodeBuilderFlags, tracker: SymbolTracker) {
expr = getParseTreeNode(expr) as typeof expr;
const type = getWidenedType(getRegularTypeOfExpression(expr));
typeToString(type, enclosingDeclaration, flags | TypeFormatFlags.MultilineObjectLiterals, writer);
return nodeBuilder.typeToTypeNode(type, enclosingDeclaration, flags | NodeBuilderFlags.MultilineObjectLiterals, tracker);
}

function hasGlobalName(name: string): boolean {
Expand Down Expand Up @@ -25468,9 +25468,13 @@ namespace ts {
return false;
}

function writeLiteralConstValue(node: VariableDeclaration | PropertyDeclaration | PropertySignature | ParameterDeclaration, writer: EmitTextWriter) {
function literalTypeToNode(type: LiteralType): Expression {
return createLiteral(type.value);
}

function createLiteralConstValue(node: VariableDeclaration | PropertyDeclaration | PropertySignature | ParameterDeclaration) {
const type = getTypeOfSymbol(getSymbolOfNode(node));
writer.writeStringLiteral(literalTypeToString(<LiteralType>type));
return literalTypeToNode(<LiteralType>type);
}

function createResolver(): EmitResolver {
Expand Down Expand Up @@ -25514,9 +25518,10 @@ namespace ts {
isImplementationOfOverload,
isRequiredInitializedParameter,
isOptionalUninitializedParameterProperty,
writeTypeOfDeclaration,
writeReturnTypeOfSignatureDeclaration,
writeTypeOfExpression,
createTypeOfDeclaration,
createReturnTypeOfSignatureDeclaration,
createTypeOfExpression,
createLiteralConstValue,
isSymbolAccessible,
isEntityNameVisible,
getConstantValue: node => {
Expand All @@ -25538,7 +25543,6 @@ namespace ts {
const symbol = node && getSymbolOfNode(node);
return !!(symbol && getCheckFlags(symbol) & CheckFlags.Late);
},
writeLiteralConstValue,
getJsxFactoryEntity: () => _jsxFactoryEntity
};

Expand Down
16 changes: 16 additions & 0 deletions src/compiler/comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,21 @@ namespace ts {
}
}

function isJSDocLikeText(text: string, start: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Deduplicate this with isJsDocStart

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't. They behave meaningfully differently. isJSDocLikeText matches anything that starts with ** and isn't an empty comment. isJsDocStart only matches comments starting with ** but not *** and doesn't filter empty comments at all.

Copy link
Member

Choose a reason for hiding this comment

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

Why does isJsDocStart check for /***? If we don't consider /*** to be JSDoc there, then we shouldn't allow it here either.

Assuming the last condition in isJsDocStart is not a typo (e.g. checking for * instead of /), it seems like both of these could be the same function:

function isJsDocStart(content: string, start: number) {
    return content.charCodeAt(start) === CharacterCodes.slash &&
        content.charCodeAt(start + 1) === CharacterCodes.asterisk &&
        content.charCodeAt(start + 2) === CharacterCodes.asterisk &&
        content.charCodeAt(start + 3) !== CharacterCodes.asterisk &&
        content.charCodeAt(start + 3) !== CharacterCodes.slash;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Why does isJsDocStart check for /***

Honestly no idea. It just seems as if we decided at some point that extra stars (like if, for example, you put line-length star guards around your comment) don't count as jsdoc as far as the parser is concerned. However I definitely think it's still correct to preserve such a comment in declaration emit, hence the difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've unified them both under the previous isJSDocLikeText implementation, since it seems like the parser definition has just sat around since its introduction three years ago, and the only justification for why /*** @type {Foo} */ is any less jsdoc than /** @type {Foo} */ I've found is a remark on usejsdoc.org; but we're typically more lenient than it says (plus, closure compiler allows it (you can see the type annotation still getting applied under the advanced mode error)) cc @DanielRosenwasser it makes sense to be more lenient here, right?

return text.charCodeAt(start + 1) === CharacterCodes.asterisk &&
text.charCodeAt(start + 2) === CharacterCodes.asterisk &&
text.charCodeAt(start + 3) !== CharacterCodes.slash;
}

function shouldWriteComment(text: string, pos: number) {
if (printerOptions.onlyPrintJsDocStyle) {
return (isJSDocLikeText(text, pos) || isPinnedComment(text, pos));
}
return true;
}

function emitLeadingComment(commentPos: number, commentEnd: number, kind: SyntaxKind, hasTrailingNewLine: boolean, rangePos: number) {
if (!shouldWriteComment(currentText, commentPos)) return;
if (!hasWrittenComment) {
emitNewLineBeforeLeadingCommentOfPosition(currentLineMap, writer, rangePos, commentPos);
hasWrittenComment = true;
Expand Down Expand Up @@ -292,6 +306,7 @@ namespace ts {
}

function emitTrailingComment(commentPos: number, commentEnd: number, _kind: SyntaxKind, hasTrailingNewLine: boolean) {
if (!shouldWriteComment(currentText, commentPos)) return;
// trailing comments are emitted at space/*trailing comment1 */space/*trailing comment2*/
if (!writer.isAtStartOfLine()) {
writer.write(" ");
Expand Down Expand Up @@ -404,6 +419,7 @@ namespace ts {
}

function writeComment(text: string, lineMap: number[], writer: EmitTextWriter, commentPos: number, commentEnd: number, newLine: string) {
if (!shouldWriteComment(currentText, commentPos)) return;
if (emitPos) emitPos(commentPos);
writeCommentRange(text, lineMap, writer, commentPos, commentEnd, newLine);
if (emitPos) emitPos(commentEnd);
Expand Down
Loading