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

Assertions in control flow analysis #32695

Merged
merged 44 commits into from
Sep 23, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
e89acb6
Reflect effects of assertion calls in control flow analysis
ahejlsberg Jul 31, 2019
77f2a41
Support 'asserts' type predicates in control flow analysis
ahejlsberg Aug 3, 2019
1f5bb97
Remove unused code
ahejlsberg Aug 3, 2019
fe70a62
Accept new API baselines
ahejlsberg Aug 3, 2019
1c55e5d
Address code review feedback
ahejlsberg Aug 5, 2019
df02ad6
Reflect control flow effects of calls to never-returning functions
ahejlsberg Aug 8, 2019
99ab53e
Make flow nodes more monomorphic
ahejlsberg Aug 9, 2019
d5e08d4
Accept baseline API changes
ahejlsberg Aug 9, 2019
19f1d3b
Less aggressive monomorphism for flow nodes
ahejlsberg Aug 9, 2019
83212e7
Accept API baseline changes
ahejlsberg Aug 9, 2019
259ba77
Merge branch 'master' into assertionsInControlFlow
ahejlsberg Aug 9, 2019
cdeddf1
Call getResolvedSignature only when needed for generics or overloads
ahejlsberg Aug 10, 2019
9791f1d
Merge branch 'master' into assertionsInControlFlow
ahejlsberg Aug 10, 2019
0599f84
Support 'asserts this' and 'asserts this is T' type predicates
ahejlsberg Aug 17, 2019
e7cbfc4
Update API to be backwards compatible
ahejlsberg Aug 17, 2019
2c36249
Accept new API baselines
ahejlsberg Aug 17, 2019
2152874
Address CR feedback
ahejlsberg Sep 10, 2019
8791b62
Accept new baselines
ahejlsberg Sep 11, 2019
5a180ba
Merge branch 'master' into assertionsInControlFlow
ahejlsberg Sep 11, 2019
436339d
Use declared type for references in unreachable code
ahejlsberg Sep 12, 2019
a9336ba
Revert "Use declared type for references in unreachable code"
ahejlsberg Sep 12, 2019
971b0df
Use declared type for references in unreachable code (again)
ahejlsberg Sep 13, 2019
3749de6
Dedicated isReachableFlowNode function to determine reachability
ahejlsberg Sep 13, 2019
3a89c8c
Use isReachableFlowNode to check for implicit return
ahejlsberg Sep 13, 2019
cc6e493
Treat exhaustive switch statements like non-returning functions in CFA
ahejlsberg Sep 14, 2019
0060964
Further CFA handling of exhaustive switch statements
ahejlsberg Sep 15, 2019
51dcce2
Accept new baselines
ahejlsberg Sep 15, 2019
59b76ce
Fix call to Debug.fail in compiler
ahejlsberg Sep 15, 2019
945babb
Fix inference circularity error triggered by exhaustive switch analysis
ahejlsberg Sep 15, 2019
d26afd7
for-in or for-of expression is evaluated before loop back edge
ahejlsberg Sep 15, 2019
05d1e68
Fix linting issues
ahejlsberg Sep 16, 2019
e97ebb7
More efficient scheme for caching flow node reachability
ahejlsberg Sep 16, 2019
def5e37
Revert "More efficient scheme for caching flow node reachability"
ahejlsberg Sep 16, 2019
6d6c620
Report grammatic and type-based unreachable code errors in the same way
ahejlsberg Sep 16, 2019
d9c9129
Ignore references in with statements in getTypeOfDottedName
ahejlsberg Sep 16, 2019
282a7af
Accept new API baselines
ahejlsberg Sep 16, 2019
8cbf694
Cache last isReachableFlowNode result + switch statement CFA fix
ahejlsberg Sep 17, 2019
9466025
Accept new baselines
ahejlsberg Sep 17, 2019
ba30fdc
Attach flow nodes only when allowUnreachableCode !== true
ahejlsberg Sep 18, 2019
cafec55
Properly handle try-finally statements in isReachableFlowNode
ahejlsberg Sep 18, 2019
21b5418
Add tests
ahejlsberg Sep 21, 2019
97d69d4
Accept new baselines
ahejlsberg Sep 21, 2019
c3dcc37
Merge branch 'master' into assertionsInControlFlow
ahejlsberg Sep 21, 2019
bcdf33d
Fix forEachChild
ahejlsberg Sep 21, 2019
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
24 changes: 24 additions & 0 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,9 @@ namespace ts {
case SyntaxKind.CaseClause:
bindCaseClause(<CaseClause>node);
break;
case SyntaxKind.ExpressionStatement:
bindExpressionStatement(<ExpressionStatement>node);
break;
case SyntaxKind.LabeledStatement:
bindLabeledStatement(<LabeledStatement>node);
break;
Expand Down Expand Up @@ -896,6 +899,11 @@ namespace ts {
return flowNodeCreated({ flags: FlowFlags.Assignment, antecedent, node });
}

function createFlowCall(antecedent: FlowNode, node: CallExpression): FlowNode {
setFlowNodeReferenced(antecedent);
return flowNodeCreated({ flags: FlowFlags.Call, antecedent, node });
}

function createFlowArrayMutation(antecedent: FlowNode, node: CallExpression | BinaryExpression): FlowNode {
setFlowNodeReferenced(antecedent);
const res: FlowArrayMutation = flowNodeCreated({ flags: FlowFlags.ArrayMutation, antecedent, node });
Expand Down Expand Up @@ -1276,6 +1284,22 @@ namespace ts {
activeLabels!.pop();
}

function isDottedName(node: Expression): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the difference to isEntityNameExpression?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! No difference, will change to use the existing function.

return node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.PropertyAccessExpression && isDottedName((<PropertyAccessExpression>node).expression);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to include this and super in property access expressions?

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 think that would be okay, but I'll have to convince myself it can't trigger circularities in control flow analysis.

}

function bindExpressionStatement(node: ExpressionStatement): void {
bind(node.expression);
// A top level call expression with a dotted function name and at least one argument
// is potentially an assertion and is therefore included in the control flow.
if (node.expression.kind === SyntaxKind.CallExpression) {
Copy link
Member

Choose a reason for hiding this comment

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

Before I forget: Even if we don't support them, we'll need tests for our behavior of:

  • Assertion constructors (arguably reasonable to support, just because construction is just calling with a replaced return value, minimally in the context of callable constructors a la js functions, it should be tested)
  • Assertion tagged templates (a tagged template is actually just a call, but given a valid template type signature, the most you could say is something like function html(text: TemplateStringsArray, holes: any[]): asserts holes[0] is Something which is... interesting.)
  • Assertion decorators (I don't even know what someone would expect here - asserting the type of the class your wrapping?)
  • Assertion JSX tags (lolwut - jsx tags are fake calls, so at best you could assert something about one of the props you're passed in?)
  • Getter/setter pairs with assertion predicates

const call = <CallExpression>node.expression;
if (isDottedName(call.expression) && call.arguments.length >= 1) {
currentFlow = createFlowCall(currentFlow, call);
}
}
}

function bindLabeledStatement(node: LabeledStatement): void {
const preStatementLabel = createLoopLabel();
const postStatementLabel = createBranchLabel();
Expand Down
229 changes: 139 additions & 90 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

14 changes: 10 additions & 4 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1906,11 +1906,17 @@ namespace ts {
//

function emitTypePredicate(node: TypePredicateNode) {
if (node.assertsModifier) {
emit(node.assertsModifier);
writeSpace();
}
emit(node.parameterName);
writeSpace();
writeKeyword("is");
writeSpace();
emit(node.type);
if (node.type) {
writeSpace();
writeKeyword("is");
writeSpace();
emit(node.type);
}
}

function emitTypeReference(node: TypeReferenceNode) {
Expand Down
7 changes: 4 additions & 3 deletions src/compiler/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -667,17 +667,18 @@ namespace ts {
return <KeywordTypeNode>createSynthesizedNode(kind);
}

export function createTypePredicateNode(parameterName: Identifier | ThisTypeNode | string, type: TypeNode) {
export function createTypePredicateNode(assertsModifier: AssertsToken | undefined, parameterName: Identifier | ThisTypeNode | string, type: TypeNode | undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a breaking API change.
typically there is a new overload added to maintain backwards compatibility. the old signature can be marked as deprecated right away and could be removed later.

const node = createSynthesizedNode(SyntaxKind.TypePredicate) as TypePredicateNode;
node.assertsModifier = assertsModifier;
node.parameterName = asName(parameterName);
node.type = type;
return node;
}

export function updateTypePredicateNode(node: TypePredicateNode, parameterName: Identifier | ThisTypeNode, type: TypeNode) {
export function updateTypePredicateNode(node: TypePredicateNode, assertsModifier: AssertsToken | undefined, parameterName: Identifier | ThisTypeNode, type: TypeNode | undefined) {
return node.parameterName !== parameterName
|| node.type !== type
? updateNode(createTypePredicateNode(parameterName, type), node)
? updateNode(createTypePredicateNode(assertsModifier, parameterName, type), node)
: node;
}

Expand Down
13 changes: 13 additions & 0 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2992,6 +2992,8 @@ namespace ts {
return parseParenthesizedType();
case SyntaxKind.ImportKeyword:
return parseImportType();
case SyntaxKind.AssertsKeyword:
return lookAhead(nextTokenIsIdentifierOrKeywordOnSameLine) ? parseAssertsTypePredicate() : parseTypeReference();
default:
return parseTypeReference();
}
Expand Down Expand Up @@ -3032,6 +3034,7 @@ namespace ts {
case SyntaxKind.DotDotDotToken:
case SyntaxKind.InferKeyword:
case SyntaxKind.ImportKeyword:
case SyntaxKind.AssertsKeyword:
return true;
case SyntaxKind.FunctionKeyword:
return !inStartOfParameter;
Expand Down Expand Up @@ -3225,6 +3228,16 @@ namespace ts {
}
}

function parseAssertsTypePredicate(): TypeNode {
const node = <TypePredicateNode>createNode(SyntaxKind.TypePredicate);
node.assertsModifier = parseExpectedToken(SyntaxKind.AssertsKeyword);
Copy link
Contributor

Choose a reason for hiding this comment

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

adding this property here and not assigning it in parseTypeOrTypePredicate where regular TypePredicate nodes are constructed, create yet another hidden class that hinders optimization at runtime.
Either assign it last in this function or (even better) assign it in both functions in the same order

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a possibility that there will be more modifiers in the future? If so, would it make sense to put this into Node#modifiers?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible, but for now I'm going to keep it the way it is.

node.parameterName = parseIdentifier();
if (parseOptional(SyntaxKind.IsKeyword)) {
node.type = parseType();
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes this type of object polymorphic. Could you instead always assign the property and use undefined if there is no type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

}
return finishNode(node);
}

function parseType(): TypeNode {
// The rules about 'yield' only apply to actual code/expression contexts. They don't
// apply to 'type' contexts. So we disable these parameters here before moving on.
Expand Down
1 change: 1 addition & 0 deletions src/compiler/scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ namespace ts {
abstract: SyntaxKind.AbstractKeyword,
any: SyntaxKind.AnyKeyword,
as: SyntaxKind.AsKeyword,
asserts: SyntaxKind.AssertsKeyword,
bigint: SyntaxKind.BigIntKeyword,
boolean: SyntaxKind.BooleanKeyword,
break: SyntaxKind.BreakKeyword,
Expand Down
49 changes: 33 additions & 16 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ namespace ts {
| SyntaxKind.AbstractKeyword
| SyntaxKind.AnyKeyword
| SyntaxKind.AsKeyword
| SyntaxKind.AssertsKeyword
| SyntaxKind.BigIntKeyword
| SyntaxKind.BooleanKeyword
| SyntaxKind.BreakKeyword
Expand Down Expand Up @@ -250,6 +251,7 @@ namespace ts {
// Contextual keywords
AbstractKeyword,
AsKeyword,
AssertsKeyword,
AnyKeyword,
AsyncKeyword,
AwaitKeyword,
Expand Down Expand Up @@ -734,6 +736,7 @@ namespace ts {
export type AwaitKeywordToken = Token<SyntaxKind.AwaitKeyword>;
export type PlusToken = Token<SyntaxKind.PlusToken>;
export type MinusToken = Token<SyntaxKind.MinusToken>;
export type AssertsToken = Token<SyntaxKind.AssertsKeyword>;

export type Modifier
= Token<SyntaxKind.AbstractKeyword>
Expand Down Expand Up @@ -1177,8 +1180,9 @@ namespace ts {
export interface TypePredicateNode extends TypeNode {
kind: SyntaxKind.TypePredicate;
parent: SignatureDeclaration | JSDocTypeExpression;
assertsModifier?: AssertsToken;
parameterName: Identifier | ThisTypeNode;
type: TypeNode;
type?: TypeNode;
}

export interface TypeQueryNode extends TypeNode {
Expand Down Expand Up @@ -2550,12 +2554,13 @@ namespace ts {
FalseCondition = 1 << 6, // Condition known to be false
SwitchClause = 1 << 7, // Switch statement clause
ArrayMutation = 1 << 8, // Potential array mutation
Referenced = 1 << 9, // Referenced as antecedent once
Shared = 1 << 10, // Referenced as antecedent more than once
PreFinally = 1 << 11, // Injected edge that links pre-finally label and pre-try flow
AfterFinally = 1 << 12, // Injected edge that links post-finally flow with the rest of the graph
Call = 1 << 9, // Potential assertion call
Referenced = 1 << 10, // Referenced as antecedent once
Shared = 1 << 11, // Referenced as antecedent more than once
PreFinally = 1 << 12, // Injected edge that links pre-finally label and pre-try flow
AfterFinally = 1 << 13, // Injected edge that links post-finally flow with the rest of the graph
/** @internal */
Cached = 1 << 13, // Indicates that at least one cross-call cache entry exists for this node, even if not a loop participant
Cached = 1 << 14, // Indicates that at least one cross-call cache entry exists for this node, even if not a loop participant
Label = BranchLabel | LoopLabel,
Condition = TrueCondition | FalseCondition
}
Expand All @@ -2574,7 +2579,7 @@ namespace ts {
}

export type FlowNode =
| AfterFinallyFlow | PreFinallyFlow | FlowStart | FlowLabel | FlowAssignment | FlowCondition | FlowSwitchClause | FlowArrayMutation;
| AfterFinallyFlow | PreFinallyFlow | FlowStart | FlowLabel | FlowAssignment | FlowCall | FlowCondition | FlowSwitchClause | FlowArrayMutation;
export interface FlowNodeBase {
flags: FlowFlags;
id?: number; // Node id used by flow type cache in checker
Expand All @@ -2599,6 +2604,11 @@ namespace ts {
antecedent: FlowNode;
}

export interface FlowCall extends FlowNodeBase {
node: CallExpression;
antecedent: FlowNode;
}

// FlowCondition represents a condition that is known to be true or false at the
// node's location in the control flow.
export interface FlowCondition extends FlowNodeBase {
Expand Down Expand Up @@ -3487,25 +3497,32 @@ namespace ts {

export const enum TypePredicateKind {
This,
Identifier
Identifier,
Assertion
}

export interface TypePredicateBase {
kind: TypePredicateKind;
export interface ThisTypePredicate {
kind: TypePredicateKind.This;
parameterName: undefined;
parameterIndex: undefined;
type: Type;
}

export interface ThisTypePredicate extends TypePredicateBase {
kind: TypePredicateKind.This;
export interface IdentifierTypePredicate {
kind: TypePredicateKind.Identifier;
parameterName: string;
parameterIndex: number;
type: Type;
}

export interface IdentifierTypePredicate extends TypePredicateBase {
kind: TypePredicateKind.Identifier;
export interface AssertionTypePredicate {
kind: TypePredicateKind.Assertion;
parameterName: string;
parameterIndex: number;
type: Type | undefined;
}

export type TypePredicate = IdentifierTypePredicate | ThisTypePredicate;
export type TypePredicate = ThisTypePredicate | IdentifierTypePredicate | AssertionTypePredicate;

/* @internal */
export type AnyImportSyntax = ImportDeclaration | ImportEqualsDeclaration;
Expand Down Expand Up @@ -3901,7 +3918,7 @@ namespace ts {
resolvedSignature?: Signature; // Cached signature of signature node or call expression
resolvedSymbol?: Symbol; // Cached name resolution result
resolvedIndexInfo?: IndexInfo; // Cached indexing info resolution result
maybeTypePredicate?: boolean; // Cached check whether call expression might reference a type predicate
resolvedTypePredicate?: TypePredicate; // Cached type predicate for call expression
enumMemberValue?: string | number; // Constant value of enum member
isVisible?: boolean; // Is this node visible
containsArgumentsReference?: boolean; // Whether a function-like declaration contains an 'arguments' reference
Expand Down
1 change: 1 addition & 0 deletions src/compiler/visitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ namespace ts {

case SyntaxKind.TypePredicate:
return updateTypePredicateNode(<TypePredicateNode>node,
visitNode((<TypePredicateNode>node).assertsModifier, visitor),
visitNode((<TypePredicateNode>node).parameterName, visitor),
visitNode((<TypePredicateNode>node).type, visitor, isTypeNode));

Expand Down
Loading