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.

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

Local types #3266

Merged
merged 18 commits into from May 31, 2015

Conversation

Projects
None yet
7 participants
@ahejlsberg
Copy link
Member

commented May 26, 2015

This PR implements support for local class, interface, enum, and type alias declarations. For example:

function f() {
    enum E {
        A, B, C
    }
    class C {
        x: E;
    }
    interface I {
        x: E;
    }
    type A = I[];
    let a: A = [new C()];
    a[0].x = E.B;
}

Local types are block scoped, similar to variables declared with let and const. For example:

function f() {
    if (true) {
        interface T { x: number }
        let v: T;
        v.x = 5;
    }
    else {
        interface T { x: string }
        let v: T;
        v.x = "hello";
    }
}

The inferred return type of a function may be a type declared locally within the function. It is not possible for callers of the function to reference such a local type, but it can of course be matched structurally. For example:

interface Point {
    x: number;
    y: number;
}

function getPointFactory(x: number, y: number) {
    class P {
        x = x;
        y = y;
    }
    return P;
}

var PointZero = getPointFactory(0, 0);
var PointOne = getPointFactory(1, 1);
var p1 = new PointZero();
var p2 = new PointZero();
var p3 = new PointOne();

Local types may reference enclosing type parameters and local class and interfaces may themselves be generic. For example:

function f1() {
    function f() {
        class C<X, Y> {
            constructor(public x: X, public y: Y) { }
        }
        return C;
    }
    let C = f();
    let v = new C(10, "hello");
    let x = v.x;  // number
    let y = v.y;  // string
}

function f2() {
    function f<X>(x: X) {
        class C<Y> {
            public x = x;
            constructor(public y: Y) { }
        }
        return C;
    }
    let C = f(10);
    let v = new C("hello");
    let x = v.x;  // number
    let y = v.y;  // string
}

function f3() {
    function f<X, Y>(x: X, y: Y) {
        class C {
            public x = x;
            public y = y;
        }
        return C;
    }
    let C = f(10, "hello");
    let v = new C();
    let x = v.x;  // number
    let y = v.y;  // string
}

Fixes #3217.
Fixes #2148.

// 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) {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

