Local types #3266
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -338,7 +338,13 @@ module ts { | |
| // Locals of a source file are not in scope (because they get merged into the global symbol table) | ||
| if (location.locals && !isGlobalSourceFile(location)) { | ||
| if (result = getSymbol(location.locals, name, meaning)) { | ||
| // Type parameters of a function are in scope in the entire function declaration, including the parameter | ||
| // list and return type. However, local types are only in scope in the function body. | ||
| if (!(meaning & SymbolFlags.Type) || !(result.flags & (SymbolFlags.Type & ~SymbolFlags.TypeParameter)) || | ||
| !isFunctionLike(location) || lastLocation === (<FunctionLikeDeclaration>location).body) { | ||
|
There was a problem hiding this comment. I'd rather this be split into more Alternatively, break each subexpression onto its own line and explain each of them independently. For instance: // We've found something in the 'locals' table, but we are sensitive to location in whether it is valid to use
// this entity.
// - A non-type entity (i.e. values, namespaces, and aliases) is unconditionally resolved.
// - A type parameters are visible throughout the entirety of a declaration, and are resolved.
// - A local type found in any declaration *other than a function* can be considered resolved.
// - A local types found in a function is only resolved if it was used *within the body of the function*.
if (!(meaning & SymbolFlags.Type) ||
!(result.flags & (SymbolFlags.Type & ~SymbolFlags.TypeParameter)) ||
!isFunctionLike(location) ||
lastLocation === (<FunctionLikeDeclaration>location).body) {There was a problem hiding this comment. In general I think it is perfectly fine (and often preferable) to have multi-line boolean expressions instead of multiple I do agree that in this case putting each condition on a separate line might make sense. There was a problem hiding this comment. I disagree. I think that multiple if statements are actually simpler. They allow you to examine each clause, one at a time. They let each clause be simple explained. They do not require you to think through multiple different forms of boolean expressions in each clause. Some minds (mine included) tend to have the problem where we see one expression, and then 'stamp' what we saw there on the next in a complicated clause. So if one expression does At this point, we've been using this code for around a year at this point, and there are still comments/complaints on the PRs about the clarity of the code. At this point, i think we should accept that these constructs are difficult for many team members to read, and we should err on the side of making the code as maintainable as possible for the team as a whole. There was a problem hiding this comment. We'll just have to disagree here. If the team has problems reading multi-line boolean expressions then I think we have a bigger problem. You're advocating turning simple and functional boolean expressions into multiple imperative if statements with temporary variables. I think that is exactly the wrong way to go. Whenever I see those I always wonder if I'm missing something subtle (which imperative code is full of), because otherwise it would just be written as a simple boolean expression. |
||
| break loop; | ||
| } | ||
| result = undefined; | ||
| } | ||
| } | ||
| switch (location.kind) { | ||
|
|
@@ -2465,30 +2471,62 @@ module ts { | |
| } | ||
| } | ||
|
|
||
| function appendTypeParameters(typeParameters: TypeParameter[], declarations: TypeParameterDeclaration[]): TypeParameter[] { | ||
| for (let declaration of declarations) { | ||
| let tp = getDeclaredTypeOfTypeParameter(getSymbolOfNode(declaration)); | ||
| if (!typeParameters) { | ||
|
There was a problem hiding this comment. You could just move this check outside the loop. if (!typeParameters && declarations.length > 0) {
typeParameters = [];
}There was a problem hiding this comment. What's there is more efficient because it creates the single element array in one go vs. first creating an empty array and then pushing. There was a problem hiding this comment. Based on what this function does, it looks like it should really be written as: function appendTypeParameters(declarations: TypeParameterDeclaration[], typeParameters?: TypeParameters[]) {
...The declarations are the actual input. The 'typeParameters' array is the output. We don't generally put output parameters before intput parameters. Also, as the second parameter is optional (based on the There was a problem hiding this comment. No, There was a problem hiding this comment. @ahejlsberg It would be best to document that for consumers so that they can quickly eye the function and know this. |
||
| typeParameters = [tp]; | ||
| } | ||
| else if (!contains(typeParameters, tp)) { | ||
| typeParameters.push(tp); | ||
| } | ||
| } | ||
| return typeParameters; | ||
| } | ||
|
|
||
| function appendOuterTypeParameters(typeParameters: TypeParameter[], node: Node): TypeParameter[]{ | ||
| while (true) { | ||
| node = node.parent; | ||
| if (!node) { | ||
| return typeParameters; | ||
| } | ||
| if (node.kind === SyntaxKind.ClassDeclaration || node.kind === SyntaxKind.FunctionDeclaration || | ||
| node.kind === SyntaxKind.FunctionExpression || node.kind === SyntaxKind.MethodDeclaration || | ||
| node.kind === SyntaxKind.ArrowFunction) { | ||
|
There was a problem hiding this comment. can you use two newlines after { after a wrapped conditional. Right now what happens is that hte next statement is indented exactly as far as the conditional, making it look like it continues the conditional. Providing the extra newline makes it clear that this is a statement inside the body of the code, and is not part of the if-expression. |
||
| let declarations = (<ClassDeclaration | FunctionLikeDeclaration>node).typeParameters; | ||
| if (declarations) { | ||
| return appendTypeParameters(appendOuterTypeParameters(typeParameters, node), declarations); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // The outer type parameters are those defined by enclosing generic classes, methods, or functions. | ||
| function getOuterTypeParametersOfClassOrInterface(symbol: Symbol): TypeParameter[] { | ||
| var kind = symbol.flags & SymbolFlags.Class ? SyntaxKind.ClassDeclaration : SyntaxKind.InterfaceDeclaration; | ||
|
There was a problem hiding this comment. Probably better to check SymbolFlags.Interface as well There was a problem hiding this comment. No need to: You can't declare classes or interfaces inside interfaces. There was a problem hiding this comment. I meant checking There was a problem hiding this comment. Well, the method is only ever called for a class or interface, so if it isn't a class it must be an interface. There was a problem hiding this comment. we'll probably need to come back to this when we do class expressions |
||
| return appendOuterTypeParameters(undefined, getDeclarationOfKind(symbol, kind)); | ||
|
There was a problem hiding this comment. named parameter for the "undefined". |
||
| } | ||
|
|
||
| // The local type parameters are the combined set of type parameters from all declarations of the class or interface. | ||
|
There was a problem hiding this comment. "all declarations of the class" - I'm a little confused, when can we get multiple declarations of a class? There was a problem hiding this comment. Unless you mean all class and interface declarations of a symbol. There was a problem hiding this comment. We currently only permit multiple declarations of interfaces, but we'll likely allow it for ambient classes as part of the work of changing the core type declarations in lib.d.ts to use class declarations. |
||
| function getLocalTypeParametersOfClassOrInterface(symbol: Symbol): TypeParameter[] { | ||
| let result: TypeParameter[]; | ||
| for (let node of symbol.declarations) { | ||
| if (node.kind === SyntaxKind.InterfaceDeclaration || node.kind === SyntaxKind.ClassDeclaration) { | ||
| let declaration = <InterfaceDeclaration>node; | ||
| if (declaration.typeParameters) { | ||
| result = appendTypeParameters(result, declaration.typeParameters); | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| // The full set of type parameters for a generic class or interface type consists of its outer type parameters plus | ||
| // its locally declared type parameters. | ||
| function getTypeParametersOfClassOrInterface(symbol: Symbol): TypeParameter[] { | ||
| return concatenate(getOuterTypeParametersOfClassOrInterface(symbol), getLocalTypeParametersOfClassOrInterface(symbol)); | ||
| } | ||
|
|
||
| function getBaseTypes(type: InterfaceType): ObjectType[] { | ||
| let typeWithBaseTypes = <InterfaceTypeWithBaseTypes>type; | ||
| if (!typeWithBaseTypes.baseTypes) { | ||
|
|
@@ -2558,10 +2596,13 @@ module ts { | |
| if (!links.declaredType) { | ||
| let kind = symbol.flags & SymbolFlags.Class ? TypeFlags.Class : TypeFlags.Interface; | ||
| let type = links.declaredType = <InterfaceType>createObjectType(kind, symbol); | ||
| let outerTypeParameters = getOuterTypeParametersOfClassOrInterface(symbol); | ||
| let localTypeParameters = getLocalTypeParametersOfClassOrInterface(symbol); | ||
| if (outerTypeParameters || localTypeParameters) { | ||
| type.flags |= TypeFlags.Reference; | ||
| type.typeParameters = concatenate(outerTypeParameters, localTypeParameters); | ||
| type.outerTypeParameters = outerTypeParameters; | ||
| type.localTypeParameters = localTypeParameters; | ||
| (<GenericType>type).instantiations = {}; | ||
| (<GenericType>type).instantiations[getTypeListId(type.typeParameters)] = <GenericType>type; | ||
| (<GenericType>type).target = <GenericType>type; | ||
|
|
@@ -2751,12 +2792,12 @@ module ts { | |
| return map(baseSignatures, baseSignature => { | ||
| let signature = baseType.flags & TypeFlags.Reference ? | ||
| getSignatureInstantiation(baseSignature, (<TypeReference>baseType).typeArguments) : cloneSignature(baseSignature); | ||
| signature.typeParameters = classType.localTypeParameters; | ||
| signature.resolvedReturnType = classType; | ||
| return signature; | ||
| }); | ||
| } | ||
| return [createSignature(undefined, classType.localTypeParameters, emptyArray, classType, 0, false, false)]; | ||
| } | ||
|
|
||
| function createTupleTypeMemberSymbols(memberTypes: Type[]): SymbolTable { | ||
|
|
@@ -3105,7 +3146,7 @@ module ts { | |
| let links = getNodeLinks(declaration); | ||
| if (!links.resolvedSignature) { | ||
| let classType = declaration.kind === SyntaxKind.Constructor ? getDeclaredTypeOfClassOrInterface((<ClassDeclaration>declaration.parent).symbol) : undefined; | ||
| let typeParameters = classType ? classType.localTypeParameters : | ||
| declaration.typeParameters ? getTypeParametersFromDeclaration(declaration.typeParameters) : undefined; | ||
| let parameters: Symbol[] = []; | ||
| let hasStringLiterals = false; | ||
|
|
@@ -3422,12 +3463,18 @@ module ts { | |
| else { | ||
| type = getDeclaredTypeOfSymbol(symbol); | ||
| if (type.flags & (TypeFlags.Class | TypeFlags.Interface) && type.flags & TypeFlags.Reference) { | ||
| // In a type reference, the outer type parameters of the referenced class or interface are automatically | ||
| // supplied as type arguments and the type reference only specifies arguments for the local type parameters | ||
|
There was a problem hiding this comment. Why do the outer type parameters have to be supplied as type arguments to the TypeReference? Why can't we just create a type reference that doesn't have any mappings for the outer type parameters? All the mappers for the outer type parameters will just end up mapping things to themselves anyway. There was a problem hiding this comment. That is true only for written type references. Type references created by instantiations will indeed end up having the type parameters mapped to different types. For example: function f<T>() {
class C {
x: T;
}
var c: C; // Really a C<T>
c = new C();
return c;
}
var c = f<string>(); // Type of c is C<string>
var s = c.x; // stringThere was a problem hiding this comment. Yes, but why is it necessary to map them at all for written type references? Why can't we just have them unmapped, and then later in an instantiation, they will get mapped? There was a problem hiding this comment. We would then need There was a problem hiding this comment. I thought the appending of the outer type parameters to the type parameters of the class happens in getDeclaredTypeOfClassOrInterface. I was asking why type arguments need to be supplied for those outer type parameters. Why can't the type reference have these outer type parameters as type parameters, but with no type arguments for them. I thought that if an instantiation is missing type arguments for its type parameters, the mapper will just return the original type parameter. I think what I am asking is: Is not passing the outer type parameters as type arguments a suitable optimization? There was a problem hiding this comment. Here's an example. Enjoy! The infinite expansion is stopped by our isDeeplyNestedGeneric check for type references, but I don't know of a mechanism that could stop what instantiateType would do. function f<T>() {
class C {
x = f<C>();
}
return new C();
}
function g<T>() {
class D {
x = g<D>();
}
return new D();
}
var a = f<string>();
var b = g<string>();
a = b;
b = a;There was a problem hiding this comment. Oh how interesting! I see how the cyclic instantiation happens. Btw, I came up with another example of this, based on a combination of the example you just gave, and the one from #3237. It looks like this: function foo<T>() {
var z = foo();
var y: {
y2: typeof z
};
return y;
}
function bar<T>() {
var z = bar();
var y: {
y2: typeof z;
}
return y;
}
var a = foo<number>();
var b = bar<number>();
a = b;Seems to stack overflow for me. Does your change fix this? What's interesting here is that T is never referenced in foo or bar. Its mere presence causes us to instantiate infinitely. Although I admit I don't really understand what is going on here. Perhaps this is more related to #3237, because we don't reuse the mapper between signature instantiations. There was a problem hiding this comment. No, my change doesn't affect this. Looks like the issue is runaway recursion in There was a problem hiding this comment. Hmm. Even if we put in place a cache for already instantiated signatures, I think you can use this as the same kind of infinitely expanding generics device as type references. For example, There was a problem hiding this comment. I've opened up #3309 for this |
||
| // of the class or interface. | ||
| let localTypeParameters = (<InterfaceType>type).localTypeParameters; | ||
| let expectedTypeArgCount = localTypeParameters ? localTypeParameters.length : 0; | ||
| let typeArgCount = node.typeArguments ? node.typeArguments.length : 0; | ||
| if (typeArgCount === expectedTypeArgCount) { | ||
| type = createTypeReference(<GenericType>type, concatenate((<InterfaceType>type).outerTypeParameters, | ||
|
There was a problem hiding this comment. By the way, I think there is an optimization you can do here. If typeArgCount and expectedTypeArgCount are zero, then |
||
| map(node.typeArguments, getTypeFromTypeNode))); | ||
| } | ||
| else { | ||
| error(node, Diagnostics.Generic_type_0_requires_1_type_argument_s, typeToString(type, /*enclosingDeclaration*/ undefined, TypeFormatFlags.WriteArrayAsGenericType), expectedTypeArgCount); | ||
| type = undefined; | ||
| } | ||
| } | ||
|
|
@@ -3884,7 +3931,7 @@ module ts { | |
| return mapper(<TypeParameter>type); | ||
| } | ||
| if (type.flags & TypeFlags.Anonymous) { | ||
| return type.symbol && type.symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.Class | SymbolFlags.TypeLiteral | SymbolFlags.ObjectLiteral) ? | ||
|
There was a problem hiding this comment. Is this for anonymous class expressions? There was a problem hiding this comment. No, it's for the anonymous type created for the static side of a class. There was a problem hiding this comment. Ah yes, because previously the static side of a class could never get instantiated. |
||
| instantiateAnonymousType(<ObjectType>type, mapper) : type; | ||
| } | ||
| if (type.flags & TypeFlags.Reference) { | ||
|
|
@@ -8968,7 +9015,6 @@ module ts { | |
| function checkFunctionDeclaration(node: FunctionDeclaration): void { | ||
| if (produceDiagnostics) { | ||
| checkFunctionLikeDeclaration(node) || | ||
| checkGrammarFunctionName(node.name) || | ||
| checkGrammarForGenerator(node); | ||
|
|
||
|
|
@@ -9330,7 +9376,7 @@ module ts { | |
|
|
||
| function checkVariableStatement(node: VariableStatement) { | ||
| // Grammar checking | ||
| checkGrammarDecorators(node) || checkGrammarModifiers(node) || checkGrammarVariableDeclarationList(node.declarationList) || checkGrammarForDisallowedLetOrConstStatement(node); | ||
|
|
||
| forEach(node.declarationList.declarations, checkSourceElement); | ||
| } | ||
|
|
@@ -9992,10 +10038,6 @@ module ts { | |
| function checkClassDeclaration(node: ClassDeclaration) { | ||
| checkGrammarDeclarationNameInStrictMode(node); | ||
| // Grammar checking | ||
| if (!node.name && !(node.flags & NodeFlags.Default)) { | ||
| grammarErrorOnFirstToken(node, Diagnostics.A_class_declaration_without_the_default_modifier_must_have_a_name); | ||
| } | ||
|
|
@@ -12247,19 +12289,28 @@ module ts { | |
| case SyntaxKind.MethodDeclaration: | ||
| case SyntaxKind.MethodSignature: | ||
| case SyntaxKind.IndexSignature: | ||
| case SyntaxKind.ModuleDeclaration: | ||
| case SyntaxKind.ImportDeclaration: | ||
| case SyntaxKind.ImportEqualsDeclaration: | ||
| case SyntaxKind.ExportDeclaration: | ||
| case SyntaxKind.ExportAssignment: | ||
| case SyntaxKind.Parameter: | ||
| break; | ||
| case SyntaxKind.ClassDeclaration: | ||
| case SyntaxKind.InterfaceDeclaration: | ||
| case SyntaxKind.VariableStatement: | ||
| case SyntaxKind.FunctionDeclaration: | ||
| case SyntaxKind.TypeAliasDeclaration: | ||
| if (node.modifiers && node.parent.kind !== SyntaxKind.ModuleBlock && node.parent.kind !== SyntaxKind.SourceFile) { | ||
| return grammarErrorOnFirstToken(node, Diagnostics.Modifiers_cannot_appear_here); | ||
| } | ||
| break; | ||
| case SyntaxKind.EnumDeclaration: | ||
| if (node.modifiers && (node.modifiers.length > 1 || node.modifiers[0].kind !== SyntaxKind.ConstKeyword) && | ||
|
There was a problem hiding this comment. Break into multiple checks. The mix of |
||
| node.parent.kind !== SyntaxKind.ModuleBlock && node.parent.kind !== SyntaxKind.SourceFile) { | ||
|
There was a problem hiding this comment. Consider: extract common helper for checking that the parent is a module or source file (repeated above as well, as well as in "shouldWriteTypeOfFunctionSymbol", "checkGrammarModifiers", "checkGrammarStatementInAmbientContext", ). |
||
| return grammarErrorOnFirstToken(node, Diagnostics.Modifiers_cannot_appear_here); | ||
| } | ||
| break; | ||
| default: | ||
| return false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2168,9 +2168,5 @@ | |
| "'class' expressions are not currently supported.": { | ||
| "category": "Error", | ||
| "code": 9003 | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess classes are already block scoped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.