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

Add support to report errors in js files #14496

Merged
merged 29 commits into from
Mar 28, 2017
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
91571f0
Add support for handeling .js file correctelly in fixAddMissingMember…
mhegazy Feb 23, 2017
0b1fff7
Add `--checkJsFiles`
mhegazy Jan 7, 2017
9f0c5ce
Add support for `//@check` directives
mhegazy Jan 7, 2017
0b247b1
Add tests
mhegazy Mar 6, 2017
1f9bb69
Add --noEmit to tests
mhegazy Mar 7, 2017
b015c1d
Allow @check directives to switch on/off checking in a file
mhegazy Mar 7, 2017
9305d4d
Change flag name to `checkJs`
mhegazy Mar 7, 2017
fb218b7
Error if `--checkJs` is used without `--allowJs`
mhegazy Mar 7, 2017
e9f8214
Code review comments
mhegazy Mar 8, 2017
a202fa4
add es6 to buildProtocol
mhegazy Mar 9, 2017
3d03f8d
Merge branch 'fixBuildBreak' into checkJSFiles
mhegazy Mar 9, 2017
fe7719f
Disable check diagnostics per line
mhegazy Mar 8, 2017
706acdf
Add quick fix to disable error checking in a .js file
mhegazy Mar 7, 2017
13e80b9
Fix building webTestServer
mhegazy Mar 7, 2017
936a91d
Add comment
mhegazy Mar 10, 2017
cc6affa
Merge remote-tracking branch 'origin/updateCodeFixForAddMissingMember…
mhegazy Mar 14, 2017
6e86596
Add debugging utilities
mhegazy Mar 14, 2017
fd9fb8f
Support static properties
mhegazy Mar 14, 2017
509b2dc
Add disableJsDiagnostics codefixes to harnes
mhegazy Mar 14, 2017
1fbbead
Merge pull request #14568 from Microsoft/checkJSFiles_QuickFixes
mhegazy Mar 14, 2017
7980629
Code review comments
mhegazy Mar 15, 2017
0dac29f
Merge branch 'master' into checkJSFiles
mhegazy Mar 15, 2017
3b57b5d
Refactor checking for checkJs value in a common helper
mhegazy Mar 15, 2017
e408cad
Merge branch 'master' into checkJSFiles
mhegazy Mar 22, 2017
db6c969
Change ingore diagonstic comment to `// @ts-ignore`
mhegazy Mar 22, 2017
3378f5c
Merge branch 'master' into checkJSFiles
mhegazy Mar 27, 2017
e630ab1
Report semantic errors for JS files if checkJs is enabled
mhegazy Mar 27, 2017
0637f24
Merge remote-tracking branch 'origin/master' into checkJSFiles
mhegazy Mar 28, 2017
8ea9617
Merge remote-tracking branch 'origin/master' into checkJSFiles
mhegazy Mar 28, 2017
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
2 changes: 1 addition & 1 deletion Jakefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,7 @@ task("generate-code-coverage", ["tests", builtLocalDirectory], function () {
// Browser tests
var nodeServerOutFile = "tests/webTestServer.js";
var nodeServerInFile = "tests/webTestServer.ts";
compileFile(nodeServerOutFile, [nodeServerInFile], [builtLocalDirectory, tscFile], [], /*useBuiltCompiler:*/ true, { noOutFile: true });
compileFile(nodeServerOutFile, [nodeServerInFile], [builtLocalDirectory, tscFile], [], /*useBuiltCompiler:*/ true, { noOutFile: true, lib: "es6" });

desc("Runs browserify on run.js to produce a file suitable for running tests in the browser");
task("browserify", ["tests", builtLocalDirectory, nodeServerOutFile], function() {
Expand Down
6 changes: 6 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,12 @@ namespace ts {
name: "plugin",
type: "object"
}
},
{
name: "checkJs",
type: "boolean",
experimental: true,
description: Diagnostics.Report_errors_in_js_files
}
];

Expand Down
24 changes: 23 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
{
"Unterminated string literal.": {
"category": "Error",
"code": 1002
Expand Down Expand Up @@ -3355,12 +3355,34 @@
"category": "Message",
"code": 90017
},
"Disable checking for this file.": {
"category": "Message",
"code": 90018
},
"Suppress this error message.": {
"category": "Message",
"code": 90019
},
"Initialize property '{0}' in the constructor.": {
"category": "Message",
"code": 90020
},
"Initialize static property '{0}'.": {
"category": "Message",
"code": 90021
},


"Octal literal types must use ES2015 syntax. Use the syntax '{0}'.": {
"category": "Error",
"code": 8017
},
"Octal literals are not allowed in enums members initializer. Use the syntax '{0}'.": {
"category": "Error",
"code": 8018
},
"Report errors in .js files.": {
"category": "Message",
"code": 8019
}
}
12 changes: 12 additions & 0 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5817,6 +5817,7 @@ namespace ts {
const typeReferenceDirectives: FileReference[] = [];
const amdDependencies: { path: string; name: string }[] = [];
let amdModuleName: string;
let checkJsDirective: CheckJsDirective = undefined;

// Keep scanning all the leading trivia in the file until we get to something that
// isn't trivia. Any single line comment will be analyzed to see if it is a
Expand Down Expand Up @@ -5878,13 +5879,24 @@ namespace ts {
amdDependencies.push(amdDependency);
}
}

const checkJsDirectiveRegEx = /^\/\/\/?\s*(@ts-check|@ts-nocheck)\s*$/gim;
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend you pull this out of the function and remove the g option. g is only really useful if you want to preserve lastIndex when using a regular expression in a loop. Also I would remove the m option as it should be unnecessary.

Consider this instead:

const checkJsDirectiveRegEx = /^\/\/\/?\s*@ts-(no)?check\s*$/i;
...
const checkJsDirectiveMatchResult = checkJsDirectiveRegEx.exec(comment);
if (checkJsDirectiveMatchResult) {
  checkJsDirective = {
    enabled: !!checkJsDirectiveMatchResult[1],
    end: range.end,
    pos: range.pos
  };
}

Capturing only the no allows you to avoid the additional case-insensitive string comparison.

const checkJsDirectiveMatchResult = checkJsDirectiveRegEx.exec(comment);
if (checkJsDirectiveMatchResult) {
checkJsDirective = {
enabled: compareStrings(checkJsDirectiveMatchResult[1], "@ts-check", /*ignoreCase*/ true) === Comparison.EqualTo,
end: range.end,
pos: range.pos
};
}
}
}