I'd rather this be split into more ifs than cram the conditions into one boolean expression.

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) {

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

In general I think it is perfectly fine (and often preferable) to have multi-line boolean expressions instead of multiple if statements with "explaining variables". The latter just make the code even more complicated to look at. There's nothing inherently simpler about multiple if statements compared to multiple boolean operands separated by && and ||. Quite the opposite actually, since with boolean expressions you know there aren't any side effects or other oddities you have to worry about.

I do agree that in this case putting each condition on a separate line might make sense.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

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 || and the next does &&, or one uses ! while the other does not, then it's very easy to 'trip' over the expression, forcing the need to go back and scan and understand the expression over and over again.

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.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

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.

function appendTypeParameters(typeParameters: TypeParameter[], declarations: TypeParameterDeclaration[]): TypeParameter[] {
for (let declaration of declarations) {
let tp = getDeclaredTypeOfTypeParameter(getSymbolOfNode(declaration));
if (!typeParameters) {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

You could just move this check outside the loop.

if (!typeParameters && declarations.length > 0) {
    typeParameters = [];
}

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

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.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

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 !typeParameters check), we should likely mark it as such.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

No, typeParameters is the object being operated on, but its allocation is deferred and never happens if there are no type parameters.

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

@ahejlsberg It would be best to document that for consumers so that they can quickly eye the function and know this.

return typeParameters;
}

function appendOuterTypeParameters(typeParameters: TypeParameter[], node: Node): TypeParameter[]{

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

Space before {

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

This is the &(@#$&( auto formatting. Are we ever going to fix that bug?

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

I couldn't find the corresponding issue, so I filed #3269.

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

Which is a duplicate of #2628.

return appendOuterTypeParameters(undefined, getDeclarationOfKind(symbol, kind));
}

// The local type parameters are the combined set of type parameters from all declarations of the class or interface.

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

"all declarations of the class" - I'm a little confused, when can we get multiple declarations of a class?

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

Unless you mean all class and interface declarations of a symbol.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

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.

@@ -300,6 +300,12 @@ module ts {
FirstNode = QualifiedName,
}

export const enum StatementFlags {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

StatementFlags seems to only be used in parser.ts, so I don't think it warrants being in types.ts.

This comment has been minimized.

Copy link
@CyrusNajmabadi
nextToken();
continue;
default:
return 0;

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

I don't like using 0 here. What does it imply? Perhaps make a StatementFlags.Unknown or StatementFlags.None.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

These are bit flags. 0 means no flags. I think that's clearer than Unknown for which I'd have to look at the definition to know it means no bit flags.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

how about StatementFlags.NotStatement

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

What's so wrong about 0? I just don't get it. StatementFlags.NotStatement might actually be non-zero and I would have to navigate to the definition to find out. As it is now it is both shorter and clearer!

This comment has been minimized.

Copy link
@RyanCavanaugh

RyanCavanaugh May 26, 2015

Member

0 is out of place in a function that otherwise returns non-numeric-literal values. When I see this code, I'm thinking "Is this actually NYI?"

What's unclear or unambiguous about None? It's widely-used as a zero value in bitflag enums.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

Since we've used None elsewhere I will go with that. But we really should use 0 everywhere to indicate no bit flags. I can't think of anything shorter or more unambiguous.

@@ -1001,10 +1001,10 @@ module ts {
switch (parsingContext) {
case ParsingContext.SourceElements:
case ParsingContext.ModuleElements:
return isSourceElement(inErrorRecovery);
return !(token === SyntaxKind.SemicolonToken && inErrorRecovery) && isModuleElement();

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

This is very cryptic and difficult to read. Can you please extract this into a helper function that makes the flow more clear. For example, i would expect it to look something like:

function isSourceOrModuleElement(inErrorRecovery: boolean) {
    // Explanation of this case so people understand why we do this.
    if (token === SyntaxKind.Semicolon && inErrorRecovery) {
        return false;
    }

    return isModuleElement();
}

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

No, I much prefer it this way. It tells me exacly what is going on and I don't have to navigate to a "helper" function to figure it out.

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 26, 2015

Member

Helper or not, at the very least, there should be a comment above the function explaining what inErrorRecovery implies about the behavior of isListElement.

case ParsingContext.BlockStatements:
case ParsingContext.SwitchClauseStatements:
return isStartOfStatement(inErrorRecovery);
return !(token === SyntaxKind.SemicolonToken && inErrorRecovery) && isStatement();

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

Same here.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

No, it's better this way. Plus there's no reason why isStatement should be bothered with an inErrorRecovery flag that it doesn't otherwise need.

@@ -345,7 +345,7 @@ module ts {
module Parser {
// Share a single scanner across all calls to parse a source file. This helps speed things
// up by avoiding the cost of creating/compiling scanners over and over again.
const scanner = createScanner(ScriptTarget.Latest, /*skipTrivia:*/ true);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

The comment is supposed to resemble a 'named parameter'. That's how they were originally introduced. And it means they're easy to search for as well. I would keep the colon.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

It is inconsistent throughout our code base, but the majority of comments have no colon. For example, there are none in checker.ts, our largest file. I chose to align with that. My preference is no colon as I don't think it adds any value.

@@ -3380,7 +3374,7 @@ module ts {
function parseBlock(ignoreMissingOpenBrace: boolean, checkForStrictMode: boolean, diagnosticMessage?: DiagnosticMessage): Block {
let node = <Block>createNode(SyntaxKind.Block);
if (parseExpected(SyntaxKind.OpenBraceToken, diagnosticMessage) || ignoreMissingOpenBrace) {
node.statements = parseList(ParsingContext.BlockStatements, checkForStrictMode, parseStatement);
node.statements = <NodeArray<Statement>>parseList(ParsingContext.BlockStatements, checkForStrictMode, parseStatement);

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

This is odd. Shouldn't we infer the type properly because you pass in parseStatement.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

Yes, that's a leftover temporary type assertion. I will get rid of it.

case SyntaxKind.InterfaceKeyword:
case SyntaxKind.TypeKeyword:
nextToken();
return isIdentifierOrKeyword() ? StatementFlags.Statement : 0;

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

Create a value in teh enum for the 0 case.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

No, I think it is clearer this way. The literal 0 unequivocally means no flags.

case SyntaxKind.ModuleKeyword:
case SyntaxKind.NamespaceKeyword:
nextToken();
return isIdentifierOrKeyword() || token === SyntaxKind.StringLiteral ? StatementFlags.ModuleElement : 0;

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

can you parenthesize this to make the precedence clearer. Or extract into a helper function.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

I don't think there's any need for either of those.

return isIdentifierOrKeyword() || token === SyntaxKind.StringLiteral ? StatementFlags.ModuleElement : 0;
case SyntaxKind.ImportKeyword:
nextToken();
return token === SyntaxKind.StringLiteral || token === SyntaxKind.AsteriskToken ||

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

parenthesize for clarity.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

No need.

if (lookAhead(nextTokenIsIdentifierOrKeywordOnSameLine)) {
return false;
}
return getDeclarationFlags() ||

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

This is very confusing to read. Can you extract out a helper function and comment how this works. Or, alternatively, provide comments to make this clearer.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

Yup, I will add a comment.

}

function parseVariableStatementOrFunctionDeclarationOrClassDeclarationWithDecoratorsOrModifiers(): FunctionDeclaration | VariableStatement | ClassDeclaration {
let start = scanner.getStartPos();
function parseDeclaration(): ModuleElement {

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

rename to parseModuleElement

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

No, parseDeclaration is the right name. A ModuleElement can be many other things that we don't parse here.

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor

Shouldn't it return a Declaration then?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member

No, a Declaration can also be many other things we don't parse here.

@@ -1493,6 +1499,8 @@ module ts {
// Class and interface types (TypeFlags.Class and TypeFlags.Interface)
export interface InterfaceType extends ObjectType {
typeParameters: TypeParameter[]; // Type parameters (undefined if non-generic)
outerTypeParameters: TypeParameter[]; // Outer type parameters (undefined if none)
localTypeParameters: TypeParameter[]; // Local type parameters (undefined if none)
}

export interface InterfaceTypeWithBaseTypes extends InterfaceType {

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

I expected to see things like changes to ClassDeclation/EnumDeclaration/etc. to make them statements.

If they're not statements, then we need to change things like Block to have an array of ModuleElements instead.

Right now the type system isn't correct vis-a-vis what we say you may encounter and what you can actually encounter.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

They are. You're looking at the type interfaces here, not the node interfaces.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

I'm a bit confused. For example, I don't see a change to EnumDeclaration to make it a Statement. As such, i would not expect that it could be contained within a Block.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 26, 2015

Author Member

Right, it looks like only ClassDeclaration was changed. I will fix the others.

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor
  1. I do not see ClassDeclaration changed here, but you said you did change that one to be a statement, right?
  2. Are there still things that are only ModuleElements but not statements? Like module/namespace declarations for instance? Are there any others?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member
  1. ClassDeclaration definitely has been changed to extend Statement. Not sure what you looking at.
  2. Module, namespace, export, and import declarations are not statements (but export can be used as a modifier and is then allowed).

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor
  1. I meant in this PR, but I guess it's been changed before.
  2. To clarify, the export modifier in a statement context is not allowed, it's just parsed as a statement.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member

We always parse all modifiers on a declaration and then issue errors later during type check. We need to do this so our incremental parsing always parses declarations the same. Also, it provides better error handling.

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor

Yes, that makes sense. Thanks for explaining.

}
if (node.kind === SyntaxKind.ClassDeclaration || node.kind === SyntaxKind.FunctionDeclaration ||
node.kind === SyntaxKind.FunctionExpression || node.kind === SyntaxKind.MethodDeclaration ||
node.kind === SyntaxKind.ArrowFunction) {

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

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.

// 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;
return appendOuterTypeParameters(undefined, getDeclarationOfKind(symbol, kind));

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

named parameter for the "undefined".

break;
case SyntaxKind.EnumDeclaration:
if (node.modifiers && (node.modifiers.length > 1 || node.modifiers[0].kind !== SyntaxKind.ConstKeyword) &&
node.parent.kind !== SyntaxKind.ModuleBlock && node.parent.kind !== SyntaxKind.SourceFile) {

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

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", ).

}
break;
case SyntaxKind.EnumDeclaration:
if (node.modifiers && (node.modifiers.length > 1 || node.modifiers[0].kind !== SyntaxKind.ConstKeyword) &&

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 26, 2015

Contributor

Break into multiple checks. The mix of &&s and ||s make this difficult to read.

nextToken();
return token === SyntaxKind.EnumKeyword
function isStatement(): boolean {
return (getStatementFlags() & StatementFlags.Statement) !== 0;

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor

I am confused about the purpose of the getStatementFlags approach. Why is it better than having isStatement and isModuleElement just make the decisions for themselves? Is it so that there can be just one place (instead of two) to account for all the node kinds here, so we don't forget any?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member

The problem is modifiers. When modifiers are present we can't tell from the first token what we're looking at. So we need to look ahead, but we don't want to do that more than once. And, as you say, when everything is in one place we're certain that there won't be any subtle differences.

return parseModuleElementOfKind(StatementFlags.StatementOrModuleElement);
}

function parseModuleElementOfKind(flags: StatementFlags): ModuleElement {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor

I think it's simpler if this just takes a boolean saying whether a module element is allowed. That's the only thing that varies in the calls to this function.

Then at the spot where do use the flags, just use the boolean to figure out whether you need the declaration flags to say StatementOrModuleElement.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member

Having a boolean parameter just makes the actual testing more complicated (because you now have to construct a bit mask from the boolean). I tried it that way and it just made it more complex to read.

case SyntaxKind.PublicKeyword:
case SyntaxKind.StaticKeyword:
case SyntaxKind.TypeKeyword:
if (getDeclarationFlags() & flags) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor

What is the intent here? I do not understand this part?

Also, what is the commonality among all these cases here? I thought it was all the declarations, but var and let are missing from this.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member

The commonality here is (a) that we want to use getDeclarationFlags to determine if we're looking at a declaration (which involves look-ahead for these starter tokens) and if so (b) call parseDeclaration which has common handling for decorators and modifiers.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member

var and let are missing because with those we already know what we're looking at. Thus, it would be a waste of time to look ahead and to check for decorators and modifiers.

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 28, 2015

Contributor

I still do not understand.

case ParsingContext.BlockStatements:
case ParsingContext.SwitchClauseStatements:
return isStartOfStatement(inErrorRecovery);
// During error recovery we don't treat empty statements as statements
return !(token === SyntaxKind.SemicolonToken && inErrorRecovery) && isStatement();

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi May 28, 2015

Contributor

I've given this a lot of thought, and i'm very opposed to what's happened here. Originally the code was very clear in intent and placed responsibility in the right location. This list parsing code simply asked:

does it look like we're starting a statement here, depending on whether we're in error recovery or not?

That function that was called did the right thing, and could be nicely composed within other functions (for example, inside isSourceElement, or parseArrowFunctionExpressionBody). Now, we've taken out the single existing error recovery case this used to handle (i.e. semicolons) and inlined that logic in 3 separate places instead of leaving it directly at the point where we see a semicolon and need to determine if it is a valid statement or not.

This is non-ideal for two reasons:

  1. It violates DRY (don't repeat yourself). Instead of having a single place where we deal with error recovery and statements, we now have three places. If future parts of the code need to use isStatement, they may forget about this case. With the existing approach consumers of the isStatement API have to think this through properly.
  2. It means any further error recovery work we do around statements will have to be put in a multiplicity of places that the author will need to discover and hunt down. As error recovery improvements are always happening throughout the life of a language, i would expect us to have to do work here in the future. The new approach makes this much more difficult.

The original code expresses intent better and provides a better solution for code composition. Looking at the structure of hte new code, i don't see any reason at all why passing along inErrorRecovery would be a problem. As such, I would strong prefer we move back that style.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 28, 2015

Author Member

I disagree. We don't propagate this flag to any of the other isXXX helpers and with the current structure all handling of inErrorRecovery is now local to isListElement, making it much clearer what the effect of the flag actually is.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

commented May 30, 2015

In terms of the parser, I prefer the approach where we indiscriminately parse statements and module elements, and subsequently error in the checker if you have a module element in a block context. Since we may eventually allow module declarations inside functions, that work may prove useful to us anyway. But I defer to you, and otherwise, I sign off.

👍

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

commented May 30, 2015

Since we've moved from typeParameters to innerTypeParameters, what is the interaction with signature help now?

For instance:

function f<FT>(x: FT) {
    class C<CT> {
        private y: CT;
    }

    return C;
}

let con = f(10);
let obj = new con</**/

Do we only see inner type params? This would be a good fourslash test.

@@ -3908,7 +4003,7 @@ module ts {
return mapper(<TypeParameter>type);
}
if (type.flags & TypeFlags.Anonymous) {
return type.symbol && type.symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.TypeLiteral | SymbolFlags.ObjectLiteral) ?
return type.symbol && type.symbol.flags & (SymbolFlags.Function | SymbolFlags.Method | SymbolFlags.Class | SymbolFlags.TypeLiteral | SymbolFlags.ObjectLiteral) ?

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 30, 2015

Contributor

Is this for anonymous class expressions?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 30, 2015

Author Member

No, it's for the anonymous type created for the static side of a class.

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 30, 2015

Contributor

Ah yes, because previously the static side of a class could never get instantiated.

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,

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 30, 2015

Contributor

By the way, I think there is an optimization you can do here. If typeArgCount and expectedTypeArgCount are zero, then type is already the correct thing because all the type arguments are exactly the same as the type arguments of the declared type. createTypeReference will just look up the type in the instantiations cache and find the type itself.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 30, 2015

Author Member

I like it.

@@ -1526,17 +1534,53 @@ module ts {
}
}

function writeSymbolTypeReference(symbol: Symbol, typeArguments: Type[], pos: number, end: number) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 30, 2015

Contributor

Can you rename pos and end, or add a comment saying what the pos and end are referring to?

@@ -1526,17 +1534,53 @@ module ts {
}
}

function writeSymbolTypeReference(symbol: Symbol, typeArguments: Type[], pos: number, end: number) {
if (!isReservedMemberName(symbol.name)) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 30, 2015

Contributor

Why do you need to check this? What are the cases where you hit a reserved member name that has type parameters, causing you to need to print it?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 30, 2015

Author Member

This happens with unnamed generic function expressions, generic arrow functions, and unnamed generic class expressions.

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 31, 2015

Contributor

Okay, can you put it in a comment? Thanks

@@ -1526,17 +1534,53 @@ module ts {
}
}

function writeSymbolTypeReference(symbol: Symbol, typeArguments: Type[], pos: number, end: number) {
if (!isReservedMemberName(symbol.name)) {
buildSymbolDisplay(symbol, writer, enclosingDeclaration, SymbolFlags.Type);

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman May 30, 2015

Contributor

I think instead of SymbolFlags.Type you should pass symbol.flags. Though I admit, I don't know what it will do, and I'm really not sure. I'm just thinking in the case where it is a function with type parameters, it is not really a Type.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg May 31, 2015

Author Member

If anything it should be SymbolFlags.Type or SymbolFlags.Value, but I'd rather leave it the way it is. It has to do with how aliases are resolved, but I don't think it matters. The sooner we can clean up that cryptic stuff the better.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

commented May 30, 2015

Writing the types looks good 👍

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

commented May 30, 2015

@DanielRosenwasser We do not have signature help for type arguments, only arguments. However, we do show the type arguments when we are giving sig help for the arguments, so this would be a good thing to test.

ahejlsberg added a commit that referenced this pull request May 31, 2015

@ahejlsberg ahejlsberg merged commit 0872ed6 into master May 31, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ahejlsberg ahejlsberg deleted the localTypes branch May 31, 2015

@danquirk danquirk referenced this pull request Jul 23, 2015

Closed

Generic meta types #3779

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.