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

Allow findAllMatchingFunctionsInText to iterate over array-like objects (#1390) #2317

Merged
merged 3 commits into from
Dec 21, 2012
Merged

Conversation

jbalsas
Copy link
Contributor

@jbalsas jbalsas commented Dec 8, 2012

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.

function addFunctionEntry(funcEntry) {
var endOffset = _getFunctionEndOffset(text, funcEntry.offsetStart);
result.push({
name: key,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using key like that seems dangerous but JSLint complains about creating functions inside loops otherwise. Is there a better way to do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Creating functions in a loop is certainly legal JavaScript, so I thought I'd Google to see what the Crockford-approved approach is. I was not let down

@ghost ghost assigned peterflynn Dec 10, 2012
@dangoor
Copy link
Contributor

dangoor commented Dec 13, 2012

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?)

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 14, 2012

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

BTW, congrats, and welcome! ;)

@dangoor
Copy link
Contributor

dangoor commented Dec 14, 2012

Thanks!

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 14, 2012

Changes pushed!

@peterflynn
Copy link
Member

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).

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 15, 2012

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 #1098, so we could check whatever's missing after it.

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 18, 2012

@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.

@ghost ghost assigned dangoor Dec 21, 2012
@dangoor
Copy link
Contributor

dangoor commented Dec 21, 2012

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)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants