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

Property assignments in Typescript #26368

Merged
merged 9 commits into from
Aug 15, 2018
Merged

Property assignments in Typescript #26368

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bfcf324
Allow special property assignments in TS
sandersn Aug 8, 2018
3309104
Improve contextual typing predicate
sandersn Aug 9, 2018
b0fe52f
More fixes.
sandersn Aug 9, 2018
3060706
Cleanup
sandersn Aug 10, 2018
872fbbf
Remove extra whitespace
sandersn Aug 10, 2018
f090caf
Some kind of fix to isAnyDeclarationName
sandersn Aug 15, 2018
0b953b3
Add fourslash and improve isAnyDeclarationName
sandersn Aug 15, 2018
869dc69
Cleanup from PR comments
sandersn Aug 15, 2018
05c2ffc
Merge branch 'master' into property-assignments-in-ts
sandersn Aug 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
5 changes: 5 additions & 0 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2477,6 +2477,11 @@ namespace ts {

function bindSpecialPropertyAssignment(node: BinaryExpression) {
const lhs = node.left as PropertyAccessEntityNameExpression;
// Class declarations in Typescript do not allow property declarations
const parentSymbol = lookupSymbolForPropertyAccess(lhs.expression);
if (!isInJavaScriptFile(node) && !isTSFunctionSymbol(parentSymbol)) {
Copy link

Choose a reason for hiding this comment

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

Nit: In both places isTsFunctionSymbol is called, you just checked isInJavaScriptFile to be false. So maybe it could just be isFunctionSymbol.

return;
}
// Fix up parent pointers since we're going to use these nodes before we bind into them
node.left.parent = node;
node.right.parent = node;
Expand Down
96 changes: 56 additions & 40 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4788,48 +4788,45 @@ namespace ts {
}

/** If we don't have an explicit JSDoc type, get the type from the initializer. */
function getInitializerTypeFromSpecialDeclarations(symbol: Symbol, resolvedSymbol: Symbol | undefined, expression: Expression, special: SpecialPropertyAssignmentKind) {
if (isBinaryExpression(expression)) {
const type = resolvedSymbol ? getTypeOfSymbol(resolvedSymbol) : getWidenedLiteralType(checkExpressionCached(expression.right));
if (type.flags & TypeFlags.Object &&
special === SpecialPropertyAssignmentKind.ModuleExports &&
symbol.escapedName === InternalSymbolName.ExportEquals) {
const exportedType = resolveStructuredTypeMembers(type as ObjectType);
const members = createSymbolTable();
copyEntries(exportedType.members, members);
if (resolvedSymbol && !resolvedSymbol.exports) {
resolvedSymbol.exports = createSymbolTable();
}
(resolvedSymbol || symbol).exports!.forEach((s, name) => {
if (members.has(name)) {
const exportedMember = exportedType.members.get(name)!;
const union = createSymbol(s.flags | exportedMember.flags, name);
union.type = getUnionType([getTypeOfSymbol(s), getTypeOfSymbol(exportedMember)]);
members.set(name, union);
}
else {
members.set(name, s);
}
});
const result = createAnonymousType(
exportedType.symbol,
members,
exportedType.callSignatures,
exportedType.constructSignatures,
exportedType.stringIndexInfo,
exportedType.numberIndexInfo);
result.objectFlags |= (getObjectFlags(type) & ObjectFlags.JSLiteral); // Propagate JSLiteral flag
return result;
function getInitializerTypeFromSpecialDeclarations(symbol: Symbol, resolvedSymbol: Symbol | undefined, expression: BinaryExpression, special: SpecialPropertyAssignmentKind) {
Copy link
Member Author

Choose a reason for hiding this comment

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

getInitializerTypeFromSpecialDeclarations is only called once, so I just made the caller check for undefined. You should turn on Ignore Whitespace to read this review.

const type = resolvedSymbol ? getTypeOfSymbol(resolvedSymbol) : getWidenedLiteralType(checkExpressionCached(expression.right));
if (type.flags & TypeFlags.Object &&
special === SpecialPropertyAssignmentKind.ModuleExports &&
symbol.escapedName === InternalSymbolName.ExportEquals) {
const exportedType = resolveStructuredTypeMembers(type as ObjectType);
const members = createSymbolTable();
copyEntries(exportedType.members, members);
if (resolvedSymbol && !resolvedSymbol.exports) {
resolvedSymbol.exports = createSymbolTable();
}
if (isEmptyArrayLiteralType(type)) {
if (noImplicitAny) {
reportImplicitAnyError(expression, anyArrayType);
(resolvedSymbol || symbol).exports!.forEach((s, name) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you briefly convince me that symbol.exports is always defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

Inside the containing if, symbol is the commonjs equivalent of export =: module.exports = { ... }. So it's guaranteed to have exports -- they are the contents of the module.exports = initializer, and we restricted the initializer's type with type.flags & TypeFlags.Object.

if (members.has(name)) {
const exportedMember = exportedType.members.get(name)!;
const union = createSymbol(s.flags | exportedMember.flags, name);
union.type = getUnionType([getTypeOfSymbol(s), getTypeOfSymbol(exportedMember)]);
members.set(name, union);
}
return anyArrayType;
else {
members.set(name, s);
}
});
const result = createAnonymousType(
exportedType.symbol,
members,
exportedType.callSignatures,
exportedType.constructSignatures,
exportedType.stringIndexInfo,
exportedType.numberIndexInfo);
result.objectFlags |= (getObjectFlags(type) & ObjectFlags.JSLiteral); // Propagate JSLiteral flag
return result;
}
if (isEmptyArrayLiteralType(type)) {
if (noImplicitAny) {
reportImplicitAnyError(expression, anyArrayType);
}
return type;
return anyArrayType;
}
return neverType;
return type;
}

function isDeclarationInConstructor(expression: Expression) {
Expand Down Expand Up @@ -5049,7 +5046,9 @@ namespace ts {
if (symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.Class | SymbolFlags.Enum | SymbolFlags.ValueModule)) {
return getTypeOfFuncClassEnumModule(symbol);
}
type = tryGetTypeFromEffectiveTypeNode(declaration) || anyType;
type = isBinaryExpression(declaration.parent) ?
getWidenedTypeFromJSSpecialPropertyDeclarations(symbol) :
tryGetTypeFromEffectiveTypeNode(declaration) || anyType;
}
else if (isPropertyAssignment(declaration)) {
type = tryGetTypeFromEffectiveTypeNode(declaration) || checkPropertyAssignment(declaration);
Expand Down Expand Up @@ -16004,7 +16003,24 @@ namespace ts {
case SpecialPropertyAssignmentKind.PrototypeProperty:
// If `binaryExpression.left` was assigned a symbol, then this is a new declaration; otherwise it is an assignment to an existing declaration.
// See `bindStaticPropertyAssignment` in `binder.ts`.
return !binaryExpression.left.symbol || binaryExpression.left.symbol.valueDeclaration && !!getJSDocTypeTag(binaryExpression.left.symbol.valueDeclaration);
if (!binaryExpression.left.symbol) {
return true;
}
else {
const decl = binaryExpression.left.symbol.valueDeclaration;
if (!decl) {
return false;
}
if (isInJavaScriptFile(decl)) {
return !!getJSDocTypeTag(decl);
}
else if (isIdentifier((binaryExpression.left as PropertyAccessExpression).expression)) {
const id = (binaryExpression.left as PropertyAccessExpression).expression as Identifier;
const parentSymbol = resolveName(id, id.escapedText, SymbolFlags.Value, undefined, id.escapedText, /*isUse*/ true);
return !isTSFunctionSymbol(parentSymbol);
}
return true;
}
case SpecialPropertyAssignmentKind.ThisProperty:
case SpecialPropertyAssignmentKind.ModuleExports:
return !binaryExpression.symbol || binaryExpression.symbol.valueDeclaration && !!getJSDocTypeTag(binaryExpression.symbol.valueDeclaration);
Expand Down
27 changes: 23 additions & 4 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1719,12 +1719,15 @@ namespace ts {
}

export function getDeclarationOfJSInitializer(node: Node): Node | undefined {
if (!isInJavaScriptFile(node) || !node.parent) {
if (!node.parent) {
return undefined;
}
let name: Expression | BindingName | undefined;
let decl: Node | undefined;
if (isVariableDeclaration(node.parent) && node.parent.initializer === node) {
if (!isInJavaScriptFile(node) && !isVarConst(node.parent)) {
return undefined;
}
name = node.parent.name;
decl = node.parent;
}
Expand Down Expand Up @@ -1886,8 +1889,12 @@ namespace ts {
/// Given a BinaryExpression, returns SpecialPropertyAssignmentKind for the various kinds of property
/// assignments we treat as special in the binder
export function getSpecialPropertyAssignmentKind(expr: BinaryExpression): SpecialPropertyAssignmentKind {
if (!isInJavaScriptFile(expr) ||
expr.operatorToken.kind !== SyntaxKind.EqualsToken ||
const special = getSpecialPropertyAssignmentKindWorker(expr);
return special === SpecialPropertyAssignmentKind.Property || isInJavaScriptFile(expr) ? special : SpecialPropertyAssignmentKind.None;
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 didn't want to create another wrapper/worker pair, but I couldn't come up with a better way to special-case the isJS check just for property assignments. Any ideas?

}

function getSpecialPropertyAssignmentKindWorker(expr: BinaryExpression): SpecialPropertyAssignmentKind {
if (expr.operatorToken.kind !== SyntaxKind.EqualsToken ||
!isPropertyAccessExpression(expr.left)) {
return SpecialPropertyAssignmentKind.None;
}
Expand Down Expand Up @@ -1948,6 +1955,15 @@ namespace ts {
!!getJSDocTypeTag(expr.parent);
}

export function isTSFunctionSymbol(symbol: Symbol | undefined) {
if (!symbol || !symbol.valueDeclaration) {
return false;
}
const decl = symbol.valueDeclaration;
return !isInJavaScriptFile(decl) &&
(decl.kind === SyntaxKind.FunctionDeclaration || isVariableDeclaration(decl) && decl.initializer && isFunctionLike(decl.initializer));
}

export function importFromModuleSpecifier(node: StringLiteralLike): AnyValidImportOrReExport {
return tryGetImportFromModuleSpecifier(node) || Debug.fail(Debug.showSyntaxKind(node.parent));
}
Expand Down Expand Up @@ -2348,7 +2364,10 @@ namespace ts {
}
else {
const binExp = name.parent.parent;
return isBinaryExpression(binExp) && getSpecialPropertyAssignmentKind(binExp) !== SpecialPropertyAssignmentKind.None && getNameOfDeclaration(binExp) === name;
return isBinaryExpression(binExp) &&
getSpecialPropertyAssignmentKind(binExp) !== SpecialPropertyAssignmentKind.None &&
(isInJavaScriptFile(name) || !isPropertyAccessExpression(binExp.left) || binExp.left.expression.symbol) &&
Copy link

Choose a reason for hiding this comment

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

Tests seem to pass without this line -- what's this protecting against?

Copy link
Member Author

Choose a reason for hiding this comment

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

FindAllReferences relies on this function, and perhaps other services do. Without this line, a few fourslash tests fail, pointing out that getSpecialPropertyAssignmentKind now recognizes any property assignment in TS files, even those that aren’t actually expando assignments.

The left side of the or restores the old behavior for JS, and the right side adds to it for TS expando assignments. However, the right side isn’t tested. I need to add a case similar to the existing JS ones.

getNameOfDeclaration(binExp) === name;
}
}
default:
Expand Down
23 changes: 3 additions & 20 deletions tests/baselines/reference/fixSignatureCaching.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ tests/cases/conformance/fixSignatureCaching.ts(474,38): error TS2339: Property '
tests/cases/conformance/fixSignatureCaching.ts(481,22): error TS2339: Property 'findMatch' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(481,37): error TS2339: Property 'mobileDetectRules' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(489,18): error TS2339: Property 'isMobileFallback' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(490,39): error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
tests/cases/conformance/fixSignatureCaching.ts(492,37): error TS2339: Property 'FALLBACK_MOBILE' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(495,51): error TS2339: Property 'FALLBACK_PHONE' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(498,52): error TS2339: Property 'FALLBACK_TABLET' does not exist on type '{}'.
Expand All @@ -49,13 +48,8 @@ tests/cases/conformance/fixSignatureCaching.ts(872,25): error TS2339: Property '
tests/cases/conformance/fixSignatureCaching.ts(893,25): error TS2339: Property 'getVersionStr' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(915,36): error TS2339: Property 'findMatches' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(915,53): error TS2339: Property 'mobileDetectRules' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(944,33): error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
tests/cases/conformance/fixSignatureCaching.ts(955,42): error TS2339: Property 'mobileGrade' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(963,22): error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
tests/cases/conformance/fixSignatureCaching.ts(964,57): error TS2339: Property 'getDeviceSmallerSide' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(967,22): error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
tests/cases/conformance/fixSignatureCaching.ts(971,18): error TS2339: Property '_impl' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
tests/cases/conformance/fixSignatureCaching.ts(973,18): error TS2339: Property 'version' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
tests/cases/conformance/fixSignatureCaching.ts(978,16): error TS2304: Cannot find name 'module'.
tests/cases/conformance/fixSignatureCaching.ts(978,42): error TS2304: Cannot find name 'module'.
tests/cases/conformance/fixSignatureCaching.ts(979,37): error TS2304: Cannot find name 'module'.
Expand All @@ -65,7 +59,7 @@ tests/cases/conformance/fixSignatureCaching.ts(981,16): error TS2304: Cannot fin
tests/cases/conformance/fixSignatureCaching.ts(983,44): error TS2339: Property 'MobileDetect' does not exist on type 'Window'.


==== tests/cases/conformance/fixSignatureCaching.ts (65 errors) ====
==== tests/cases/conformance/fixSignatureCaching.ts (59 errors) ====
// Repro from #10697

(function (define, undefined) {
Expand Down Expand Up @@ -608,8 +602,6 @@ tests/cases/conformance/fixSignatureCaching.ts(983,44): error TS2339: Property '
~~~~~~~~~~~~~~~~
!!! error TS2339: Property 'isMobileFallback' does not exist on type '{}'.
phoneSized = MobileDetect.isPhoneSized(maxPhoneWidth);
~~~~~~~~~~~~
!!! error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
if (phoneSized === undefined) {
cache.mobile = impl.FALLBACK_MOBILE;
~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -1112,8 +1104,6 @@ tests/cases/conformance/fixSignatureCaching.ts(983,44): error TS2339: Property '
*/
isPhoneSized: function (maxPhoneWidth) {
return MobileDetect.isPhoneSized(maxPhoneWidth || this.maxPhoneWidth);
~~~~~~~~~~~~
!!! error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
},

/**
Expand All @@ -1135,26 +1125,18 @@ tests/cases/conformance/fixSignatureCaching.ts(983,44): error TS2339: Property '
// environment-dependent
if (typeof window !== 'undefined' && window.screen) {
MobileDetect.isPhoneSized = function (maxPhoneWidth) {
~~~~~~~~~~~~
!!! error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
return maxPhoneWidth < 0 ? undefined : impl.getDeviceSmallerSide() <= maxPhoneWidth;
~~~~~~~~~~~~~~~~~~~~
!!! error TS2339: Property 'getDeviceSmallerSide' does not exist on type '{}'.
};
} else {
MobileDetect.isPhoneSized = function () {};
~~~~~~~~~~~~
!!! error TS2339: Property 'isPhoneSized' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.
}

// should not be replaced by a completely new object - just overwrite existing methods
MobileDetect._impl = impl;
~~~~~
!!! error TS2339: Property '_impl' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.

MobileDetect.version = '1.3.3 2016-07-31';
~~~~~~~
!!! error TS2339: Property 'version' does not exist on type '(userAgent: any, maxPhoneWidth: any) => void'.

return MobileDetect;
}); // end of call of define()
Expand Down Expand Up @@ -1183,4 +1165,5 @@ tests/cases/conformance/fixSignatureCaching.ts(983,44): error TS2339: Property '
// please file a bug if you get this error!
throw new Error('unknown environment');
}
})());
})());

3 changes: 2 additions & 1 deletion tests/baselines/reference/fixSignatureCaching.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,8 @@ define(function () {
// please file a bug if you get this error!
throw new Error('unknown environment');
}
})());
})());


//// [fixSignatureCaching.js]
// Repro from #10697
Expand Down
Loading