Fixed find all references for private identifiers.

[dragomirtitian]

cc: @mheiber
Fixes #35883

[mheiber]

Thanks so much for fixing this! I think it would be nice to have a fourslash test for this, similar to the go-to-definition tests.

[dragomirtitian]

@mheiber wops forgot to commit the file (fourslash tests is the way I tested it first 😊), fixed now. Also missed the --light=false flag 😒

[DanielRosenwasser]

Holy crap that was fast.

@typescript-bot pack this

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at e617035. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/58750/artifacts?artifactName=tgz&fileId=5E66E04217EF14986D569FB1808D108536FD94CF62AEB30DA971C708F220048602&fileName=/typescript-3.8.0-insiders.20191228.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[DanielRosenwasser]

For some reason, find-all-refs at the end of the private name is failing.

class C {
  #foo/*findAllRefs1*/: string;

  blah() {
    this.#foo/*findAllRefs2*/;
  }
}

[dragomirtitian]

@DanielRosenwasser Fixed. Figuring out how to write the fourslash test.. 30 min.. figuring out the bug 10 min ... 😅

I fear there will be a lot more bugs relating to identifier | private_identifier in the code-base though (not just find references). They are very easy to miss 😞

[mheiber]

@dragomirtitian , re

I fear there will be a lot more bugs relating to identifier | private_identifier in the code-base though (not just find references). They are very easy to miss 😞.

I agree it's hard to catch all the cases where private identifiers need to be considered. If you have ideas for how to refactor so that the type system can help catch more of these cases, I'd be interested in trying the ideas out.

[mheiber]

The "privateProperties" in the test name may be confusing, given that we have private modifier and ES private fields (#).

Other ES private field tests have privateName in the name. I propose using privateName here for consistency, and perhaps a future PR to change the tests to esPrivate or hashPrivate or privateIdentifier or whatever works best.

[dragomirtitian]

Was not aware of the convention. Renamed the test file.

[mheiber]

Re Daniel's comment:

For some reason, find-all-refs at the end of the private name is failing.

class C {
  #foo/*findAllRefs1*/: string;

  blah() {
    this.#foo/*findAllRefs2*/;
  }
}

fwiw, I built the lang server from this PR locally and did some manual testing. I'm seeing public and private fields behave the same way when I right-click the end of a name in VS Code. Building on Daniel's example:

export default 3;

class C {
  #foo: string;
  bar: number;

  blah() {
    this.#foo/*loc1*/;
    this.bar/*loc2*/;
  }
}

• Right-clicking the final o in #foo and selecting FindAllReferences causes the references to show up.
• Right-clicking the r in bar and selecting FindAllReferences causes the references to show up

[dragomirtitian]

@mheiber Your last comment is not clear to me. Is there something that does not work in the current PR? Daniel reported the bug previously, but I since fixed it (I amended the original commit so you can't really see the fix separately, it was caused my a missing || privateIdentifier in getTouchingPropertyName)

[dragomirtitian]

I am unsure why the tests CI/build fails for node12. I tried running gulp runtests-parallel --light=false locally with node 12.6 and it succeeded without any issues 😕

[mheiber]

3 failing tests due to timeouts. Seems spurious to me, fwiw:

         fourslash-server tests
           tests/cases/fourslash/server/ngProxy2.ts
             fourslash-server test ngProxy2.ts runs correctly:
     Error: Timeout of 40000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
      at processImmediate (internal/timers.js:439:21)

[DanielRosenwasser]

@typescript-bot pack this

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at ace8822. You can monitor the build here. It should now contribute to this PR's status checks.

[DanielRosenwasser]

Just as a heads up, I think you implemented the test correctly (especially since you want to make sure that private names from other classes don't get picked up), but in the future, I believe there are other helpers to make sure that all ranges in the test provide the same results to find-all-references.

[typescript-bot]

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/59109/artifacts?artifactName=tgz&fileId=E5EAE627031F6656553838DC38B26BB76E995AE71D29726B92BDBE3749E6ABA602&fileName=/typescript-3.8.0-insiders.20200102.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.