Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Remember files that timed out in Tern #8269

Merged
merged 2 commits into from
Jul 16, 2014
Merged

Remember files that timed out in Tern #8269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71a6dfc
remember files that timed out in tern
redmunds Jun 27, 2014
f13b271
remove defaultExclusions preference
redmunds Jul 16, 2014
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: 2 additions & 0 deletions src/extensions/default/JavaScriptCodeHints/MessageIds.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ define(function (require, exports, module) {
TERN_PRIME_PUMP_MSG = "PrimePump",
TERN_GET_GUESSES_MSG = "GetGuesses",
TERN_WORKER_READY = "WorkerReady",
TERN_INFERENCE_TIMEDOUT = "InferenceTimedOut",
SET_CONFIG = "SetConfig";

// Message parameter constants
Expand All @@ -58,6 +59,7 @@ define(function (require, exports, module) {
exports.TERN_FILE_INFO_TYPE_PART = TERN_FILE_INFO_TYPE_PART;
exports.TERN_FILE_INFO_TYPE_FULL = TERN_FILE_INFO_TYPE_FULL;
exports.TERN_FILE_INFO_TYPE_EMPTY = TERN_FILE_INFO_TYPE_EMPTY;
exports.TERN_INFERENCE_TIMEDOUT = TERN_INFERENCE_TIMEDOUT;
exports.SET_CONFIG = SET_CONFIG;
});

Expand Down
156 changes: 124 additions & 32 deletions src/extensions/default/JavaScriptCodeHints/ScopeManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,22 @@ define(function (require, exports, module) {

var _ = brackets.getModule("thirdparty/lodash");

var DocumentManager = brackets.getModule("document/DocumentManager"),
LanguageManager = brackets.getModule("language/LanguageManager"),
ProjectManager = brackets.getModule("project/ProjectManager"),
var CodeMirror = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror"),
DefaultDialogs = brackets.getModule("widgets/DefaultDialogs"),
Dialogs = brackets.getModule("widgets/Dialogs"),
DocumentManager = brackets.getModule("document/DocumentManager"),
EditorManager = brackets.getModule("editor/EditorManager"),
ExtensionUtils = brackets.getModule("utils/ExtensionUtils"),
FileSystem = brackets.getModule("filesystem/FileSystem"),
FileUtils = brackets.getModule("file/FileUtils"),
CodeMirror = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror"),
PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
globmatch = brackets.getModule("thirdparty/globmatch"),
HintUtils = require("HintUtils"),
LanguageManager = brackets.getModule("language/LanguageManager"),
PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
ProjectManager = brackets.getModule("project/ProjectManager"),
Strings = brackets.getModule("strings"),
StringUtils = brackets.getModule("utils/StringUtils");

var HintUtils = require("HintUtils"),
MessageIds = require("MessageIds"),
Preferences = require("Preferences");

Expand All @@ -62,7 +68,6 @@ define(function (require, exports, module) {

var MAX_HINTS = 30, // how often to reset the tern server
LARGE_LINE_CHANGE = 100,
LARGE_LINE_COUNT = 2000,
OFFSET_ZERO = {line: 0, ch: 0};

var config = {};
Expand Down Expand Up @@ -197,6 +202,36 @@ define(function (require, exports, module) {
return excludes.test(testPath);
}

/**
* Test if the file path is in current editor
*
* @param {string} filePath file path to test for exclusion.
* @return {boolean} true if in editor, false otherwise.
*/
function isFileBeingEdited(filePath) {
var currentEditor = EditorManager.getFocusedEditor(),
currentDoc = currentEditor && currentEditor.document;

return (currentDoc && currentDoc.file.fullPath === filePath);
}

/**
* Test if the file path is an internal exclusion.
*
* @param {string} path file path to test for exclusion.
* @return {boolean} true if excluded, false otherwise.
*/
function isFileExcludedInternal(path) {
// The detectedExclusions are files detected to be troublesome with current versions of Tern.
// detectedExclusions is an array of full paths.
var detectedExclusions = PreferencesManager.get("jscodehints.detectedExclusions") || [];
if (detectedExclusions && detectedExclusions.indexOf(path) !== -1) {
return true;
}

return false;
}

/**
* Test if the file should be excluded from analysis.
*
Expand All @@ -218,18 +253,16 @@ define(function (require, exports, module) {
return true;
}

var defaultExclusions = PreferencesManager.get("jscodehints.defaultExclusions");
if (isFileExcludedInternal(file.fullPath)) {
return true;
}

// The defaultExclusions are the ones we ship with Brackets to filter out files that we know
// to be troublesome with current versions of Tern. They can be overridden with a .brackets.json
// file in your project. defaultExclusions is an array of globs.
return defaultExclusions &&
_.isArray(defaultExclusions) &&
_.some(defaultExclusions, _.partial(globmatch, file.fullPath));
return false;
}

/**
* Add a pending request waiting for the tern-worker to complete.
* If file is a detected exclusion, then reject request.
*
* @param {string} file - the name of the file
* @param {{line: number, ch: number}} offset - the offset into the file the request is for
Expand All @@ -240,6 +273,12 @@ define(function (require, exports, module) {
var requests,
key = file + "@" + offset.line + "@" + offset.ch,
$deferredRequest;

// Reject detected exclusions
if (isFileExcludedInternal(file)) {
return (new $.Deferred()).reject().promise();
}

if (_.has(pendingTernRequests, key)) {
requests = pendingTernRequests[key];
} else {
Expand All @@ -250,7 +289,7 @@ define(function (require, exports, module) {
if (_.has(requests, type)) {
$deferredRequest = requests[type];
} else {
requests[type] = $deferredRequest = $.Deferred();
requests[type] = $deferredRequest = new $.Deferred();
}
return $deferredRequest.promise();
}
Expand Down Expand Up @@ -520,7 +559,7 @@ define(function (require, exports, module) {
result = {type: MessageIds.TERN_FILE_INFO_TYPE_EMPTY,
name: path,
text: ""};
} else if (!preventPartialUpdates && session.editor.lineCount() > LARGE_LINE_COUNT &&
} else if (!preventPartialUpdates &&
(documentChanges.to - documentChanges.from < LARGE_LINE_CHANGE) &&
documentChanges.from <= start.line &&
documentChanges.to > end.line) {
Expand Down Expand Up @@ -650,7 +689,7 @@ define(function (require, exports, module) {
* Handle the response from the tern web worker when
* it responds to the update file message.
*
* @param {{path:string, type: string}} response - the response from the worker
* @param {{path: string, type: string}} response - the response from the worker
*/
function handleUpdateFile(response) {

Expand All @@ -663,6 +702,49 @@ define(function (require, exports, module) {
}
}

/**
* Handle timed out inference
*
* @param {{path: string, type: string}} response - the response from the worker
*/
function handleTimedOut(response) {

var detectedExclusions = PreferencesManager.get("jscodehints.detectedExclusions") || [],
filePath = response.file;

// Don't exclude the file currently being edited
if (isFileBeingEdited(filePath)) {
return;
}

// Handle file that is already excluded
if (detectedExclusions.indexOf(filePath) !== -1) {
console.log("JavaScriptCodeHints.handleTimedOut: file already in detectedExclusions array timed out: " + filePath);
return;
}

// Save detected exclusion in project prefs so no further time is wasted on it
detectedExclusions.push(filePath);
PreferencesManager.set("jscodehints.detectedExclusions", detectedExclusions, { location: { scope: "project" } });

// Show informational dialog
Dialogs.showModalDialog(
DefaultDialogs.DIALOG_ID_INFO,
Strings.DETECTED_EXCLUSION_TITLE,
StringUtils.format(
Strings.DETECTED_EXCLUSION_INFO,
StringUtils.breakableUrl(filePath)
),
[
{
className : Dialogs.DIALOG_BTN_CLASS_PRIMARY,
id : Dialogs.DIALOG_BTN_OK,
text : Strings.OK
}
]
);
}

/**
* Encapsulate all the logic to talk to the worker thread. This will create
* a new instance of a TernWorker, which the rest of the hinting code can use to talk
Expand Down Expand Up @@ -777,7 +859,7 @@ define(function (require, exports, module) {
*/
function getDocText(filePath) {
if (!FileSystem.isAbsolutePath(filePath)) {
return new $.Deferred().reject();
return (new $.Deferred()).reject().promise();
}

var file = FileSystem.getFileForPath(filePath),
Expand Down Expand Up @@ -824,16 +906,18 @@ define(function (require, exports, module) {
}
});
}

getDocText(name).fail(function () {
getDocText(rootTernDir + name).fail(function () {
// check relative to project root
getDocText(projectRoot + name)
// last look for any files that end with the right path
// in the project
.fail(findNameInProject);

if (!isFileExcludedInternal(name)) {
getDocText(name).fail(function () {
getDocText(rootTernDir + name).fail(function () {
// check relative to project root
getDocText(projectRoot + name)
// last look for any files that end with the right path
// in the project
.fail(findNameInProject);
});
});
});
}
}

/**
Expand All @@ -847,7 +931,7 @@ define(function (require, exports, module) {
type : MessageIds.TERN_PRIME_PUMP_MSG,
path : path
});

return addPendingRequest(path, OFFSET_ZERO, MessageIds.TERN_PRIME_PUMP_MSG);
}

Expand Down Expand Up @@ -980,6 +1064,8 @@ define(function (require, exports, module) {
handleGetGuesses(response);
} else if (type === MessageIds.TERN_UPDATE_FILE_MSG) {
handleUpdateFile(response);
} else if (type === MessageIds.TERN_INFERENCE_TIMEDOUT) {
handleTimedOut(response);
} else if (type === MessageIds.TERN_WORKER_READY) {
workerDeferred.resolveWith(null, [_ternWorker]);
} else {
Expand Down Expand Up @@ -1011,11 +1097,17 @@ define(function (require, exports, module) {
env : ternEnvironment,
timeout : PreferencesManager.get("jscodehints.inferenceTimeout")
};

if (config.debug) {
console.debug("Sending message", msg);

if (worker) {
if (config.debug) {
console.debug("Sending message", msg);
}
worker.postMessage(msg);
} else {
if (config.debug) {
console.debug("Worker null. Cannot send message", msg);
}
}
worker.postMessage(msg);
});
rootTernDir = dir + "/";
}
Expand Down
4 changes: 2 additions & 2 deletions src/extensions/default/JavaScriptCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ define(function (require, exports, module) {
matcher = null, // string matcher for hints
ignoreChange; // can ignore next "change" event if true;

// Define the defaultExclusions which are files that are known to cause Tern to run out of control.
PreferencesManager.definePreference("jscodehints.defaultExclusions", "array", []);
// Define the detectedExclusions which are files that have been detected to cause Tern to run out of control.
PreferencesManager.definePreference("jscodehints.detectedExclusions", "array", []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Following up on the previous discussion, I think we would want these exclusions to show up in UI. That way, if the user has reason to believe that the automatically excluded files may now work correctly, they'd be able to remove them easily via the UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if that problematic library is no longer part of their project, they could remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be nice for user to be able to edit in UI, but I think it introduces a whole new concept of an exclusion list that is only shown for editing. As I tried to state earlier, the detectedExclusions list is always checked (so user cannot select or unselect this exclusion list) and any other user exclusion list is also checked.

I don't think it's worth trying to update UI for this special case. I think we should merge this PR as is (i.e. require user to manually edit prefs).

One other idea I had is to automatically remove any file being edited from detectedExclusions list. Presumably user is editing file to fix it, and if it fails again, it will be re-added to detectedExclusions list. While this sounds slick, it may be tricky to get this just right. But this doesn't handle case of a problematic file being updated outside of Brackets.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I was thinking was that there's just one exclusion list. We already need to add an exclusions preference to replace the one in the .jscodehints file. If we detect a problem, we could automatically add the problem file to the one, single exclusion list. If the user has another reason to exclude a file, they would just add the exclusion to that same list.

That said, the good thing about a detectedExclusions list is that the user is not left guessing how the file got there. Sticking with detectedExclusions makes sense to me from that perspective.


// This preference controls when Tern will time out when trying to understand files
PreferencesManager.definePreference("jscodehints.inferenceTimeout", "number", 5000);
Expand Down
6 changes: 5 additions & 1 deletion src/extensions/default/JavaScriptCodeHints/tern-worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ var config = {};
*/
function _reportError(e, file) {
if (e instanceof Infer.TimedOut) {
_log("Timeout during Tern processing of " + file);
// Post a message back to the main thread with timedout info
self.postMessage({
type: MessageIds.TERN_INFERENCE_TIMEDOUT,
file: file
});
} else {
_log("Error thrown in tern_worker:" + e.message + "\n" + e.stack);
}
Expand Down
2 changes: 2 additions & 0 deletions src/nls/root/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,8 @@ define({
"CMD_JUMPTO_DEFINITION" : "Jump to Definition",
"CMD_SHOW_PARAMETER_HINT" : "Show Parameter Hint",
"NO_ARGUMENTS" : "<no parameters>",
"DETECTED_EXCLUSION_TITLE" : "JavaScript File Inference Problem",
"DETECTED_EXCLUSION_INFO" : "Brackets ran into trouble processing:<br><br>{0}<br><br>This file will no longer be processed for code hints and jump to definition. To turn this back on, open <code>.brackets.json</code> in your project and remove the file from jscodehints.exclusions.",

// extensions/default/JSLint
"JSLINT_NAME" : "JSLint",
Expand Down