[CLOSED] Allow findAllMatchingFunctionsInText to iterate over array-like objects (#1390)

[core-ai-bot]

Issue by jbalsas
Saturday Dec 08, 2012 at 23:07 GMT
Originally opened as adobe/brackets#2317

---

This is a possible fix for #1390 where no results appear in the Go to Definition dialog.

The root cause is that in this case, there is a .prototype.length = function () which turns the allFunctions object in findAllMatchingFunctionsInText into an array-like object, and due to the length property it fails to iterate using $.each.

The fix substitutes the $.each for a for..in loop.

---

jbalsas included the following code: https://github.com/adobe/brackets/pull/2317/commits

[core-ai-bot]

Comment by dangoor
Thursday Dec 13, 2012 at 22:49 GMT

---

Your fix worked fine for me when I fired it up (thanks!)

As someone looking at this code for the first time, I found "key" to be not very descriptive (same goes for index in the original code).

A little renaming and Crockford-approved function creating yielded this:

    /**
     * Finds all instances of the specified functionName in "text".
     * Returns an Array of Objects with start and end properties.
     *
     *`@`param text {!String} JS text to search
     *`@`param searchName {!String} function name to search for
     *`@`return {Array.<{offset:number, functionName:string}>}
     *      Array of objects containing the start offset for each matched function name.
     */
    function findAllMatchingFunctionsInText(text, searchName) {
        var allFunctions = _findAllFunctionsInText(text);
        var result = [];
        var lines = text.split("\n");
        var functionName;

        function makeAddFunction(functionName) {
            return function (funcEntry) {
                var endOffset = _getFunctionEndOffset(text, funcEntry.offsetStart);
                result.push({
                    name: functionName,
                    lineStart: StringUtils.offsetToLineNum(lines, funcEntry.offsetStart),
                    lineEnd: StringUtils.offsetToLineNum(lines, endOffset)
                });
            };
        }

        for (functionName in allFunctions) {
            if (allFunctions.hasOwnProperty(functionName)) {
                if (functionName === searchName || searchName === "*") {
                    var addFunctionEntry = makeAddFunction(functionName);
                    allFunctions[functionName].forEach(addFunctionEntry);
                }
            }
        }

        return result;
    }

How does that look to you? (or anyone else reading this, for that matter?)

[core-ai-bot]

Comment by jbalsas
Friday Dec 14, 2012 at 11:47 GMT

---

@dangoor That does look better, thanks! I'll try to add your suggestions and push the changes later today.

BTW, congrats, and welcome! ;)

[core-ai-bot]

Comment by dangoor
Friday Dec 14, 2012 at 14:10 GMT

---

Thanks!

[core-ai-bot]

Comment by jbalsas
Friday Dec 14, 2012 at 16:34 GMT

---

Changes pushed!

[core-ai-bot]

Comment by peterflynn
Saturday Dec 15, 2012 at 04:04 GMT

---

Are there any other places this bug could occur? It seems like $.each() is unsafe for iterating general string->* maps anywhere because of this API "feature." I see a bunch of other places where we use $.each() on objects and thus could conceivably get bit by this: QuickOpen.multiFieldSort(), PreferenceStorage.setAllValues(), FileIndexManager, and PerfUtils. Maybe we should have a shared utility function for enumerating object keys, and fix all these places at once? (There's already a CollectionUtils module where we could put it).

[core-ai-bot]

Comment by jbalsas
Saturday Dec 15, 2012 at 14:38 GMT

---

I considered it, but wasn't sure how much you'd want to refactor the existing calls...

What would you expect for this? CollectionUtils.forEach(object, fn(index, value)) to directly substitute the $.each call? We may want to use Object.keys to iterate through the object own properties directly.

I guess, while we're at it, we'll cover several cases from adobe/brackets#1098, so we could check whatever's missing after it.

[core-ai-bot]

Comment by jbalsas
Tuesday Dec 18, 2012 at 16:41 GMT

---

@dangoor@peterflynn I've refactored the loop into CollectionUtils.forEach, and updated JSUtils to use it instead of the old $.each.

I've checked, and there are plenty of references to $.each, so we may want to get this right first, and then use #1098 to clear the rest.

[core-ai-bot]

Comment by dangoor
Friday Dec 21, 2012 at 20:18 GMT

---

Sorry for the delay in getting back to you here. I like this revision, thanks for making the adjustments. I'll be merging this soon.

I made a couple of tweaks to the final landing. The most notable is that I swapped the arguments passed to the callback of CollectionUtils.forEach (it's now value, key). I thought this would be good for two reasons:

1. it's more consistent with the Array.forEach (value, index)
2. it's also consistent with underscore.each

(other than that, I added a change to the test suite that demonstrates the bug in #1390, though the JSUtils tests all pass with this code change)