sourceFile.referencedFiles = referencedFiles;
sourceFile.typeReferenceDirectives = typeReferenceDirectives;
sourceFile.amdDependencies = amdDependencies;
sourceFile.moduleName = amdModuleName;
sourceFile.checkJsDirective = checkJsDirective;
}

function setExternalModuleIndicator(sourceFile: SourceFile) {
Expand Down
41 changes: 36 additions & 5 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace ts {
const emptyArray: any[] = [];
const suppressDiagnosticCommentRegEx = /(^\s*$)|(^\s*\/\/\/?\s*(@ts-suppress)?)/;

export function findConfigFile(searchPath: string, fileExists: (fileName: string) => boolean, configName = "tsconfig.json"): string {
while (true) {
Expand Down Expand Up @@ -916,17 +917,43 @@ namespace ts {

Debug.assert(!!sourceFile.bindDiagnostics);
const bindDiagnostics = sourceFile.bindDiagnostics;
// For JavaScript files, we don't want to report semantic errors.
// Instead, we'll report errors for using TypeScript-only constructs from within a
// JavaScript file when we get syntactic diagnostics for the file.
const checkDiagnostics = isSourceFileJavaScript(sourceFile) ? [] : typeChecker.getDiagnostics(sourceFile, cancellationToken);
// For JavaScript files, we don't want to report semantic errors unless explicitly requested.
const includeCheckDiagnostics = !isSourceFileJavaScript(sourceFile) ||
(sourceFile.checkJsDirective ? sourceFile.checkJsDirective.enabled : options.checkJs);
const checkDiagnostics = includeCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : [];
const fileProcessingDiagnosticsInFile = fileProcessingDiagnostics.getDiagnostics(sourceFile.fileName);
const programDiagnosticsInFile = programDiagnostics.getDiagnostics(sourceFile.fileName);

return bindDiagnostics.concat(checkDiagnostics, fileProcessingDiagnosticsInFile, programDiagnosticsInFile);
const diagnostics = bindDiagnostics.concat(checkDiagnostics, fileProcessingDiagnosticsInFile, programDiagnosticsInFile);
return isSourceFileJavaScript(sourceFile)
? filter(diagnostics, shouldReportDiagnostic)
: diagnostics;
});
}

/**
* Skip errors if previous line start with '// @ts-suppress' comment, not counting non-empty non-comment lines
*/
function shouldReportDiagnostic(diagnostic: Diagnostic) {
const { file, start } = diagnostic;
const lineStarts = getLineStarts(file);
let { line } = computeLineAndCharacterOfPosition(lineStarts, start);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • can line have more than one error where only one should be suppresed
  • seems that ts-suppress should be located immediately before the line to be suppressed so if user will put a comment between ts-suppress and target line it will stop working. Is it intended?

while (line > 0) {
const previousLineText = file.text.slice(lineStarts[line - 1], lineStarts[line]);
const result = suppressDiagnosticCommentRegEx.exec(previousLineText);
if (!result) {
// non-empty line
return true;
}
if (result[3]) {
// @ts-suppress
return false;
}
line--;
}
return true;
}

function getJavaScriptSyntacticDiagnosticsForFile(sourceFile: SourceFile): Diagnostic[] {
return runWithCancellationToken(() => {
const diagnostics: Diagnostic[] = [];
Expand Down Expand Up @@ -1727,6 +1754,10 @@ namespace ts {
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Option_0_cannot_be_specified_with_option_1, "allowJs", "declaration"));
}

if (options.checkJs && !options.allowJs) {
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "checkJs", "allowJs"));
}

if (options.emitDecoratorMetadata &&
!options.experimentalDecorators) {
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "emitDecoratorMetadata", "experimentalDecorators"));
Expand Down
6 changes: 6 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1940,6 +1940,10 @@ namespace ts {
fileName: string;
}

