[Dependency Scanning] On failure to locate a module, attempt to diagnose if binary dependencies contain search paths with this module. #81919
Conversation
artemcm
force-pushed
the
DiagnoseMissingModulesSeenInSerializedSearchPaths
branch
3 times, most recently
from
5eed2d4 to
a4af685
Compare
|
@swift-ci test |
artemcm
requested review from
xymus,
cachemeifyoucan,
hborla,
slavapestov and
xedin
as code owners
artemcm
force-pushed
the
DiagnoseMissingModulesSeenInSerializedSearchPaths
branch
from
b746c2c to
e5edd7e
Compare
|
@swift-ci test |
| @@ -2378,6 +2378,9 @@ NOTE(dependency_as_imported_by_main_module,none, | |||
| "a dependency of main module '%0'", (StringRef)) | |||
| NOTE(dependency_as_imported_by, none, | |||
| "a dependency of %select{Swift|Clang}2 module '%0': '%1'", (StringRef, StringRef, bool)) | |||
| GROUPED_NOTE(inherited_search_path_resolves_module,MissingModuleOnKnownPaths,none, | |||
There was a problem hiding this comment.
I would expect the MissingModuleOnKnownPaths group to be associated with the original warning/error, rather than with the note. I see why you structured it this way, but I think it could be done instead by emitting a different error diagnostic ID when the conditions are met
There was a problem hiding this comment.
In general, note is associated with the previous error or warning, not standalone. Maybe the sensible choice is to make a separate version of module not found error that is in a group for documentation.
There was a problem hiding this comment.
I removed the GROUPED_NOTE and added a new GROUPED_ERROR for this scenario which is always going to be followed by the new note in this PR.
| @@ -2378,6 +2378,9 @@ NOTE(dependency_as_imported_by_main_module,none, | |||
| "a dependency of main module '%0'", (StringRef)) | |||
| NOTE(dependency_as_imported_by, none, | |||
| "a dependency of %select{Swift|Clang}2 module '%0': '%1'", (StringRef, StringRef, bool)) | |||
| GROUPED_NOTE(inherited_search_path_resolves_module,MissingModuleOnKnownPaths,none, | |||
There was a problem hiding this comment.
In general, note is associated with the previous error or warning, not standalone. Maybe the sensible choice is to make a separate version of module not found error that is in a group for documentation.
| IsSystemField // isSystem | ||
| >; | ||
| using SearchPathArrayLayout = | ||
| BCRecordLayout<SEARCH_PATH_ARRAY_NODE, IdentifierIDArryField>; |
There was a problem hiding this comment.
Do we need to serialize this? We only serialize successful scanning output right? If the search path changed, I assume the hash will change, thus the entire graph is invalidated.
There was a problem hiding this comment.
I prefer that at least with small things like these the serialized scanner cache be relatively self-contained. And however contrived, the added cached-missing-module-found-in-serialized-paths.swift test shows one example where we may be re-using prior scan results which are legitimately not invalidated and still end up emitting this diagnostic because a module which was previously on the search paths got removed and can now only be resolved using binary dependency serialized search paths which also got captured in the serialized data.
There was a problem hiding this comment.
I also still want to add a tool that dumps the contents of these files in a human-readable format and that would make a nice diagnostic tool for compiler folk to be able to examine prior scan results and have them capture these details.
| @@ -2378,6 +2378,9 @@ NOTE(dependency_as_imported_by_main_module,none, | |||
| "a dependency of main module '%0'", (StringRef)) | |||
| NOTE(dependency_as_imported_by, none, | |||
| "a dependency of %select{Swift|Clang}2 module '%0': '%1'", (StringRef, StringRef, bool)) | |||
| GROUPED_NOTE(inherited_search_path_resolves_module,MissingModuleOnKnownPaths,none, | |||
| "'%0' can be found using a search path that was specified when building module '%1' ('%2'). " | |||
| "This search path was not specified on the current compilation.", (StringRef, StringRef, StringRef)) | |||
There was a problem hiding this comment.
This part of the note reads like the search path in the binary module has to be specified. Maybe this is better?
This search path is not inherited by the current module and needs to be specified explicitly if needed
There was a problem hiding this comment.
We decided to add the group with a corresponding documentation blurb in order to avoid talking about "inheriting" of search paths in the diagnostic here. Otherwise, I also added an emphasis that this discovered search paths needs to explicitly be specified on the current invocation.
| // Note: this will permanently mutate this worker with additional search | ||
| // paths. That's fine because we are diagnosing a scan failure here, but | ||
| // worth being aware of. | ||
| withDependencyScanningWorker( |
There was a problem hiding this comment.
This search is very inefficient (yes, it is in an error path, but that error path might take forever). You are adding path one by one to search with all the paths that are previously added. It is done for number of missing modules x number of binary modules x number of search paths in the binary modules even many paths are probably duplicated.
I don't know if it is possible to do one extra scan with all the information and look at the scan result to figure out the which search path worked by inspecting the search result.
There was a problem hiding this comment.
I changed it to be per-module. And I derive the search path from the discovered module's path.
There was a problem hiding this comment.
I think number of missing modules x number of binary modules is a reasonable search space, given that the former is likely to be 1 and it would take a bit more complexity to track which binary modules contributed which paths and such.
There was a problem hiding this comment.
It is not really tracked for search path contribution anyway. I think if a search path can be found in two different modules, you just report the one from whichever module is sorted first in std::set Maybe it should just be a dependency order traverse and build a map for search path and moduleID, the go back and figure out the search path from definition location, then reverse map to the moduleID.
The other benefit to do in one shot is you always search for swift first, then clang, so you won't take a search path that can only find the clang module when swift module exists.
However, I don't have benchmark result to convince if the extra benefit is worth the effort, even though I don't think it should be too bad.
There was a problem hiding this comment.
It is not really tracked for search path contribution anyway.
We sort of do, because the first binary module for which we take the search paths and then the lookup succeeds is guaranteed to be the one which contributed the culprit path, and then the search will exit.
I tried doing the reverse lookup from the defining path of the discovered module back to search paths contributed by each individual binary module and I worry that doing the attribution from module-defining path to a specific search path from a given module can be brittle as I'm not sure it's always going to be doable with a simple string comparison. It would be more unfortunate to detect this scenario and emit a diagnostic which does not accurately pinpoint the contributing module. On top of the added complexity of needing to do this, I'm inclined to keep this as-is, and we can always re-consider in the future.
There was a problem hiding this comment.
I am not arguing it is not deterministic but the definition of first binary module is probably just ordered by name or something.
Also the case can be quite complicated for a clang module, since if you try to look up A which depends on B, both in different paths, you first add a path where A locates, you will still failed to find A, until you pull in a different module which has a path that finds B, then you can find A. Your error message is going to be attributing to a module and a path that doesn't even related to A.
There was a problem hiding this comment.
Also the case can be quite complicated for a clang module, since if you try to look up
Awhich depends onB, both in different paths, you first add a path whereAlocates, you will still failed to findA, until you pull in a different module which has a path that findsB, then you can findA. Your error message is going to be attributing to a module and a path that doesn't even related toA.
This is an interesting example, though the current diagnostic will still help this user incrementally.
Suppose we are iterating over binary modules X, Y, Z where X adds search path to find A and Z adds search path to find B.
We will get a diagnostic that says:
"Could not find A. We found A on search paths in Z: <search path to B>"
And once the user added those search paths, they will then get:
"Could not find A. We found A on search paths in X: <search path to A>"
So at least they will be able to resolve that, still, even though the initial diagnostic is suboptimal.
Similarly, for the simpler case of a single search path, if the first module is just one of multiple that contribute a key search path, that's not necessarily a bad thing for this diagnostic.
The alternative is to track defining search paths of all transitively-discovered modules and attempt to attribute them to a combination of Swift modules which separately contributed constituent search paths. This scenario is possible, but the common case I have seen and imagine would trigger this the most is simply having a test target which imports a library target but does not specify some search path required to find a library dependency, so I am not sure the complexity of the above is worthwhile.
And I'm still concerned about the brittleness of the reverse lookup from a .modulemap or .swiftinterface path to a search path contained in a binary module since this has to reverse the actual lookup logic.
There was a problem hiding this comment.
And I'm still concerned about the brittleness of the reverse lookup from a .modulemap or .swiftinterface path to a search path contained in a binary module since this has to reverse the actual lookup logic.
The problem is you already did that. Folding the modules into one search is easy but reverse map to search path is hard.
"Could not find A. We found A on search paths in Z: "
The downside is people might just stop here and complain that A is not in <search path to B> and file a bug report
There was a problem hiding this comment.
The problem is you already did that. Folding the modules into one search is easy but reverse map to search path is hard.
As done now, module attribution is free (with the caveat of the multiple required paths coming from multiple binary modules edge case you outline above) and getModuleDefiningPath is done as a utility to best-effort print a path to provide a suggestion.
The brittleness I was referring to is that if we for some reason cannot with 100% certainty do the reverse map to search path contributed by some module then the diagnostic is much less useful without module attribution. With most build system, my intuition is that knowing which other "target" has the correct configuration would be the most actionable piece of information. Or do you think that module attribution is less important than displaying a path? Do you have a suggestion on how to do search path attribution in a guaranteed way?
There was a problem hiding this comment.
I do think path is more important than search path, and I think developer can figure out the search path from the given path themselves.
My suggestions:
- Since this note doesn't need to be perfect, there is a less perfect way to issue this warning without doing any searching. That is if the missing module is a direct dependency of a binary module (or indirect if you want to compute that), you know the search path when building that module was at some point, found the module, so you can issue a note directly without searching, just need to word it less confidently.
- If you intended to search anyway, I don't know how bad that is. Maybe some perf number for real world use case will be helpful (since you append some paths to an already large search path).
One more thing that comes to my mind. The reason why the search path is in the module is because we didn't remove them from command-line but there is no reason for the explicit build compilation need to know the search path so we can totally drop them and search path will no longer be available in binary modules. I don't know if the goal is to hold onto those flags forever for diagnostic purposes or we are going to drop them in near future.
artemcm
force-pushed
the
DiagnoseMissingModulesSeenInSerializedSearchPaths
branch
from
e5edd7e to
d02f462
Compare
|
@swift-ci test |
|
@swift-ci test macOS platform |
…ose if binary dependencies contain search paths with this module. Unlike with implicitly-built modules (prior to Swift 6 mode), explicitly-built modules require that all search paths be specified explicitly and no longer inherit search paths serialized into discovered Swift binary modules. This behavior was never intentional and is considered a bug. This change adds a diagnostic note to a scan failure: for each binary Swift module dependency, the scanner will attempt to execute a dependency scanning query for each serialized search path inside that module. If such diagnostic query returns a result, a diagnostic will be emitted to inform the user that the dependency may be found in the search path configuration of another Swift binary module dependency, specifying which search path contains the "missing" module, and stating that such search paths are not automatically inherited by the current compilation.
… failure which specifies that a missing module dependency can be found on a search path serialized in another Swift binary module dependency.
artemcm
force-pushed
the
DiagnoseMissingModulesSeenInSerializedSearchPaths
branch
from
d02f462 to
d4abba1
Compare
|
@swift-ci test |
|
@swift-ci test macOS platform |
|
@swift-ci test macOS platform |
|
@swift-ci test macOS platform |
1 similar comment
|
@swift-ci test macOS platform |
|
@swift-ci test macOS platform |
Unlike with implicitly-built modules (prior to Swift 6 mode), explicitly-built modules require that all search paths be specified explicitly and no longer inherit search paths serialized into discovered Swift binary modules. This behavior was never intentional and is considered a bug. This change adds a diagnostic note to a scan failure: for each binary Swift module dependency, the scanner will attempt to execute a dependency scanning query for each serialized search path inside that module. If such diagnostic query returns a result, a diagnostic will be emitted to inform the user that the dependency may be found in the search path configuration of another Swift binary module dependency, specifying which search path contains the "missing" module, and stating that such search paths are not automatically inherited by the current compilation.