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

Always emit unresolved import errors and hide unused import lint #64054

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Sep 1, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2019
src/librustc_resolve/resolve_imports.rs Outdated Show resolved Hide resolved
src/librustc_resolve/resolve_imports.rs Outdated Show resolved Hide resolved
src/librustc_resolve/resolve_imports.rs Outdated Show resolved Hide resolved
src/librustc_resolve/resolve_imports.rs Outdated Show resolved Hide resolved
src/librustc_resolve/resolve_imports.rs Show resolved Hide resolved
@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 Sep 3, 2019
@estebank
Copy link
Contributor Author

estebank commented Sep 3, 2019

@petrochenkov I originally had a similar intuition to what you mentioned, but in practice I found that we only have hard failures on the first time a module is not found. After that we always get Indeterminate, which makes 1) some self.finalize_import(indeterminate_import).is_some(), but not all, and 2) subsequent import errors are silenced. I don't fully understand why this is the case, as this behavior doesn't jive with what I would have expected the logic to do. The presented code is what I arrived at empirically to.

@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 Sep 3, 2019
@petrochenkov
Copy link
Contributor

(I need to make some experiments.)

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 7, 2019

Mostly superseded by #64265.
I didn't include the "report all the indeterminate imports instead of one" part (which is also reasonable, but orthogonal), only the "unused" part.

@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 Sep 7, 2019
@estebank
Copy link
Contributor Author

estebank commented Sep 7, 2019

Is the behavior of "report all the indeterminate imports as unresolved" correct? I can publish a PR after yours gets merged to do so. I think it is useful to have but not critical if it could lead to bad false positives (due to dependencies: use foo::bar; use bar::baz;).

@petrochenkov
Copy link
Contributor

I don't think it's correct as implemented because finalize_import doesn't always return Some(err) for indeterminate imports, but they should be reported anyway.

What I think we should do is something roughly like this:

for import in determinate_imports.chain(indeterminate_imports) {
    if let Some(err) = finalize_import(import) {
        report_error(err)
    } else if is_indeterminate(import) {
        report_error("cannot determine resolution")
    }
}

@petrochenkov
Copy link
Contributor

due to dependencies: use foo::bar; use bar::baz;

AFAIK, this case should be already covered by marking use foo::bar; as successfully resolved to Res::Err.

@bors
Copy link
Contributor

bors commented Sep 8, 2019

☔ The latest upstream changes (presumably #64281) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank estebank 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 Sep 8, 2019
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Sep 9, 2019

📌 Commit 9a56187 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 Sep 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 9, 2019
… r=petrochenkov

Always emit unresolved import errors and hide unused import lint

Fix rust-lang#63724.

r? @petrochenkov
bors added a commit that referenced this pull request Sep 9, 2019
Rollup of 5 pull requests

Successful merges:

 - #63806 (Upgrade rand to 0.7)
 - #64054 (Always emit unresolved import errors and hide unused import lint)
 - #64279 (Bump RLS and Rustfmt submodules to use rustc-ap-* v583)
 - #64317 (Update LLVM submodule)
 - #64320 (Update version of `rustc-std-workspace-*` crates)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Sep 10, 2019
Rollup of 5 pull requests

Successful merges:

 - #63806 (Upgrade rand to 0.7)
 - #64054 (Always emit unresolved import errors and hide unused import lint)
 - #64279 (Bump RLS and Rustfmt submodules to use rustc-ap-* v583)
 - #64317 (Update LLVM submodule)
 - #64320 (Update version of `rustc-std-workspace-*` crates)

Failed merges:

r? @ghost
@bors bors merged commit 9a56187 into rust-lang:master Sep 10, 2019
@estebank estebank deleted the unused-import-is-to-eager branch November 9, 2023 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Silence unused_import warning on unresolved import
4 participants