Skip to content

[Dependency Scanning] Adding Diagnostics C-API for Invalid Negative-stat-cached Paths Incurred During Clang Modules Scan #81944

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

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

Conversation

qiongsiwu
Copy link
Contributor

@qiongsiwu qiongsiwu commented Jun 3, 2025

This PR adds a C-API swiftscan_scanner_get_invalid_negative_stat_cached_paths that takes a
DependencyScanningTool as input.

rdar://150954767

@qiongsiwu
Copy link
Contributor Author

qiongsiwu commented Jun 3, 2025

Note to reviewers:

  1. I am not sure if it is sufficient to simply pick the base file system to check against new files as I did in SwiftDependencyScanningService::getInvalidNegativeStatCachedPaths. Comments/suggestions are welcome!
  2. Most of the PR is plumbing. Specifically, I need to propagate the information from the SharedCache within the DependencyScanningService all the way up to Swift's DependencyScanningService. Additionally, a new function swiftscan_string_set_t * create_set(const std::vector<llvm::StringRef> &stringRefs) is added to avoid converting a vector of StringRefs to a vector of std::strings.
  3. I don't have a good idea of how to add a unit test for this feature. The key challenge is that I need to do two scans with the same service, and insert a new file into the file system in between the scans. I am looking into ways to implement this test. Pointers to existing examples are appreciated!

@qiongsiwu qiongsiwu self-assigned this Jun 3, 2025
Copy link
Contributor

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

Can you specify the intent usage this new API? In both commit message and proper documentation around the API?

For the test, I created swift-scan-test binary to test C APIs from scanner. There is a scan option but only takes one command. Potentially, you can add a new command that takes multiple commands (maybe through a YAML or JSON file) so some work needs to be done here.

Alternatively, you can look to write the test in swift-driver repo.

@@ -838,9 +838,11 @@ generateFullDependencyGraph(const CompilerInstance &instance,
swiftTextualDeps->textualModuleDetails.bridgingSourceFiles),
create_set(clangHeaderDependencyNames),
create_set(bridgedOverlayDependencyNames),
/*sourceImportedDependencies*/ create_set({}),
/*sourceImportedDependencies*/
create_set(std::vector<std::string>()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to change to create_empty_set?

@@ -458,6 +458,10 @@ swiftscan_scanner_diagnostics_query(swiftscan_scanner_t scanner);
SWIFTSCAN_PUBLIC void
swiftscan_scanner_diagnostics_reset(swiftscan_scanner_t scanner);

SWIFTSCAN_PUBLIC swiftscan_string_set_t *
swiftscan_scanner_get_invalid_negative_stat_cached_paths(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be in the Scanner Diagnostics API section and need proper documentation.

@qiongsiwu
Copy link
Contributor Author

I think I can test the C++ API in Swift's DependencyScanningService by adding a new test in https://github.com/swiftlang/swift/tree/main/unittests/DependencyScan. There I can have some control over the instance of scanning service and the sequences of actions we need to trigger the diagnostics (similar to https://github.com/swiftlang/swift/blob/main/unittests/DependencyScan/ModuleDeps.cpp).

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.

2 participants