-
Notifications
You must be signed in to change notification settings - Fork 13.9k
fix(resolve): not defined extern crate shadow_name
#111761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is just how expansion algorithm works, it turns not-yet-expanded macro calls (including attribute invocations) into dummy nodes, and then expands them into real nodes. Cases like this is exactly why we have |
There's a third solution - remove the assert, it is not very important. The assert already doesn't hold when ambiguity errors are reported (that's why it's wrapped into the The "macro-expanded Ideally we'd use something like let no_ambiguity = self.ambiguity_errors.len() == prev_ambiguity_errors_len || that_specific_extern_crate_error_was_reported;but the assert is not important enough for |
|
Simplified reproduction without unresolved attributes and crates: macro_rules! m {
() => {
extern crate core as std;
}
}
m!();
use std::mem;
fn main() {}Could you use it as the test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a good solution too.
Could you add a comment saying that this is an ambiguity error, but it's not registered in ambiguity_errors, and that triggers some asserts later on if the binding is added, so we don't add the binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth discussing the actual binding of use std::xxx.
When we add return here, std::xxx ultimately refers to prelude::std.
However, based on your comment, it seems that std::xxx will point to extern crate blash(I guess, but not entirely sure).
There might be a difference between these two resolutions, so in my opinion, we should determine which action is better first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice is not that important because it only happens during failing compilation anyway.
The difference may only happen in some secondary diagnostics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this case is migrated to regular ambiguity errors, then extern crate blah will be selected as a resolution, yes, because it's closer in scopes at the resolution point.
(This migration is probably a better long term solution, but it's a minor language change, so it's better done separately from fixing an ICE.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the migration to regular ambiguity errors happens then this test case:
macro_rules! m {
() => {
extern crate core as std;
use std::mem;
}
}
m!();
fn main() {}won't produce any errors.
|
@rustbot ready |
|
@bors r+ rollup |
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#111427 ([rustdoc][JSON] Use exclusively externally tagged enums in the JSON representation) - rust-lang#111486 (Pretty-print inherent projections correctly) - rust-lang#111722 (Document stack-protector option) - rust-lang#111761 (fix(resolve): not defined `extern crate shadow_name`) - rust-lang#111845 (Update books) - rust-lang#111851 (CFI: Fix encode_region: unexpected ReEarlyBound(0, 'a)) - rust-lang#111871 (Migrate GUI colors test to original CSS color format) r? `@ghost` `@rustbot` modify labels: rollup
Fixes #109148
Why does #109148 panic?
When resolving
use std::xxit entersvisit_scopesfromearly_resolve_ident_in_lexical_scope, and iters twice during the loop:scopebreak_resultModulepointed to rootUndetermined, so result isNoneExternPreludeExternPreludestdThen, the result of
maybe_resolve_pathisModule(std), soimport.imported_module.setis executed.Finally, during the
finalize_importofuse std::xx,resolve_pathreturnsNonModulebecauseBinding(Ident(std), Module(root)'s binding points toextern crate blah as std, which causes the assertion to fail atassert!(import.imported_module.get().is_none());.Investigation
The question is why
#[a] extern crate blah as stdis not defined as a binding ofstd::xxx, which causes the iteration twice duringvisit_scopeswhen resolvingstd::xxx. Ideally, the value ofbreak_result.is_some()should have been valid in the first iteration.After debugging, I found that because
#[a] extern crate blah as stdhad been dummied byplaceholderduringcollect_invocations, so it had lost its attrs, span, etc..., so it will not be defined. However,expand_invocadded them back, then the nextbuild_reduced_graph,#[a] extern crate blah as stdwould have been defined, so it makes the result ofresolved_pathunexpected, and the program panics.Try to solve
I think there has two-way to solve this issue:
resolve_importsduringfully_expand_fragment. However, I do not think this is a good idea because it would mess up the current design.extern crate blah as stdduring the secondbuild_reduced_graph, which is very easy and more reasonable.r? @petrochenkov