Skip to content

fix(resolve): replace bindings to dummy for unresolved imports #109602

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
May 19, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Mar 25, 2023

close #109343

In #109343, f in pub use f as g points to:

namespace binding
type external crate f
value None
macro None

When resolve value_ns during resolve_doc_links, the value of the binding of single_import pub use f as g goes to pub use inner::f, and since it does not satisfy !self.is_accessible_from(binding.vis, single_import.parent_scope.module) and returns Err(Undetermined), which eventually goes to PathResult::Indeterminate => unreachable!.

This PR replace all namespace binding to dummy_binding for indeterminate import, so, the bindings of pub use f as g had been changed to followings after finalize:

namespace binding
type dummy
value dummy
macro dummy

r?@petrochenkov

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 25, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 26, 2023

In my opinion, there are three solutions:

  1. as I described in the PR, skip the binding whose Import marked by dummy.
  2. add dummy_binding to the import_dummy_binding function for all namespaces whose binding has the value None, but this doesn't seem like a good way.
  3. change the way resolve doc meta is handled to make sure it has the similar logic as the following code:
#![crate_type = "lib"]
extern crate f;

pub use inner::f;

pub use f as g;

g!{}

fn main() {}

@petrochenkov
Copy link
Contributor

  1. add dummy_binding to the import_dummy_binding function for all namespaces whose binding has the value None, but this doesn't seem like a good way.

But we are already doing that?
In fn import_dummy_binding, just below dummy.set(true);.

and since it does not satisfy [!self.is_accessible_from(binding.vis, single_import.parent_scope.module)]

Why does is_accessible_from return false?
Everything seems public enough there.
Maybe visibilities are not set correctly for dummy bindings?

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 30, 2023

But we are already doing that?

No, at this stage we only add dummy_binding to import if the target_binding of all three namespaces is None.

The type namespace of f points to external crate f, so it does not satisfy this condition, so it ends up without value and macro without dummy_binding in it.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 30, 2023

Wait a minute, maybe we're not talking about the same import, I need to look at it again

@petrochenkov
Copy link
Contributor

so it ends up without value and macro without dummy_binding in it

And without dummy.set(true) too, so I'm not sure what is changed.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 30, 2023

The logic goes like this:

For pub use inner::f, its three bindings point to dummy_binding.

For pub use f as g, since the binding of f's type namespace points to extern crate f, its macro and value both point to None instead of dummy_binding.

When dealing with value namespace, f gets the binding which pointed to pub use inner::f (instead of dummy_binding), which is visible to true, so is_accessible_from returns true and !is_accessible_from returns false

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Mar 30, 2023

And without dummy.set(true) too, so I'm not sure what is changed.

Yes, for pub use f as g does not work.

Perhaps we could put binding.is_dummy_binding_import to something that doesn't rely on a token value like dummy, for example to import.target_binding.all(|binding| binding.get().is_none())

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 31, 2023
@petrochenkov
Copy link
Contributor

The issue is not related to the extern crate compatibility case.
Further minimized reproduction:

#![crate_type = "lib"]

pub mod f {}
pub use unresolved::f;

/// [g]
pub use f as g;

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2023
@bvanjoi bvanjoi force-pushed the fix-issue-109343 branch from dccd3ff to 16fbe21 Compare May 11, 2023 18:10
@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 11, 2023

In the newest commit, I had replaced all bindings to dummy for indeterminate imports, It seems more reasonable to avoid panic caused by Undetermined, cc @petrochenkov

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 11, 2023
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2023
@bvanjoi bvanjoi force-pushed the fix-issue-109343 branch from 16fbe21 to 1e8f887 Compare May 12, 2023 16:18
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 15, 2023
@klensy
Copy link
Contributor

klensy commented May 15, 2023

PR in rollup #111577 will be merged soon, testing already.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 15, 2023

PR description is very out of date.

Thanks for the tips! @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2023
@bvanjoi bvanjoi changed the title fix(resolve): continue when resolved of signle_import is dummy fix(resolve): replace bindings to dummy for unresolved imports May 15, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 16, 2023

📌 Commit 1e8f887 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2023
@bors
Copy link
Collaborator

bors commented May 17, 2023

⌛ Testing commit 1e8f887 with merge 94aba6b5c5ca54c7be1c4f20739fc233ada488c8...

@bors
Copy link
Collaborator

bors commented May 17, 2023

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 17, 2023
@rust-log-analyzer

This comment has been minimized.

@bvanjoi bvanjoi force-pushed the fix-issue-109343 branch from 1e8f887 to f34678c Compare May 18, 2023 01:23
@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 18, 2023

timeout again and I rebased latest code. so @rustbot ready

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 18, 2023

📌 Commit f34678c has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2023
@bors
Copy link
Collaborator

bors commented May 19, 2023

⌛ Testing commit f34678c with merge 92f5dea...

@bors
Copy link
Collaborator

bors commented May 19, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 92f5dea to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2023
@bors bors merged commit 92f5dea into rust-lang:master May 19, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (92f5dea): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.7% [-0.7%, -0.7%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.7% [-1.7%, -1.7%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 641.544s -> 641.663s (0.02%)

@bvanjoi bvanjoi deleted the fix-issue-109343 branch May 20, 2023 09:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: unreachable compiler/rustc_resolve/src/lib.rs
8 participants