Skip to content

Add script and test to check for any unused Diagnostics in the codebase #68902

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mininny
Copy link
Contributor

@mininny mininny commented Oct 2, 2023

This PR adds a python script and swift test file to check for any unused diagnostics in the codebase.
If uses regex to get all the diagnostics kinds from Diagnostic~.def files, and uses aho-corasick algorithm for faster searching of all the diagnostics in all files under /lib

  • I'm not yet sure how to properly use pythong script inside a swift test.
  • I'm not sure if it's okay to use external pip package aho-corasick inside swift test - would it be better to write the implementation in the python script entirely?

Resolves #67306

@mininny mininny force-pushed the test-check-unused-diagnostics branch from 856da1c to 60bee4f Compare October 2, 2023 09:27
@tbkka
Copy link
Contributor

tbkka commented Oct 2, 2023

@hamishknight @hborla I wonder if this script should be used in a test? That would help ensure we don't inadvertently leave behind unused diagnostics.

@mininny mininny marked this pull request as ready for review October 9, 2023 13:20
@mininny
Copy link
Contributor Author

mininny commented Oct 15, 2023

Hi @hamishknight @hborla,
I'd appreciate it if you could give any input about this script.

If you believe it's not suitable for adding to a test, I can document the stale diagnostics I've identified and create the necessary issues for their removal.

Thanks :)


def check_for_unused_diagnostics():
diagnostics = extract_all_diagnostics()
unused_diagnostics = check_strings_in_files(diagnostics, './lib')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may need to check ./include too since we could emit diagnostics in headers

@hamishknight
Copy link
Contributor

Sorry for the late reply! Adding it as a test seems reasonable to me, cc @xedin too. The script looks good to me modulo the above comment. Regarding using an external pip package, I believe we already have a python script test that depends on an external pip package (test/Python/python_lint.swift), though I think ideally we'd keep the dependencies to a minimum. Out of curiosity, how much longer does the script take doing a standard regex search?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We should have a tool that finds unused diagnostic IDs
3 participants