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

Issue 90702 fix: Stop treating some crate loading failures as fatal errors #91045

Merged
merged 3 commits into from
Dec 2, 2021

Conversation

mjptree
Copy link
Contributor

@mjptree mjptree commented Nov 19, 2021

Surface mulitple extern crate resolution errors at a time.

This is achieved by creating a dummy crate, instead of aborting directly after the resolution error. The ExternCrateError has been added to allow propagating the resolution error from rustc_metadata crate to the rustc_resolve with a minimal public surface. The import_extern_crate function is a block that was factored out from build_reduced_graph_for_item for better organization. The only added functionality made to it where the added error handling in the process_extern_crate call. The remaining bits in this function are the same as before.

Resolves #90702

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2021
@rust-log-analyzer

This comment has been minimized.

@petrochenkov petrochenkov changed the title Issue 90702 fix Issue 90702 fix: Stop treating some crate loading failures as fatal errors Nov 20, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added A-resolve Area: Path resolution T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 20, 2021
src/test/ui/crate-loading/missing-std.rs Outdated Show resolved Hide resolved
src/test/ui/extern/extern-crate-path-to-missing.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/build_reduced_graph.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/locator.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/creader.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Two general comments:

  • Could you move the extraction of fn build_reduced_graph_for_extern_crate to a separate commit that only moves code and doesn't do anything else? (It can actually be the very first commit.)
  • ExternCrateError looks unnecessary, I'd keep the old scheme and report the error from fn resolve_crate.

@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 Nov 25, 2021
@mjptree
Copy link
Contributor Author

mjptree commented Nov 26, 2021

I restructered the PR into three commits grouped as thematically as possible incorporating all your suggestions. I hope this makes it easier to disambiguate.

EDIT: Wait, I messed up the commits. The first 2 commits were somehow squashed. I'll submit a new one.

@mjptree
Copy link
Contributor Author

mjptree commented Nov 26, 2021

@rustbot label -S-waiting-on-author +S-waiting-on-review

@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 Nov 26, 2021
compiler/rustc_metadata/src/creader.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/creader.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/creader.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/locator.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/build_reduced_graph.rs Outdated 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 Nov 27, 2021
@mjptree
Copy link
Contributor Author

mjptree commented Nov 28, 2021

I'm aware the last commit currently fails 2 tests. But I'd like you to have a look at it whether these changes are what you had in mind.

Also, if you look at the two tests src/test/ui/crate-loading/invalid-rlib.rs and src/test/ui/rust-2018/uniform-paths/deadlock.rs it would be helpful if you have any suggestions how my changes caused these changes in error output. I'll also continue investigating further after work today.

@rustbot label -S-waiting-on-author +S-waiting-on-review

@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 Nov 28, 2021
@rust-log-analyzer

This comment has been minimized.

compiler/rustc_metadata/src/creader.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/creader.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Also, if you look at the two tests src/test/ui/crate-loading/invalid-rlib.rs and src/test/ui/rust-2018/uniform-paths/deadlock.rs it would be helpful if you have any suggestions how my changes caused these changes in error output.

IIRC, imports may attempt to load crates for the second time during import validation (which we now reach due to not producing a fatal error), this looks like the reason for the invalid-rlib.rs change.
The extra messages in deadlock.rs also look like an output from import validation.

You can attempt to avoid these extra messages for already erroneous imports (the relevant code should be somewhere inside fn finalize_imports), but it's a task for a separate PR.

@bors
Copy link
Contributor

bors commented Nov 29, 2021

📌 Commit 2c9d9b4 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 Nov 29, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 30, 2021
…nkov

Issue 90702 fix: Stop treating some crate loading failures as fatal errors

Surface mulitple `extern crate` resolution errors at a time.

This is achieved by creating a dummy crate, instead of aborting directly after the resolution error. The `ExternCrateError` has been added to allow propagating the resolution error from `rustc_metadata` crate to the `rustc_resolve` with a minimal public surface. The `import_extern_crate` function is a block that was factored out from `build_reduced_graph_for_item` for better organization. The only added functionality made to it where the added error handling in the `process_extern_crate` call. The remaining bits in this function are the same as before.

Resolves rust-lang#90702

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 30, 2021
…nkov

Issue 90702 fix: Stop treating some crate loading failures as fatal errors

Surface mulitple `extern crate` resolution errors at a time.

This is achieved by creating a dummy crate, instead of aborting directly after the resolution error. The `ExternCrateError` has been added to allow propagating the resolution error from `rustc_metadata` crate to the `rustc_resolve` with a minimal public surface. The `import_extern_crate` function is a block that was factored out from `build_reduced_graph_for_item` for better organization. The only added functionality made to it where the added error handling in the `process_extern_crate` call. The remaining bits in this function are the same as before.

Resolves rust-lang#90702

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

Mmh, this might cause this test failure here: #91405 (comment)
but I'm not 100% certain.
@bors rollup=iffy

@petrochenkov
Copy link
Contributor

Yes, looks like this PR is the reason, ui/extern-flag/empty-extern-arg.rs has target-dependent output.

ui\panic-handler\weak-lang-item.rs is a similar test and it uses // needs-unwind to disable the test on targets producing different diagnostics.

@petrochenkov
Copy link
Contributor

@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 Dec 1, 2021
@mjptree
Copy link
Contributor Author

mjptree commented Dec 1, 2021

ui\panic-handler\weak-lang-item.rs is a similar test and it uses // needs-unwind to disable the test on targets producing different diagnostics.

I'm trying the same fix then here, ignoring this particular test for panic=abort and emscripten targets.

@rustbot label -S-waiting-on-author +S-waiting-on-review

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

@bors r+

@bors
Copy link
Contributor

bors commented Dec 2, 2021

📌 Commit 2ca9333 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 Dec 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2021
…askrgr

Rollup of 4 iffy pull requests

Successful merges:

 - rust-lang#89234 (Disallow non-c-like but "fieldless" ADTs from being casted to integer if they use arbitrary enum discriminant)
 - rust-lang#91045 (Issue 90702 fix: Stop treating some crate loading failures as fatal errors)
 - rust-lang#91394 (Bump stage0 compiler)
 - rust-lang#91411 (Enable svh tests on msvc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 87ca333 into rust-lang:master Dec 2, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 2, 2021
@mjptree mjptree deleted the issue-90702-fix branch December 2, 2021 22:09
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. 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.

Surface multiple "can't find crate" errors at a time
9 participants