Restore all_are_submodules import logic as workaround for #4498
#5016
Conversation
This file contains hidden or 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
The logic in build to determine what imported modules are depended on used to elide dependencies to m in `from m import a, b, c` if all of a, b, c were submodules. This was removed in #4910 because it seemed like it ought not be necessary (and that semantically there *was* a dependency), and early versions of #4910 depended removing it. The addition of this dependency, though, can cause cycles that wouldn't be there otherwise, which can cause #4498 (invalid type when using aliases in import cycles) to trip when it otherwise wouldn't. We've seen this once in a bug report and once internally, so restore the `all_are_submodules` logic in avoid triggering #4498 in these cases. Fixes #5015
|
Oh right, I had forgotten that to help justify removing that code I found a super obscure bug that removing it fixed. This will take marginally more thought if I don't just want to reintroduce it. |
Unfortunately the dependency on the module is actually required for correctness in some corner cases, so instead of eliding the import, we lower its priority. This causes the cycles in the regressions we are looking at to get processed in the order that works. This is obviously just a workaround.
|
Unfortunately the dependency on the module is actually required for This is obviously just a workaround. |
|
Though it looks like this fixes the #5015 bug but not the one we've observed internally. (Eliding the module when |
gvanrossum
approved these changes
May 14, 2018
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.
The logic in build to determine what imported modules are depended on
used to elide dependencies to m in
from m import a, b, cif all ofa, b, c were submodules. This was removed in #4910 because it seemed
like it ought not be necessary (and that semantically there was
a dependency), and early versions of #4910 depended removing it.
The addition of this dependency, though, can cause cycles that
wouldn't be there otherwise, which can cause #4498 (invalid type when
using aliases in import cycles) to trip when it otherwise wouldn't.
We've seen this once in a bug report and once internally, so restore
the
all_are_submoduleslogic in avoid triggering #4498 in thesecases.
Fixes #5015