Fix iterating over namespaces imported using '*' (star) reporting unused classes

[heystewart]

Using import * as classes from..., and then doing an iteration for (... in classes) erroneously marks the imported classes as 'not used'.

The import-star path leads to a namespace being added, and the reference to the namespace due to iteration was not being considered, although Object.keys-type accesses were.

While fixing the ...in case, I realized the ...of iteration case was also potentially broken, although it was very difficult to build a test for that particular edge case ... it is highly dependent on the tsconfig options.

Drive-bys:

• The tests tend to test specific issues occurring, but not that other entries are empty. I added 'noIssuesReported' test helper for this. To do this, I also added key names to the Issues, to be able to iterate over them safely without the if (files) continue pattern. Open to suggestions on this one...
• I specified to 'ignoreBinaries' and 'ignoreDependencies', but I still ended up with an entry in Issues for the package.json file, but this entry was empty. I have fixed the 'ignore' code to delete the file entry (in this case, package.json) when all the issues in the file are ignored.

[webpro]

Thanks for the pull request, @heystewart

The tests tend to test specific issues occurring, but not that other entries are empty.

For assertions, including empty entries, there are counters and issues. You could console.log({ counters, issues }) and see all types of issues. Many tests have something like this to assert there are no issues reported:

  assert.deepEqual(counters, {
    ...baseCounters,
    processed: 1,
    total: 1,
  });

I specified to 'ignoreBinaries' and 'ignoreDependencies', but I still ended up with an entry in Issues for the package.json file, but this entry was empty. I have fixed the 'ignore' code to delete the file entry (in this case, package.json) when all the issues in the file are ignored.

The tests are pretty much integration/end-to-end tests in the sense that also references to binaries in package.json#scripts are reported if the fixtures folder does not actually contain the binary file. So either the (dummy) package should be added to it's node_modules or the reference to the binary should be removed from package.json#scripts.

Any chance you could minimize this PR to its essentials? I'm interested in merging that :)

As a side note, I refer to adding more supported cases as additional features, not things that were bugs or broken. Knip as a whole essentially is heuristics and patterns to maximize output value and thus minimize false positives

[heystewart]

The tests are pretty much integration/end-to-end tests in the sense that also references to binaries in package.json#scripts are reported if the fixtures folder does not actually contain the binary file. So either the (dummy) package should be added to it's node_modules or the reference to the binary should be removed from package.json#scripts.

I'm not quite sure I understand! The code is producing the 'correct' issues, but it creates an entry for a package that has only ignored errors, and therefore shows an empty list. I am removing the empty list so that the output is consistent with what would occur if the ignored binaries weren't even mentioned.

[webpro]

Probably best to remove unrelated things from this PR so we can focus on its main purpose :)

[heystewart]

As a side note, I refer to adding more supported cases as additional features, not things that were bugs or broken. Knip as a whole essentially is heuristics and patterns to maximize output value and thus minimize false positives

🤔 are you referring to the ignore binaries, or the infer error? This is a false positive ("You can delete this code!" when it is not true), so I am guessing this would be considered a bug.

If you are referring to the ignore binaries, I can revert the noIssuesReported and all that; I consider it a positive effect, since it leads to standardization and completeness of the tests, but it is not as critical as the actual import-star fix.

[webpro]

Would be great if we could leave in only what's specifically required for the feature you'd like to add. Then we can discuss, track, squash-merge and have release notes generated from (merge) commits separately.

[heystewart]

PTALA @webpro

[heystewart]

So either the (dummy) package should be added to it's node_modules or the reference to the binary should be removed from package.json#scripts.

To clarify, the binary was in the fixture package intentionally to ensure that the fixture code was generating the expected output.

[pkg-pr-new]

Open in Stackblitz

npm i https://pkg.pr.new/knip@898

commit: d34e32e

[webpro]

Thanks a lot for bearing with me @heystewart! I might make minor edits. Planning to release this tomorrow.

[webpro]

🚀 This pull request is included in v5.42.0. See Release 5.42.0 for release notes.

Using Knip in a commercial project? Please consider becoming a sponsor.