Skip to content
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

resolve: mark it undetermined if single import is not has any bindings #124840

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented May 7, 2024

This issue arises from incorrect resolution updates, for example:

mod a {
    pub mod b {
        pub mod c {}
    }
}

use a::*;

use b::c;
use c as b;

fn main() {}
  1. In the first loop, binding (root, b) is refer to root::a::b due to use a::*.
    1. However, binding (root, c) isn't defined by use b::c during this stage because use c as b falls under the single_imports of (root, b), where the imported_module hasn't been computed yet. This results in marking the path_res for b as Indeterminate.
    2. Then, the imported_module for use c as b will be recorded.
  2. In the second loop, use b::c will be processed again:
    1. Firstly, it attempts to find the path_res for (root, b).
    2. It will iterate through the single_imports of use b::c, encounter use c as b, attempt to resolve c in root, and ultimately return Err(Undetermined), thus passing the iterator.
    3. Use the binding (root, b) -> root::a::b introduced by use a::* and ultimately return root::a::b as the path_res of b.
    4. Then define the binding (root, c) -> root::a::b::c.
  3. Then process use c as b, update the resolution for (root, b) to refer to root::a::b::c, ultimately causing inconsistency.

In my view, step 2.2 has an issue where it should exit early, similar to the behavior when there's no imported_module. Therefore, I've added an attribute called indeterminate to ImportData. This will help us handle only those single imports that have at least one determined binding.

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 May 7, 2024
@rust-log-analyzer

This comment has been minimized.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 7, 2024

#124840 (comment)

CI failed, so mark it as a draft.

reduced:

mod a {
    pub(crate) use crate::S;
}
mod b {
    pub struct S;
}
use b::*;
use self::a::S;

fn main() {}

@bvanjoi bvanjoi marked this pull request as draft May 7, 2024 11:08
@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 7, 2024

Update: I've changed that it now only returns Undetermined when resolving modules.

I'm not sure if there's a better solution or potential edge cases. Regardless, since CI is passing, mark it as ready for review.

@bvanjoi bvanjoi marked this pull request as ready for review May 7, 2024 15:09
@bvanjoi bvanjoi force-pushed the fix-124490 branch 2 times, most recently from 5bdd09b to ebc10a4 Compare May 16, 2024 02:33
@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 24, 2024
@petrochenkov
Copy link
Contributor

  1. It will iterate through the single_imports of use b::c, encounter use c as b, attempt to resolve c in root, and ultimately return Err(Undetermined), thus passing the iterator.

I don't understand, both Err(Undetermined) and the new case introduced in this PR result in

return Err((Undetermined, Weak::No))

and neither of them is "passing the iterator" (if I understand it correctly).

Does resolve_ident_in_module(...) returns Err(Determined) or Ok(binding) maybe, even if single_import.indeterminate is true?
It's not clear to me how this can happen.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 25, 2024

Does resolve_ident_in_module(...) returns Err(Determined)

Yep, it will return Err(Determined) for resolve_ident_in_module(c, root). It might be a typo..

@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 25, 2024

@rustbot ready

@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 May 25, 2024
@petrochenkov
Copy link
Contributor

The PR is now looking nice after the cleanup, but I still don't understand why it works :D

Why is "all bindings undetermined" able to correctly reject something that resolve_ident_in_module is not?
I would expect resolve_ident_in_module to return Undetermined without this additional intervention, that feels pretty arbitrary (especially with the module().is_some() condition).
@rustbot author

@rustbot rustbot 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 28, 2024
@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 29, 2024

The reason why resolve_ident_in_module(c, root) returns Err(Determined) is because it skips the iteration of single_imports. More specifically, the single_import.vis value is None because it was already taken at the entry point. This results in hitting a continue.

@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 29, 2024

Also, I tried moving the single_import.imported_module.get() condition before single_import.vis.get(), but ./x.py build failed to compile. Therefore, I've decided not to solve this problem in this manner. :E

