Skip to content

Inconsistency in getDocumentHighlights for import aliases #3688

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

Closed
leeavital opened this issue Jun 30, 2015 · 12 comments
Closed

Inconsistency in getDocumentHighlights for import aliases #3688

leeavital opened this issue Jun 30, 2015 · 12 comments
Assignees
Labels
API Relates to the public API for TypeScript Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@leeavital
Copy link

For the file:

import fs = require('fs');
import ws = fs.writeStream;

getDocumentHighlights will not find that the fs on line 1 is the same as the one on line 2.

But for the file:

import fs = require('fs');
var ws = fs.writeStream;

I wrote a small program to demonstrate the problem.

var ts = require('typescript');
var fs = require('fs');

var source = fs.readFileSync('test.ts').toString();
var sourceFile = ts.createSourceFile('test.ts', source, 1, false);

var host = {
  getCompilationSettings: function() {
      return {
          noResolve: true,
          target: ts.ScriptTarget.ES5
      };
  },
  getCurrentDirectory: function() {
    return '';
  },
  getDefaultLibFileName: function() {
    return "lib.d.ts"
  },
  getScriptFileNames: function() {
    return ['test.ts'];
  },
  getScriptIsOpen: function() {
    return true
  },
  getScriptVersion: function() {
    return "1"
  },
  getScriptSnapshot: function() {
    return {
      getLength: function() {
        return source.length;
      },
      getLineStartPositions: function () {
        return ts.computeLineStarts(source);
      },
      getText: function(start, end) {
        return source.substring(start, end);
      }
    };
  }
};

var documentRegistry = ts.createDocumentRegistry();
var languageServices = ts.createLanguageService(host, documentRegistry);

var importFS  = sourceFile.statements[0];
var highlights = languageServices.getDocumentHighlights('test.ts', importFS.name.pos + 1, ['test.ts']);
console.log(highlights[0].highlightSpans.length);

When test.ts contains the first example, it outputs 1. When test.ts contains the second example, it outputs 2.

@DanielRosenwasser
Copy link
Member

I assume this also means find-references and rename is also not working in this scenario.

@jkillian
Copy link

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();
}

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

I've been trying to figure out the root cause but haven't been able to yet. 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)

@DanielRosenwasser
Copy link
Member

@jkillian I'm not able to reproduce the error:

image

it's likely unrelated to the issue filed here, so if you're still seeing it, can you file a separate bug?

@DanielRosenwasser DanielRosenwasser changed the title Inconsistency in LanguageServices.getDocumentHighlights Inconsistency in getDocumentHighlights for import aliases Aug 31, 2015
@jkillian
Copy link

@DanielRosenwasser Created a new issue from my previous comment with a few details added: #4560

@jkillian
Copy link

@leeavital The issue comes from this code:

getScriptSnapshot: function() {
    return {
      getLength: function() {
        return source.length;
      },
      getLineStartPositions: function () {
        return ts.computeLineStarts(source);
      },
      getText: function(start, end) {
        return source.substring(start, end);
      }
    };
  }

getScriptSnapshot can request snapshots for files other than source, so only returning code form source causes things to happen incorrectly.

@DanielRosenwasser DanielRosenwasser added External Relates to another program, environment, or user action which we cannot control. and removed Bug A bug in TypeScript labels Sep 11, 2015
@jkillian
Copy link

My apologies, I think this is still a bug. I had assumed that our incorrect usage of the language services was the issue, but it doesn't seem like that's the case. FindReferences in Sublime doesn't mark both references. In addition, this fourslash test fails:

/// <reference path='fourslash.ts' />

////import [|fs|] = require('fs');
////import ws = [|fs|].writeStream;

let ranges = test.ranges()
for (let range of ranges) {
    goTo.position(range.start);

    verify.renameLocations(/*findInStrings*/ false, /*findInComments*/ false);
}

(related palantir/tslint#325)

@mhegazy mhegazy reopened this Sep 14, 2015
@mhegazy mhegazy added Bug A bug in TypeScript External Relates to another program, environment, or user action which we cannot control. API Relates to the public API for TypeScript and removed External Relates to another program, environment, or user action which we cannot control. Bug A bug in TypeScript labels Sep 14, 2015
@mhegazy mhegazy modified the milestones: TypeScript 1.7, TypeScript 1.8 Oct 9, 2015
@adidahiya
Copy link
Contributor

@DanielRosenwasser Any updates on this? Is the fix fairly involved?

@DanielRosenwasser
Copy link
Member

I'm honestly not sure, I'll try to find some time to work on this soon.

@DanielRosenwasser
Copy link
Member

I wonder if this is related to #5501 or #4970.

@mhegazy mhegazy assigned mhegazy and unassigned DanielRosenwasser Apr 25, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Jun 6, 2016

@jkillian is this still an issue? i believe it has been fixed in latest.

@jkillian
Copy link

jkillian commented Jun 6, 2016

Verified that this is fixed in the latest nightly, thanks! (palantir/tslint#1294) Feel free to close this issue

@mhegazy
Copy link
Contributor

mhegazy commented Jun 6, 2016

thanks!

@mhegazy mhegazy closed this as completed Jun 6, 2016
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jun 6, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Relates to the public API for TypeScript Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

5 participants