Skip to content
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

Resolve only relative references in open files on syntax server #39476

Merged
merged 5 commits into from
Jul 16, 2020

Conversation

sheetalkamat
Copy link
Member

Resolve only relative module references in the open files

@sheetalkamat sheetalkamat force-pushed the syntaxOnlyResolutionsInOpenFiles branch from eb8a109 to 56fb305 Compare July 7, 2020 21:26
@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 7, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 56fb305. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 7, 2020

Looks like there's no packed build because of quote preference changes in a fourslash test.

Click for log
  71790 passing (8m)
  1 failing

  1) 
       
         fourslash tests
           tests/cases/fourslash/codeFixClassImplementInterfaceAutoImports.ts
             fourslash test codeFixClassImplementInterfaceAutoImports.ts runs correctly:
     Error: At line 1, col 0: Actual range text doesn't match expected text.

Expected:
import { Base } from './interface';
import A from './types1';
import { B, C, D } from './types2';

export class C implements Base {
    a: Readonly<A> & { kind: "a"; };
    b<T extends B = B>(p1: C): D<C> {
        throw new Error('Method not implemented.');
    }
}

Actual:
import { Base } from './interface';
import A from './types1';
import { B, C, D } from './types2';

export class C implements Base {
    a: Readonly<A> & { kind: 'a'; };
    b<T extends B = B>(p1: C): D<C> {
        throw new Error('Method not implemented.');
    }
}
      at TestState.raiseError (src/harness/fourslashImpl.ts:525:19)
      at TestState.verifyTextMatches (src/harness/fourslashImpl.ts:2659:22)
      at Object.runFourSlashTest (src/harness/fourslashImpl.ts:3712:9)
      at Context.<anonymous> (src/testRunner/fourslashRunner.ts:57:43)
      at processImmediate (internal/timers.js:456:21)


@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 7, 2020

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at db10229. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @sheetalkamat, 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/79103/artifacts?artifactName=tgz&fileId=C530B3CA0A000595EA0BE821554CD29D734277E601853EF651F4909FCB1BC0C902&fileName=/typescript-4.0.0-insiders.20200707.tgz"
    }
}

and then running npm install.

@typescript-bot
Copy link
Collaborator

Hey @sheetalkamat, something went wrong when looking for the build artifact. (You can check the log here).

@sheetalkamat sheetalkamat linked an issue Jul 8, 2020 that may be closed by this pull request
@sheetalkamat
Copy link
Member Author

@andrewbranch can you please review again. I had to add another commit to ensure we aren't included triple slash reference tree (but including them only from open file) since we removed noResolve.

@DanielRosenwasser @RyanCavanaugh what is your take on this. do we want to get this in?

@DanielRosenwasser
Copy link
Member

Yes, I think we want to get this in, but I have kind of been worrying about two scenarios:

  1. How well does this work when a file has 100+ imports? It's an edge-case, but it's something we should consider.
  2. Is there a way to remove pressure from the project system over time and go back to the single-file view of the world? Once the semantic server is loaded, we probably can unload some memory.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Latest change makes sense to me 👍

Comment on lines +1312 to -1326
hasChangedAutomaticTypeDirectiveNames,
includeTripleslashReferencesFrom: maybeBind(host, host.includeTripleslashReferencesFrom),
trace: maybeBind(host, host.trace),
resolveModuleNames: maybeBind(host, host.resolveModuleNames),
resolveTypeReferenceDirectives: maybeBind(host, host.resolveTypeReferenceDirectives),
useSourceOfProjectReferenceRedirect: maybeBind(host, host.useSourceOfProjectReferenceRedirect),
};
if (host.trace) {
compilerHost.trace = message => host.trace!(message);
}

if (host.resolveModuleNames) {
compilerHost.resolveModuleNames = (...args) => host.resolveModuleNames!(...args);
}
if (host.resolveTypeReferenceDirectives) {
compilerHost.resolveTypeReferenceDirectives = (...args) => host.resolveTypeReferenceDirectives!(...args);
}
if (host.useSourceOfProjectReferenceRedirect) {
compilerHost.useSourceOfProjectReferenceRedirect = () => host.useSourceOfProjectReferenceRedirect!();
}
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Co-authored-by: Daniel Rosenwasser <DanielRosenwasser@users.noreply.github.com>
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 16, 2020
@sheetalkamat
Copy link
Member Author

  1. How well does this work when a file has 100+ imports? It's an edge-case, but it's something we should consider.

Should we cap this to say resolving 10 module names per file or something. But i am not sure if anyone uses those many relative imports in the file. Dont have data to cap on number of say that we should handle it.

  1. Is there a way to remove pressure from the project system over time and go back to the single-file view of the world? Once the semantic server is loaded, we probably can unload some memory.

Do you think we should only use single file as open file (file thats last opened or asked to update project based on semantic operation requested). I did consider this but that just means we wont reuse the program as often if say user navigates to new file using go to def and hovers over certain item and goes back as an example. The current one keeps program updates to minimal and keeps on reusing things till it can.. leading to less program updates and more info. I am open to suggestion though.

@DanielRosenwasser
Copy link
Member

Should we cap this to say resolving 10 module names per file or something

Let's do it in a later pass, I'd like to try to get this in the next nightly so that we can get feedback as soon as we can.

The current one keeps program updates to minimal and keeps on reusing things till it can

Yeah, that's a good point. If this can unblock those strenuous cross-project program loads, I think the responsiveness will make up for it.

@sheetalkamat
Copy link
Member Author

Ok then merging.

@sheetalkamat sheetalkamat merged commit 667ba74 into master Jul 16, 2020
@sheetalkamat sheetalkamat deleted the syntaxOnlyResolutionsInOpenFiles branch July 16, 2020 21:31
@sheetalkamat sheetalkamat added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 16, 2020
@typescript-bot typescript-bot added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make cross-file go-to-definition work in the syntax-only server
4 participants