export interface CheckJsDirective extends TextRange {
enabled: boolean;
}

export type CommentKind = SyntaxKind.SingleLineCommentTrivia | SyntaxKind.MultiLineCommentTrivia;

export interface CommentRange extends TextRange {
Expand Down Expand Up @@ -2282,6 +2286,7 @@ namespace ts {
/* @internal */ moduleAugmentations: LiteralExpression[];
/* @internal */ patternAmbientModules?: PatternAmbientModule[];
/* @internal */ ambientModuleNames: string[];
/* @internal */ checkJsDirective: CheckJsDirective | undefined;
}

export interface Bundle extends Node {
Expand Down Expand Up @@ -3323,6 +3328,7 @@ namespace ts {
alwaysStrict?: boolean; // Always combine with strict property
baseUrl?: string;
charset?: string;
checkJs?: boolean;
/* @internal */ configFilePath?: string;
declaration?: boolean;
declarationDir?: string;
Expand Down
11 changes: 10 additions & 1 deletion src/harness/fourslash.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//
//
// Copyright (c) Microsoft Corporation. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -2554,6 +2554,11 @@ namespace FourSlash {
}
}

public printAvailableCodeFixes() {
const codeFixes = this.getCodeFixActions(this.activeFile.fileName);
Harness.IO.log(stringify(codeFixes));
}

// Get the text of the entire line the caret is currently at
private getCurrentLineContent() {
const text = this.getFileContent(this.activeFile.fileName);
Expand Down Expand Up @@ -3742,6 +3747,10 @@ namespace FourSlashInterface {
this.state.printCompletionListMembers();
}

public printAvailableCodeFixes() {
this.state.printAvailableCodeFixes();
}

public printBreakpointLocation(pos: number) {
this.state.printBreakpointLocation(pos);
}
Expand Down
2 changes: 1 addition & 1 deletion src/harness/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ namespace Utils {
for (const childName in node) {
if (childName === "parent" || childName === "nextContainer" || childName === "modifiers" || childName === "externalModuleIndicator" ||
// for now ignore jsdoc comments
childName === "jsDocComment") {
childName === "jsDocComment" || childName === "checkJsDirective") {
continue;
}
const child = (<any>node)[childName];
Expand Down
3 changes: 2 additions & 1 deletion src/harness/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
{
"extends": "../tsconfig-base",
"compilerOptions": {
"removeComments": false,
Expand Down Expand Up @@ -81,6 +81,7 @@
"../services/codefixes/helpers.ts",
"../services/codefixes/importFixes.ts",
"../services/codefixes/unusedIdentifierFixes.ts",
"../services/codefixes/disableJsDiagnostics.ts",

"harness.ts",
"sourceMapRecorder.ts",
Expand Down
74 changes: 74 additions & 0 deletions src/services/codefixes/disableJsDiagnostics.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/* @internal */
namespace ts.codefix {
registerCodeFix({
errorCodes: getApplicableDiagnosticCodes(),
getCodeActions: getDisableJsDiagnosticsCodeActions
});

function getApplicableDiagnosticCodes(): number[] {
const allDiagnostcs = <MapLike<DiagnosticMessage>>Diagnostics;
return Object.keys(allDiagnostcs)
.filter(d => allDiagnostcs[d] && allDiagnostcs[d].category === DiagnosticCategory.Error)
.map(d => allDiagnostcs[d].code);
}

function shouldCheckJsFile(sourceFile: SourceFile, compilerOptions: CompilerOptions) {
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 the same snipped with this one. Can we share this?

return sourceFile.checkJsDirective ? sourceFile.checkJsDirective.enabled : compilerOptions.checkJs;
}

function getSuppressCommentLocationForLocation(sourceFile: SourceFile, position: number, newLineCharacter: string) {
let { line } = getLineAndCharacterOfPosition(sourceFile, position);
const lineStartPosition = getStartPositionOfLine(line, sourceFile);
const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition);

// First try to see if we can put the '// @ts-suppress' on the previous line.
// We need to make sure that we are not in the middle of a string literal or a comment.
// We also want to check if the previous line holds a comment for a node on the next line
// if so, we do not want to separate the node from its comment if we can.
if (!isInComment(sourceFile, startPosition) && !isInString(sourceFile, startPosition) && !isInTemplateString(sourceFile, startPosition)) {
const token = getTouchingToken(sourceFile, startPosition);
const tokenLeadingCommnets = getLeadingCommentRangesOfNode(token, sourceFile);
if (!tokenLeadingCommnets || !tokenLeadingCommnets.length || tokenLeadingCommnets[0].pos >= startPosition) {
return {
span: { start: startPosition, length: 0 },
newText: `// @ts-suppress${newLineCharacter}`
};
}
}

// If all fails, add an extra new line immediatlly before the error span.
return {
span: { start: position, length: 0 },
newText: `${position === startPosition ? "" : newLineCharacter}// @ts-suppress${newLineCharacter}`
};
}

function getDisableJsDiagnosticsCodeActions(context: CodeFixContext): CodeAction[] | undefined {
const { sourceFile, program, newLineCharacter, span } = context;

if (!isInJavaScriptFile(sourceFile) || !shouldCheckJsFile(sourceFile, program.getCompilerOptions())) {
return undefined;
}

return [{
description: getLocaleSpecificMessage(Diagnostics.Suppress_this_error_message),
changes: [{
fileName: sourceFile.fileName,
textChanges: [getSuppressCommentLocationForLocation(sourceFile, span.start, newLineCharacter)]
}]
},
{
description: getLocaleSpecificMessage(Diagnostics.Disable_checking_for_this_file),
changes: [{
fileName: sourceFile.fileName,
textChanges: [{
span: {
start: sourceFile.checkJsDirective ? sourceFile.checkJsDirective.pos : 0,
length: sourceFile.checkJsDirective ? sourceFile.checkJsDirective.end - sourceFile.checkJsDirective.pos : 0
},
newText: `// @ts-nocheck${newLineCharacter}`
}]
}]
}];
}
}
Loading