Follow test references to calculate unused files by sandersn · Pull Request #710 · microsoft/types-publisher

sandersn · 2019-11-21T22:29:20Z

To make sure that I didn't break anything, I dumped the list of dependencies generated during parsing. In getTypingInfo, I added the following code:

    console.log(`"${packageName}": {`)
    let first = true
    for (const k in res) {
        if (first)
            first = false
        else
            console.log(",")
        console.log(`  "${k}": ${JSON.stringify(res[k].dependencies.map(d => d.name))}`)
    }
    console.log("},")

I put the same code in the production version of types-publisher. This produced (1) almost-legal JSON (2) a diffable file for both. I diffed the files and found 57 differences:

30: order differences from sorting or not.
2: incorrect types="x/v7" references that are now corrected to just types="x" by the additional call to rootName. I added a test to show that we detect and prevent such references.
2: incorrect references "chrome/chromecast" from types="chrome/chromecast" are corrected to just "chrome" by the additional call to rootName.
1: incorrect reference to "@ember/services", from @types/ember__services itself.
22: missing references from untested files. I added entries to OTHER_FILES.txt to add these missing references.

There were lots of unused files that I automatically added OTHER_FILES.txt entries for. That's the next step in the process I wrote up at #708.

Note: I also need to figure out how to fix the 2 references to types="x/v7" which the tests relied on. I think I'll have to add them to the whitelist. =[
Edit: I just updated the tests to newer versions of dependencies: DefinitelyTyped/DefinitelyTyped#40575

Lots of notes on how it *should* work, plus some new tests. It doesn't work the way it should yet though.

Still need to look at TODOs in definition-parser and probably write more tests.

1. errors when it's already reachable (it shouldn't be in the list) 2. errors when it doesn't exist (it ... also shouldn't be in the list).

even though it is extremely confusing

sandersn

Explanatory comments. Let me know if you think any of it should be baked into the source as comments.

sandersn · 2019-11-22T17:44:50Z

src/lib/definition-parser.ts

@@ -155,28 +155,25 @@ async function getTypingDataForSingleTypesVersion(
    oldMajorVersion: number | undefined,
 ): Promise<TypingDataFromIndividualTypeScriptVersion> {
    const tsconfig = await fs.readJson("tsconfig.json") as TsConfig; // tslint:disable-line await-promise (tslint bug)


allReferencedFiles finds all the files that are referenced transitively from the files listed in tsconfig. Then we manually reference d.ts files from OTHER_FILES.txt as references.

Next, the external dependencies are calculated. This part doesn't change too much, I just moved checkAllFilesUsed's call earlier and the no-windows-\ check earlier, into checkAllFilesUsed.

sandersn · 2019-11-22T17:47:22Z

src/lib/module-info.ts

        }
    }

-    return { declFiles: sort(all.keys()), dependencies, declaredModules, globals: sort(globals) };


I moved the sort(all.keys()) out of this function since it didn't particularly belong.

sandersn · 2019-11-22T17:47:53Z

src/lib/module-info.ts


-    function addDependency(dependency: string): void {
+    function addDependency(ref: string): void {
+        if (ref.startsWith(".")) return;


this now happens for all calls to addDependency, which fixes a couple of bogus dependencies I saw on DT.

sandersn · 2019-11-22T17:48:22Z

src/lib/module-info.ts

-    return slash === -1 ? importText : importText.slice(0, slash);
+    const root = importText.slice(0, slash);
+    const postImport = importText.slice(slash + 1);
+    if (slash > -1 && postImport.match(/v\d+$/) && !typeFiles.has(postImport + ".d.ts")) {


this error is new, and required a couple of packages on DT to be fixed.

Much later: This should be legal if the import is from the same package, eg "foo/v3" should be legal inside "foo".
Not sure how to detect this yet.

sandersn · 2019-11-22T17:50:28Z

src/lib/module-info.ts


 /** Returns a map from filename (path relative to `directory`) to the SourceFile we parsed for it. */
-async function allReferencedFiles(entryFilenames: ReadonlyArray<string>, fs: FS, baseDirectory: string): Promise<Map<string, ts.SourceFile>> {
+export async function allReferencedFiles(


allReferencedFiles now scans through .d.ts AND .ts files, instead of only looking at d.ts files. It uses extension d.ts-vs-ts to decide whether the referenced file is a type file or a test file.

This is not precisely equivalent to the old algorithm but it produces the same results on DT (so far at least).

sandersn · 2019-11-22T17:53:22Z

src/lib/module-info.ts

-            if (!imported.startsWith(".") && !dependencies.has(imported) && imported !== pkgName) {
-                testDependencies.add(imported);
+            if (!imported.startsWith(".")) {
+                const dep = rootName(imported, typeFiles);


this call to rootName is new, and caught several mistakes on DT.

sandersn · 2019-11-22T17:53:47Z

src/mocks.ts

+    boring.set("boring-tests.ts", `
+import { superstor } from "super-big-fun-hus";
+import { drills } from "boring";
+import { hovercars } from "boring/secondary";


need to figure out how to work a cybertruck reference in here somewhere

andrewbranch

This looks great. (I reviewed for intent, not correctness, since you have tests that presumably validate everything works.)

andrewbranch · 2019-11-22T21:02:59Z

src/lib/definition-parser.ts

 }

-/** In addition to dependencies found oun source code, also get dependencies from tsconfig. */
+/** In addition to dependencies found in source code, also get dependencies from tsconfig. */


sandersn · 2019-11-22T21:04:28Z

I'll merge and ship once I have the related DT PR(s) ready to go.

sandersn · 2019-11-26T18:15:57Z

I missed a last minute change, which I just pushed to master: c169523

sandersn added 11 commits November 19, 2019 16:06

Saving work so far

e47e6c4

Lots of notes on how it *should* work, plus some new tests. It doesn't work the way it should yet though.

fix header format in test files

bd2c2bf

In-progress, very broken

b803311

Add .vscode/launch.json for debugging

cf83d20

Tests all pass, refactor is mostly done.

7f9f499

Still need to look at TODOs in definition-parser and probably write more tests.

cleanup/refactoring

ed6413b

add dependencies from OTHER_FILES.txt

6e663bc

more refactoring

3dcefc6

remove TODOs

ff0da44

error on bad types references

416a4f1

more errors for OTHER_FILES.txt

4f4eb11

1. errors when it's already reachable (it shouldn't be in the list) 2. errors when it doesn't exist (it ... also shouldn't be in the list).

sandersn requested a review from RyanCavanaugh as a code owner November 21, 2019 22:29

sandersn mentioned this pull request Nov 21, 2019

index.d.ts should be the only d.ts file allowed in tsconfig.json #708

Closed

9 tasks

sandersn added 3 commits November 21, 2019 15:29

fix test failures

c7e7ee0

actually fix tests

a7ee958

Allow files named v1.d.ts

c5d2740

even though it is extremely confusing

sandersn requested a review from andrewbranch November 22, 2019 17:28

sandersn commented Nov 22, 2019

View reviewed changes

add logging and commentary

5925433

andrewbranch approved these changes Nov 22, 2019

View reviewed changes

Improve new error message

bd99c23

sandersn merged commit 85b12f6 into master Nov 26, 2019

sandersn deleted the follow-test-references-to-calculate-unused-files branch November 26, 2019 18:12

jablko mentioned this pull request Apr 21, 2020

testDependencies should include only top-level package names #517

Open

Conversation

sandersn commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewbranch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Nov 22, 2019

Uh oh!

sandersn commented Nov 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sandersn commented Nov 21, 2019 •

edited

Loading