Allow explicit dependencies to resolve ambiguities #20853
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lilatomic
commented
Apr 28, 2024
| out.add(addr) | ||
| maybe_tgt_generator = addr.maybe_convert_to_target_generator() | ||
| if maybe_tgt_generator in self.includes: | ||
| # TODO: do we include the maybe_tgt_generator or the addr? |
There was a problem hiding this comment.
I'm honestly not sure about this. I should also probably add a test to pin down the desired behaviour
|
(Thanks for looking at this, given #20806 affects 2.20.0, I've pre-emptively labelled this for cherry-picking back to that branch.) |
|
What's the status on this? |
|
I think we said that this wasn't the right approach. In the slack thread we determined that we'd probably want to just give more/better warnings. The summary of the reasoning is that it's intentional that explicit deps function as overrides. So dep inference should not attempt resolution, and should instead defer to the explicit deps |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In case of an ambiguous dependency inference, we give the help text:
However, adding an explicit dependency doesn't actually resolve that.
In ExplicitlyProvidedDependencies.disambiguated, if the target is in the explicitly provided dependencies, we just return. This results in the dependency being unowned, but included because of the explicit dependency link.
This MR rebuilds that function to use the explicitly provided dependencies to attempt to resolve an ambiguity. The logic is now as follows:
This algorithm change does result in changes to dependency inference. We can now resolve a situation which had both includes and excludes, and the excludes would resolve it. Previously, this would not be resolved.
I don't think this will result in other changes in resolution. (I considered the case of the same address being included and excluded, but it seems that the excludes currently functionally take priority).
fixes #20806