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

initial revision of reachability checks #4788

Merged
merged 25 commits into from
Nov 2, 2015
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b62ef0d
initial revision of reachability checks in binder
vladima Sep 3, 2015
beb1aa3
addressed PR feedback
vladima Sep 15, 2015
9aeb0f8
merge with master
vladima Sep 15, 2015
682c14c
addressed PR feedback
vladima Sep 16, 2015
69321a0
merge with master
vladima Sep 16, 2015
4711f0e
merge with master
vladima Sep 22, 2015
98ac805
Merge branch 'master' into reachabilityChecks
vladima Sep 29, 2015
ae175d0
merge with master
vladima Oct 1, 2015
9d24e0f
Merge branch 'master' into reachabilityChecks
vladima Oct 2, 2015
ca0d580
merge with master, fix linter issues
vladima Oct 2, 2015
f0f5a0d
updated command line options, accepted baselines
vladima Oct 3, 2015
eb04f32
Merge branch 'master' into reachabilityChecks
vladima Oct 3, 2015
ebfcd25
merge with master
vladima Oct 11, 2015
938dd74
Merge branch 'master' into reachabilityChecks
vladima Oct 13, 2015
17716fb
accepted baselines
vladima Oct 13, 2015
bc02341
addressed PR feedback, updated tests to suppress reachability errors …
vladima Oct 13, 2015
238e1c6
partially suppress reachability errors in tests
vladima Oct 13, 2015
5532778
suppress reachability errors in remaining tests
vladima Oct 13, 2015
7b12617
Merge branch 'master' into reachabilityChecks
vladima Oct 16, 2015
f9eaed7
Merge branch 'master' into reachabilityChecks
vladima Oct 19, 2015
f96980d
merge with master
vladima Oct 22, 2015
2779352
make binder singleton, inline bindWithReachabilityChecks
vladima Oct 22, 2015
7d09f26
defer allocation of error message text in binder
vladima Oct 22, 2015
d2a11b5
merge with master
vladima Oct 27, 2015
3f11c0b
merge with master
vladima Oct 29, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
477 changes: 447 additions & 30 deletions src/compiler/binder.ts

Large diffs are not rendered by default.

