Don’t re-index all files that include a header when a header is modified #1306
Conversation
When a header is modified, we don’t we want to re-index all main files that include it. Instead, we just want to index one main to effectively re-index the header itself. I originally implemented re-indexing of all files that include the header but on second thought, headers are like Swift modules, where we also don’t re-index all dependencies either. And if you change a low-level header that’s included by the entire project, you probably don’t want the indexer to go off and re-index the entire project.
|
@swift-ci Please test |
| "MyLibrary/include/OtherHeader.h": "", | ||
| "MyLibrary/include/Header.h": """ | ||
| #include "OtherHeader.h" | ||
| void 1️⃣someFunc(); | ||
| """, | ||
| "MyFile.c": """ | ||
| #include "Header.h" |
There was a problem hiding this comment.
Would it be worth keeping a test that at least checks that transitive includes work for a single file?
| let filesToReIndex = await filesToReIndex(changedFiles: events.map(\.uri)) | ||
|
|
||
| // We only re-index the files that were changed and don't re-index any of their dependencies. See the | ||
| // `Documentation/Files_To_Reindex.md` file. |
There was a problem hiding this comment.
Does that need updating to mention we only pick one file to reindex?
There was a problem hiding this comment.
Yeah, thought about that on my way home last night as well and wanted to put up a PR to change that anyway. But well spotted 👍🏽
| guard let target = await buildSystemManager.canonicalConfiguredTarget(for: file.uri) else { | ||
| logger.error("Not indexing \(file.uri.forLogging) because the target could not be determined") |
There was a problem hiding this comment.
It probably(?) doesn't matter, but IMO passing the main file here makes more sense since that's the thing we're indexing
There was a problem hiding this comment.
For canonicalConfiguredTarget(for:): Yes, you’re right, we should use file.mainFile ?? file.uri
For logger.error: Added both file.uri and file.mainFile to the logging message.
| // module if one of its dependent modules has changed. | ||
| return index.checked(for: .deletedFiles).mainFilesContainingFile(uri: uri, crossLanguage: false) | ||
| }.flatMap { $0 } | ||
| return Set(filesToReIndex) |
There was a problem hiding this comment.
Given we're no longer uniquing, is there any concern that if e.g 5 headers get modified, and those headers are all included by a single main file, that we could end up running 5 concurrent indexing jobs for that main file?
There was a problem hiding this comment.
I also didn't really think about the scheduling logic here :) Looking at it again, we'll block the requests on each other, and then they should all be considered up-to-date
There was a problem hiding this comment.
We don’t actually, because they all have different FileToIndex.uri and thus they would actually be executing in parallel. Fixing that now.
|
Will address review comments in a follow-up PR. |
Address review comments to #1306
When a header is modified, we don’t we want to re-index all main files that include it. Instead, we just want to index one main to effectively re-index the header itself.
I originally implemented re-indexing of all files that include the header but on second thought, headers are like Swift modules, where we also don’t re-index all dependencies either. And if you change a low-level header that’s included by the entire project, you probably don’t want the indexer to go off and re-index the entire project.