resolve: Improve import failure detection and lay groundwork for RFC 1422 #32328
Conversation
|
After this PR, the first coherence condition from this comment will hold. |
| }; | ||
| (resolve_in_ns(ValueNS, value_determined.get()), | ||
| resolve_in_ns(TypeNS, type_determined.get())) | ||
| }; |
There was a problem hiding this comment.
The improved failure detection subsumes this hack.
|
☔ The latest upstream changes (presumably #32167) made this pull request unmergeable. Please resolve the merge conflicts. |
`resolved_globs` into a single field `globs: RefCell<Vec<ImportDirective>>`.
|
(note: starting reviewing, didn't yet finish) |
| None => return Some(Indeterminate), | ||
| }; | ||
| let name = match directive.subclass { | ||
| SingleImport { source, target, .. } if source == target => target, |
There was a problem hiding this comment.
I don't quite follow this -- why are you checking if source == target? Can you maybe give an example of some Rust code that would trigger this path?
|
looks good, though I'd like to learn the answer to my question :) r=me with an extended comment |
|
I improved the design in the above commit so that the The check was needed since this code assumed that there was a cycle when we try to resolve a name in a module that is already borrowed (i.e. in which we are already trying to resolve a name). If we only follow globs and non-renamed single imports, this assumption is correct since the names are guaranteed to be the same. If we followed renamed single imports, struct Bar;
use foo::Baz;
//^ here, we would try to resolve `foo::Baz`. Since `foo::Baz` is not yet successful,
//| we would follow the only single import that can define it (`use self::Bar as Baz`) and
//| try to resolve `foo::Bar`, which would incorrectly fail since `foo` is already borrowed
//| (causing the above import to incorrectly fail).
mod foo {
use self::Bar as Baz;
use Bar;
}The above commit borrows the |
|
I was always suspicious of that logic around cycle detection, makes sense. @bors r+ |
|
📌 Commit 6f09dea has been approved by |
|
⌛ Testing commit 6f09dea with merge 3562671... |
|
@bors: retry force clean |
|
⌛ Testing commit 6f09dea with merge 7fd331e... |
resolve: Improve import failure detection and lay groundwork for RFC 1422 This PR improves import failure detection and lays some groundwork for RFC 1422. More specifically, it - Avoids recomputing the resolution of an import directive's module path. - Refactors code in `resolve_imports` that does not scale to the arbitrarily many levels of visibility that will be required by RFC 1422. - Replaces `ModuleS`'s fields `public_glob_count`, `private_glob_count`, and `resolved_globs` with a list of glob import directives `globs`. - Replaces `NameResolution`'s fields `pub_outstanding_references` and `outstanding_references` with a field `single_imports` of a newly defined type `SingleImports`. - Improves import failure detection by detecting cycles that include single imports (currently, only cycles of globs are detected). This fixes #32119. r? @nikomatsakis
This PR improves import failure detection and lays some groundwork for RFC 1422.
More specifically, it
resolve_importsthat does not scale to the arbitrarily many levels of visibility that will be required by RFC 1422.ModuleS's fieldspublic_glob_count,private_glob_count, andresolved_globswith a list of glob import directivesglobs.NameResolution's fieldspub_outstanding_referencesandoutstanding_referenceswith a fieldsingle_importsof a newly defined typeSingleImports.r? @nikomatsakis