56 changes: 24 additions & 32 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ namespace ts {
let getGlobalPromiseConstructorLikeType: () => ObjectType;
let getGlobalThenableType: () => ObjectType;

let jsxElementClassType: Type;

let tupleTypes: Map<TupleType> = {};
let unionTypes: Map<UnionType> = {};
let intersectionTypes: Map<IntersectionType> = {};
Expand Down Expand Up @@ -7845,7 +7847,6 @@ namespace ts {
return prop || unknownSymbol;
}

let jsxElementClassType: Type = undefined;
function getJsxGlobalElementClassType(): Type {
if (!jsxElementClassType) {
jsxElementClassType = getExportedTypeFromNamespace(JsxNames.JSX, JsxNames.ElementClass);
Expand Down Expand Up @@ -9589,21 +9590,11 @@ namespace ts {
return aggregatedTypes;
}

function bodyContainsAReturnStatement(funcBody: Block) {
return forEachReturnStatement(funcBody, returnStatement => {
return true;
});
}

function bodyContainsSingleThrowStatement(body: Block) {
return (body.statements.length === 1) && (body.statements[0].kind === SyntaxKind.ThrowStatement);
}

// TypeScript Specification 1.0 (6.3) - July 2014
// An explicitly typed function whose return type isn't the Void or the Any type
// must have at least one return statement somewhere in its body.
// An exception to this rule is if the function implementation consists of a single 'throw' statement.
function checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(func: FunctionLikeDeclaration, returnType: Type): void {
function checkAllCodePathsInNonVoidFunctionReturnOrThrow(func: FunctionLikeDeclaration, returnType: Type): void {
if (!produceDiagnostics) {
return;
}
Expand All @@ -9614,26 +9605,20 @@ namespace ts {
}

// If all we have is a function signature, or an arrow function with an expression body, then there is nothing to check.
if (nodeIsMissing(func.body) || func.body.kind !== SyntaxKind.Block) {
// also if HasImplicitReturnValue flags is not set this means that all codepaths in function body end with return of throw
if (nodeIsMissing(func.body) || func.body.kind !== SyntaxKind.Block || !(func.flags & NodeFlags.HasImplicitReturn)) {
return;
}

let bodyBlock = <Block>func.body;

// Ensure the body has at least one return expression.
if (bodyContainsAReturnStatement(bodyBlock)) {
return;
if (func.flags & NodeFlags.HasExplicitReturn) {
if (compilerOptions.noImplicitReturns) {
error(func.type, Diagnostics.Not_all_code_paths_return_a_value);
}
}

// If there are no return expressions, then we need to check if
// the function body consists solely of a throw statement;
// this is to make an exception for unimplemented functions.
if (bodyContainsSingleThrowStatement(bodyBlock)) {
return;
else {
// This function does not conform to the specification.
error(func.type, Diagnostics.A_function_whose_declared_type_is_neither_void_nor_any_must_return_a_value);
}

// This function does not conform to the specification.
error(func.type, Diagnostics.A_function_whose_declared_type_is_neither_void_nor_any_must_return_a_value_or_consist_of_a_single_throw_statement);
}

function checkFunctionExpressionOrObjectLiteralMethod(node: FunctionExpression | MethodDeclaration, contextualMapper?: TypeMapper): Type {
Expand Down Expand Up @@ -9713,7 +9698,7 @@ namespace ts {
}

if (returnType && !node.asteriskToken) {
checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, isAsync ? promisedType : returnType);
checkAllCodePathsInNonVoidFunctionReturnOrThrow(node, isAsync ? promisedType : returnType);
}

if (node.body) {
Expand Down Expand Up @@ -10914,8 +10899,15 @@ namespace ts {
checkGrammarFunctionLikeDeclaration(node) || checkGrammarAccessor(node) || checkGrammarComputedPropertyName(node.name);

if (node.kind === SyntaxKind.GetAccessor) {
if (!isInAmbientContext(node) && nodeIsPresent(node.body) && !(bodyContainsAReturnStatement(<Block>node.body) || bodyContainsSingleThrowStatement(<Block>node.body))) {
error(node.name, Diagnostics.A_get_accessor_must_return_a_value_or_consist_of_a_single_throw_statement);
if (!isInAmbientContext(node) && nodeIsPresent(node.body) && (node.flags & NodeFlags.HasImplicitReturn)) {
if (node.flags & NodeFlags.HasExplicitReturn) {
if (compilerOptions.noImplicitReturns) {
error(node.name, Diagnostics.Not_all_code_paths_return_a_value);
}
}
else {
error(node.name, Diagnostics.A_get_accessor_must_return_a_value);
}
}
}

Expand Down Expand Up @@ -11843,7 +11835,7 @@ namespace ts {
promisedType = checkAsyncFunctionReturnType(node);
}

checkIfNonVoidFunctionHasReturnExpressionsOrSingleThrowStatment(node, isAsync ? promisedType : returnType);
checkAllCodePathsInNonVoidFunctionReturnOrThrow(node, isAsync ? promisedType : returnType);
}

if (produceDiagnostics && !node.type) {
Expand Down Expand Up @@ -14871,7 +14863,7 @@ namespace ts {
function initializeTypeChecker() {
// Bind all source files and propagate errors
forEach(host.getSourceFiles(), file => {
bindSourceFile(file);
bindSourceFile(file, compilerOptions);
});

// Initialize global symbol table
Expand Down
20 changes: 20 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,26 @@ namespace ts {
},
description: Diagnostics.Specifies_module_resolution_strategy_Colon_node_Node_js_or_classic_TypeScript_pre_1_6,
error: Diagnostics.Argument_for_moduleResolution_option_must_be_node_or_classic,
},
{
name: "allowUnusedLabels",
type: "boolean",
description: Diagnostics.Do_not_report_errors_on_unused_labels
},
{
name: "noImplicitReturns",
type: "boolean",
description: Diagnostics.Report_error_when_not_all_code_paths_in_function_return_a_value
Copy link
Contributor

Choose a reason for hiding this comment

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

nit : Report_errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't have a consistent style here. My understanding of phrasing is: 'use this option to do...' but not 'this option does...' so I'd prefer to keep existing version. Probably @JsonFreeman has different opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yuit's comment was about singular vs plural, rather than the mood, if I understood correctly. The mood inconsistency is a different issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JsonFreeman @vladima yep, it is more about the consistency on plural or singular on the word "error". I will be ok either one but I think it is nice to keep them consistent

},
{
name: "noFallthroughCasesInSwitch",
type: "boolean",
description: Diagnostics.Report_errors_for_fallthrough_cases_in_switch_statement
},
{
name: "allowUnreachableCode",
type: "boolean",
description: Diagnostics.Do_not_report_errors_on_unreachable_code
}
];

Expand Down
39 changes: 34 additions & 5 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@
"category": "Error",
"code": 2354
},
"A function whose declared type is neither 'void' nor 'any' must return a value or consist of a single 'throw' statement.": {
"A function whose declared type is neither 'void' nor 'any' must return a value.": {
"category": "Error",
"code": 2355
},
Expand Down Expand Up @@ -1092,7 +1092,7 @@
"category": "Error",
"code": 2377
},
"A 'get' accessor must return a value or consist of a single 'throw' statement.": {
"A 'get' accessor must return a value.": {
"category": "Error",
"code": 2378
},
Expand Down Expand Up @@ -2298,7 +2298,22 @@
"category": "Message",
"code": 6072
},

"Do not report errors on unused labels.": {
"category": "Message",
"code": 6073
},
"Report error when not all code paths in function return a value.": {
"category": "Message",
"code": 6074
},
"Report errors for fallthrough cases in switch statement.": {
"category": "Message",
"code": 6075
},
"Do not report errors on unreachable code.": {
"category": "Message",
"code": 6076
},
"Variable '{0}' implicitly has an '{1}' type.": {
"category": "Error",
"code": 7005
Expand Down Expand Up @@ -2367,8 +2382,22 @@
"category": "Error",
"code": 7026
},


"Unreachable code detected.": {
"category": "Error",
"code": 7027
},
"Unused label.": {
"category": "Error",
"code": 7028
},
"Fallthrough case in switch.": {
"category": "Error",
"code": 7029
},
"Not all code paths return a value.": {
"category": "Error",
"code": 7030
},
"You cannot rename this element.": {
"category": "Error",
"code": 8000
Expand Down
47 changes: 27 additions & 20 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,28 +362,31 @@ namespace ts {

export const enum NodeFlags {
None = 0,
Export = 0x00000001, // Declarations
Ambient = 0x00000002, // Declarations
Public = 0x00000010, // Property/Method
Private = 0x00000020, // Property/Method
Protected = 0x00000040, // Property/Method
Static = 0x00000080, // Property/Method
Abstract = 0x00000100, // Class/Method/ConstructSignature
Async = 0x00000200, // Property/Method/Function
Default = 0x00000400, // Function/Class (export default declaration)
MultiLine = 0x00000800, // Multi-line array or object literal
Synthetic = 0x00001000, // Synthetic node (for full fidelity)
DeclarationFile = 0x00002000, // Node is a .d.ts file
Let = 0x00004000, // Variable declaration
Const = 0x00008000, // Variable declaration
OctalLiteral = 0x00010000, // Octal numeric literal
Namespace = 0x00020000, // Namespace declaration
ExportContext = 0x00040000, // Export context (initialized by binding)
ContainsThis = 0x00080000, // Interface contains references to "this"

Export = 1 << 1, // Declarations
Ambient = 1 << 2, // Declarations
Public = 1 << 3, // Property/Method
Private = 1 << 4, // Property/Method
Protected = 1 << 5, // Property/Method
Static = 1 << 6, // Property/Method
Abstract = 1 << 7, // Class/Method/ConstructSignature
Async = 1 << 8, // Property/Method/Function
Default = 1 << 9, // Function/Class (export default declaration)
MultiLine = 1 << 10, // Multi-line array or object literal
Synthetic = 1 << 11, // Synthetic node (for full fidelity)
DeclarationFile = 1 << 12, // Node is a .d.ts file
Let = 1 << 13, // Variable declaration
Const = 1 << 14, // Variable declaration
OctalLiteral = 1 << 15, // Octal numeric literal
Namespace = 1 << 16, // Namespace declaration
ExportContext = 1 << 17, // Export context (initialized by binding)
ContainsThis = 1 << 18, // Interface contains references to "this"
HasImplicitReturn = 1 << 19, // If function implicitly returns on one of codepaths (initialized by binding)
HasExplicitReturn = 1 << 20, // If function has explicit reachable return on one of codepaths (initialized by binding)
Modifier = Export | Ambient | Public | Private | Protected | Static | Abstract | Default | Async,
AccessibilityModifier = Public | Private | Protected,
BlockScoped = Let | Const
BlockScoped = Let | Const,

ReachabilityCheckFlags = HasImplicitReturn | HasExplicitReturn
}

/* @internal */
Expand Down Expand Up @@ -2089,6 +2092,10 @@ namespace ts {
experimentalDecorators?: boolean;
emitDecoratorMetadata?: boolean;
moduleResolution?: ModuleResolutionKind;
allowUnusedLabels?: boolean;
allowUnreachableCode?: boolean;
noImplicitReturns?: boolean;
noFallthroughCasesInSwitch?: boolean;
/* @internal */ stripInternal?: boolean;

// Skip checking lib.d.ts to help speed up tests.
Expand Down
39 changes: 20 additions & 19 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -622,25 +622,26 @@ namespace ts {
}

export function isFunctionLike(node: Node): node is FunctionLikeDeclaration {
if (node) {
switch (node.kind) {
case SyntaxKind.Constructor:
case SyntaxKind.FunctionExpression:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.ArrowFunction:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.CallSignature:
case SyntaxKind.ConstructSignature:
case SyntaxKind.IndexSignature:
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructorType:
return true;
}
return node && isFunctionLikeKind(node.kind);
}

export function isFunctionLikeKind(kind: SyntaxKind): boolean {
switch (kind) {
case SyntaxKind.Constructor:
case SyntaxKind.FunctionExpression:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.ArrowFunction:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.CallSignature:
case SyntaxKind.ConstructSignature:
case SyntaxKind.IndexSignature:
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructorType:
return true;
}
return false;
}

export function introducesArgumentsExoticObject(node: Node) {
Expand Down Expand Up @@ -1212,7 +1213,7 @@ namespace ts {
case SyntaxKind.LabeledStatement:
case SyntaxKind.ReturnStatement:
case SyntaxKind.SwitchStatement:
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops!

case SyntaxKind.ThrowKeyword:
case SyntaxKind.ThrowStatement:
case SyntaxKind.TryStatement:
case SyntaxKind.VariableStatement:
case SyntaxKind.WhileStatement:
Expand Down
5 changes: 1 addition & 4 deletions src/services/breakpoints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ namespace ts.BreakpointResolver {
// fall through.

case SyntaxKind.CatchClause:
return spanInNode(lastOrUndefined((<Block>node.parent).statements));;
return spanInNode(lastOrUndefined((<Block>node.parent).statements));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah! Was this reported as unreachable? Awesome.


case SyntaxKind.CaseBlock:
// breakpoint in last statement of the last clause
Expand Down Expand Up @@ -493,9 +493,6 @@ namespace ts.BreakpointResolver {
default:
return spanInNode(node.parent);
}

// Default to parent node
return spanInNode(node.parent);
}

function spanInColonToken(node: Node): TextSpan {
Expand Down
1 change: 0 additions & 1 deletion src/services/formatting/smartIndenter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,6 @@ namespace ts.formatting {
return node;
}
}
return node;
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/services/navigateTo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ namespace ts.NavigateTo {
let patternMatcher = createPatternMatcher(searchValue);
let rawItems: RawNavigateToItem[] = [];

// This means "compare in a case insensitive manner."
let baseSensitivity: Intl.CollatorOptions = { sensitivity: "base" };

// Search the declarations in all files and output matched NavigateToItem into array of NavigateToItem[]
forEach(program.getSourceFiles(), sourceFile => {
cancellationToken.throwIfCancellationRequested();
Expand Down Expand Up @@ -162,8 +165,6 @@ namespace ts.NavigateTo {
return bestMatchKind;
}

// This means "compare in a case insensitive manner."
let baseSensitivity: Intl.CollatorOptions = { sensitivity: "base" };
function compareNavigateToItems(i1: RawNavigateToItem, i2: RawNavigateToItem) {
// TODO(cyrusn): get the gamut of comparisons that VS already uses here.
// Right now we just sort by kind first, and then by name of the item.
Expand Down
Loading