Possible incorrect getDocumentHighlights behavior with global lexically scoped variables

[jkillian]

There seem to be some related funny issues going on. When calling getDocumentHighlights on the position of the hello variable declaration in the following code snippet:

// Incorrect case:

class HelloWorld {
    constructor(public name: string) { }
    sayHello() {
        return `Hello, ${this.name}!`;
    }
};

let hello = new HelloWorld("TSLint");
hello.sayHello();

only the declaration is returned and not the usage on the next line. However, in the following code snippet, both uses are returned, correctly:

// Correct case:

class HelloWorld {
    constructor(public name: string) { }
    sayHello() {
        return `Hello, ${this.name}!`;
    }
};

if (true) {
    let hello = new HelloWorld("TSLint");
    hello.sayHello();
}

// Correct case (var vs. let):

class HelloWorld {
    constructor(public name: string) { }
    sayHello() {
        return `Hello, ${this.name}!`;
    }
};

var hello = new HelloWorld("TSLint");
hello.sayHello();

Also strangely, Sublime will correctly show both references in all three cases.

I've been trying to figure out the root cause but haven't been able to yet. It seems that the failure occurs specifically for lexically scoped variables declared at global scope, such as let declarations at global scope.

In case it helps, I did notice that in the getReferencesInNode function the array of possible positions contains exactly the two actual positions in both the incorrect and correct case. However, there is a difference when finding the relatedSymbol:

var referenceSymbol = typeChecker.getSymbolAtLocation(referenceLocation);
if (referenceSymbol) {
    var referenceSymbolDeclaration = referenceSymbol.valueDeclaration;
    var shorthandValueSymbol = typeChecker.getShorthandAssignmentValueSymbol(referenceSymbolDeclaration);
    var relatedSymbol = getRelatedSymbol(searchSymbols, referenceSymbol, referenceLocation);
    // NOTE (added for the Github issue):
    // relatedSymbol is undefined here in incorrect case, defined in the correct case
    if (relatedSymbol) {
        var referencedSymbol = getReferencedSymbol(relatedSymbol);
        referencedSymbol.references.push(getReferenceEntryFromNode(referenceLocation));
    }
    else if (!(referenceSymbol.flags & 67108864 /* Transient */) && searchSymbols.indexOf(shorthandValueSymbol) >= 0) {
        var referencedSymbol = getReferencedSymbol(shorthandValueSymbol);
        referencedSymbol.references.push(getReferenceEntryFromNode(referenceSymbolDeclaration.name));
    }
}

(Related to palantir/tslint#570)

[jkillian]

@DanielRosenwasser Figured out the problem I believe, In TSLint, we constructed a language service host object as such:

    export function createLanguageServiceHost(fileName: string, source: string) {
        const host: ts.LanguageServiceHost = {
            getCompilationSettings: () => Lint.createCompilerOptions(),
            getCurrentDirectory: () => "",
            getDefaultLibFileName: () => "lib.d.ts",
            getScriptFileNames: () => [fileName],
            getScriptSnapshot: () => ts.ScriptSnapshot.fromString(source),
            getScriptVersion: () => "1",
            log: (message) => { /* */ }
        };

        return host;
    }

The issue arises here:

getScriptSnapshot: () => ts.ScriptSnapshot.fromString(source),

When the language services seem requests a snapshot for the lib file, we were returning the source for the source file which is obviously non-sensical. Strangely enough, everything still worked fine in most cases, but not all cases (see above).

Do you have a suggestion on a good way to change this?

[DanielRosenwasser]

@mhegazy can check me on this, but consider just returning an empty string. That way there won't be any conflicting declarations. Let us know if that fixes the issue.

[jkillian]

Thanks! This change you suggested seems to fix the issues we were having (and all our tests pass with this change):

getScriptSnapshot: name => ts.ScriptSnapshot.fromString(name === fileName ? source : ""),

So this won't cause any issues with the language services? Seems a bit hacky and just want to double-check that it's a safe change to make

[mhegazy]

this looks good to me. an empty lib file should do the trick.

[mhegazy]

so is this issue resolved now?

[jkillian]

Yes, thanks for the help. #3688 should be resolved as well, it was caused by the same issue I believe.