@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 May 29, 2024
@petrochenkov
Copy link
Contributor

I suspect that resolver may be stuck on some valid code after this change, let's check.
@bors try

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2024
@petrochenkov
Copy link
Contributor

r=me with the comment #124840 (comment).
@rustbot author

@rustbot rustbot 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 Jun 3, 2024
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 4, 2024

@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 Jun 4, 2024
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2024

📌 Commit f67a0eb 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 Jun 4, 2024
fmease added a commit to fmease/rust that referenced this pull request Jun 4, 2024
resolve: mark it undetermined if single import is not has any bindings

- Fixes rust-lang#124490
- Fixes rust-lang#125013

This issue arises from incorrect resolution updates, for example:

```rust
mod a {
    pub mod b {
        pub mod c {}
    }
}

use a::*;

use b::c;
use c as b;

fn main() {}
```

1. In the first loop, binding `(root, b)` is refer to `root::a::b` due to `use a::*`.
    1. However, binding `(root, c)` isn't defined by `use b::c` during this stage because `use c as b` falls under the `single_imports` of `(root, b)`, where the `imported_module` hasn't been computed yet. This results in marking the `path_res` for `b` as `Indeterminate`.
    2. Then, the `imported_module` for `use c as b` will be recorded.
2. In the second loop, `use b::c` will be processed again:
    1. Firstly, it attempts to find the `path_res` for `(root, b)`.
    2. It will iterate through the `single_imports` of `use b::c`, encounter `use c as b`, attempt to resolve `c` in `root`, and ultimately return `Err(Undetermined)`, thus passing the iterator.
    3. Use the binding `(root, b)` -> `root::a::b` introduced by `use a::*` and ultimately return `root::a::b` as the `path_res` of `b`.
    4. Then define the binding `(root, c)` -> `root::a::b::c`.
3. Then process `use c as b`, update the resolution for `(root, b)` to refer to `root::a::b::c`, ultimately causing inconsistency.

In my view, step `2.2` has an issue where it should exit early, similar to the behavior when there's no `imported_module`. Therefore, I've added an attribute called `indeterminate` to `ImportData`. This will help us handle only those single imports that have at least one determined binding.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#122192 (Do not try to reveal hidden types when trying to prove Freeze in the defining scope)
 - rust-lang#124840 (resolve: mark it undetermined if single import is not has any bindings)
 - rust-lang#125622 (Winnow private method candidates instead of assuming any candidate of the right name will apply)
 - rust-lang#125871 (Orphanck[old solver]: Consider opaque types to never cover type parameters)
 - rust-lang#125893 (Handle all GVN binops in a single place.)
 - rust-lang#125911 (delete bootstrap build before switching to bumped rustc)
 - rust-lang#125918 (Revert: create const block bodies in typeck via query feeding)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#122192 (Do not try to reveal hidden types when trying to prove Freeze in the defining scope)
 - rust-lang#124840 (resolve: mark it undetermined if single import is not has any bindings)
 - rust-lang#125622 (Winnow private method candidates instead of assuming any candidate of the right name will apply)
 - rust-lang#125871 (Orphanck[old solver]: Consider opaque types to never cover type parameters)
 - rust-lang#125893 (Handle all GVN binops in a single place.)
 - rust-lang#125911 (delete bootstrap build before switching to bumped rustc)
 - rust-lang#125918 (Revert: create const block bodies in typeck via query feeding)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#122192 (Do not try to reveal hidden types when trying to prove Freeze in the defining scope)
 - rust-lang#124840 (resolve: mark it undetermined if single import is not has any bindings)
 - rust-lang#125622 (Winnow private method candidates instead of assuming any candidate of the right name will apply)
 - rust-lang#125871 (Orphanck[old solver]: Consider opaque types to never cover type parameters)
 - rust-lang#125893 (Handle all GVN binops in a single place.)
 - rust-lang#125911 (delete bootstrap build before switching to bumped rustc)
 - rust-lang#125918 (Revert: create const block bodies in typeck via query feeding)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#124840 (resolve: mark it undetermined if single import is not has any bindings)
 - rust-lang#125622 (Winnow private method candidates instead of assuming any candidate of the right name will apply)
 - rust-lang#125648 (Remove unused(?) `~/rustsrc` folder from docker script)
 - rust-lang#125672 (Add more ABI test cases to miri (RFC 3391))
 - rust-lang#125800 (Fix `mut` static task queue in SGX target)
 - rust-lang#125871 (Orphanck[old solver]: Consider opaque types to never cover type parameters)
 - rust-lang#125893 (Handle all GVN binops in a single place.)
 - rust-lang#126008 (Port `tests/run-make-fulldeps/issue-19371` to ui-fulldeps)
 - rust-lang#126032 (Update description of the `IsTerminal` example)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 69a8c13 into rust-lang:master Jun 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup merge of rust-lang#124840 - bvanjoi:fix-124490, r=petrochenkov

