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

Fix ICE with use clippy::a::b; #83336

Merged
merged 1 commit into from
Mar 22, 2021
Merged

Fix ICE with use clippy::a::b; #83336

merged 1 commit into from
Mar 22, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Mar 21, 2021

Fixes #83317.

@camelid camelid added the A-resolve Area: Path resolution label Mar 21, 2021
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2021
Comment on lines 1 to 5
error[E0433]: failed to resolve: maybe a missing crate `clippy`?
--> $DIR/tool-mod-child.rs:1:5
|
LL | use clippy::a::b;
| ^^^^^^ maybe a missing crate `clippy`?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent with the error for use clippy::a;:

error[E0432]: unresolved import `clippy`
 --> src/lib.rs:1:5
  |
1 | use clippy::a;
  |     ^^^^^^ `clippy` is a tool module, not a module

How should I change this code to get that error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, the clippy::a::b error is the same as the error for rustdoc::a::b.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, they're both tool lints.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change clippy::a and rustdoc::a gave different errors, but now they're the same... weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned lcnr Mar 21, 2021
src/test/ui/imports/tool-mod-child.rs Outdated Show resolved Hide resolved
}
// The error was already reported earlier.
return None;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be

            PathResult::NonModule(path_res) => {
                if no_ambiguity {
                    assert!(import.imported_module.get().is_none());
                }
                // The error was already reported earlier.
                return None;
            }

Res::Err was used to discern between reachable cases and (seemingly) unreachable cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, why do clippy::a and clippy::a::b seem to go through very different code paths? clippy::a didn't trigger the ICE, only clippy::a::b.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what exactly happens there, but both imports and tool attributes are "special" in some regards.

  • Import paths are split into two parts - module (everything except for the last segment, type namespace) and the final segment resolved multiple times in all namespaces.
  • Tool attributes are also split into two parts but in a different way - the first segment resolved as a tool module, and all other segments that are not even resolved (because no definitions for them exist in source code), just automatically assumed to be a tool attribute. (Perhaps the implementation is taking a shortcut here and it could be done better, at least for diagnostics.)

When you combine these two special cases together you get some unique code paths.

@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 21, 2021
@petrochenkov
Copy link
Contributor

This is draft because the error is not quite right.

Could you make a different PR for orthogonal diagnostic changes? They may cause a large diff in tests, depending on the scope.
This PR fixes the ICE and that's enough.

@camelid
Copy link
Member Author

camelid commented Mar 21, 2021

This is draft because the error is not quite right.

Could you make a different PR for orthogonal diagnostic changes? They may cause a large diff in tests, depending on the scope.
This PR fixes the ICE and that's enough.

The issue I was talking about is that use clippy::a; and use clippy::a::b; give different errors:

error[E0433]: failed to resolve: maybe a missing crate `clippy`?
  --> $DIR/tool-mod-child.rs:2:5
   |
LL | use clippy::a::b;
   |     ^^^^^^ maybe a missing crate `clippy`?

error[E0432]: unresolved import `clippy`
  --> $DIR/tool-mod-child.rs:1:5
   |
LL | use clippy::a;
   |     ^^^^^^ maybe a missing crate `clippy`?

But if you think the errors are okay, they're fine for me as well.

@camelid camelid 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 21, 2021
@camelid camelid marked this pull request as ready for review March 21, 2021 21:15
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2021

📌 Commit bfae41d has been approved by petrochenkov

@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 Mar 21, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#80193 (stabilize `feature(osstring_ascii)`)
 - rust-lang#80771 (Make NonNull::as_ref (and friends) return refs with unbound lifetimes)
 - rust-lang#81607 (Implement TrustedLen and TrustedRandomAccess for Range<integer>, array::IntoIter, VecDequeue's iterators)
 - rust-lang#82554 (Fix invalid slice access in String::retain)
 - rust-lang#82686 (Move `std::sys::unix::platform` to `std::sys::unix::ext`)
 - rust-lang#82771 (slice: Stabilize IterMut::as_slice.)
 - rust-lang#83329 (Cleanup LLVM debuginfo module docs)
 - rust-lang#83336 (Fix ICE with `use clippy::a::b;`)
 - rust-lang#83350 (Download a more recent LLVM version if `src/version` is modified)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ea5ba76 into rust-lang:master Mar 22, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 22, 2021
@camelid camelid deleted the tool-mod-ice branch March 23, 2021 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Path resolution S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: 'internal error: entered unreachable code', compiler/rustc_resolve/src/imports.rs:965:70
8 participants