resolve: mark it undetermined if single import is not has any bindings

- Fixes rust-lang#124490
- Fixes rust-lang#125013

This issue arises from incorrect resolution updates, for example:

```rust
mod a {
    pub mod b {
        pub mod c {}
    }
}

use a::*;

use b::c;
use c as b;

fn main() {}
```

1. In the first loop, binding `(root, b)` is refer to `root::a::b` due to `use a::*`.
    1. However, binding `(root, c)` isn't defined by `use b::c` during this stage because `use c as b` falls under the `single_imports` of `(root, b)`, where the `imported_module` hasn't been computed yet. This results in marking the `path_res` for `b` as `Indeterminate`.
    2. Then, the `imported_module` for `use c as b` will be recorded.
2. In the second loop, `use b::c` will be processed again:
    1. Firstly, it attempts to find the `path_res` for `(root, b)`.
    2. It will iterate through the `single_imports` of `use b::c`, encounter `use c as b`, attempt to resolve `c` in `root`, and ultimately return `Err(Undetermined)`, thus passing the iterator.
    3. Use the binding `(root, b)` -> `root::a::b` introduced by `use a::*` and ultimately return `root::a::b` as the `path_res` of `b`.
    4. Then define the binding `(root, c)` -> `root::a::b::c`.
3. Then process `use c as b`, update the resolution for `(root, b)` to refer to `root::a::b::c`, ultimately causing inconsistency.

In my view, step `2.2` has an issue where it should exit early, similar to the behavior when there's no `imported_module`. Therefore, I've added an attribute called `indeterminate` to `ImportData`. This will help us handle only those single imports that have at least one determined binding.

r? ``@petrochenkov``
@fmease
Copy link
Member

fmease commented Jun 6, 2024

bors sleepy @bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 6, 2024
@bvanjoi bvanjoi deleted the fix-124490 branch June 6, 2024 08:25
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 7, 2024
mark binding undetermined if target name exist and not obtained

- Fixes rust-lang#124490
- Fixes rust-lang#125013

Following up on rust-lang#124840, I think handling only `target_bindings` is sufficient.

r? `@petrochenkov`
fmease added a commit to fmease/rust that referenced this pull request Jun 8, 2024
mark binding undetermined if target name exist and not obtained

- Fixes rust-lang#124490
- Fixes rust-lang#125013

Following up on rust-lang#124840, I think handling only `target_bindings` is sufficient.

r? `@petrochenkov`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2024
Rollup merge of rust-lang#126065 - bvanjoi:fix-124490, r=petrochenkov

mark binding undetermined if target name exist and not obtained

- Fixes rust-lang#124490
- Fixes rust-lang#125013

Following up on rust-lang#124840, I think handling only `target_bindings` is sufficient.

r? `@petrochenkov`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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:assertion failed: import.imported_module.get().is_none() ICE: inconsistent resolution for an import
7 participants