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.

Sign up for GitHub

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

Jump to bottom

Allow for a missing adt_def in NamePrivacyVisitor. #121482

Merged
merged 1 commit into from Feb 23, 2024
Merged

Allow for a missing adt_def in NamePrivacyVisitor. #121482

merged 1 commit into from Feb 23, 2024

Conversation

nnethercote
Copy link
Contributor

This was caused by 72b172b in #121206. That commit removed an early return from analysis when there are stashed errors. As a result, it's possible to reach privacy analysis when there are stashed errors, which means more code paths can be reached. One such code path was handled in that commit, where a span_bug was changed to a span_delayed_bug.

This commit handles another such code path uncovered by fuzzing, in much the same way.

Fixes #121455.

r? @oli-obk

@nnethercote
Allow for a missing adt_def in NamePrivacyVisitor.
21bb1a4 
This was caused by 72b172b in rust-lang#121206. That commit removed an early
return from `analysis` when there are stashed errors. As a result, it's
possible to reach privacy analysis when there are stashed errors, which
means more code paths can be reached. One such code path was handled in
that commit, where a `span_bug` was changed to a `span_delayed_bug`.

This commit handles another such code path uncovered by fuzzing, in much
the same way.

Fixes rust-lang#121455.
@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 Feb 23, 2024
oli-obk
oli-obk reviewed Feb 23, 2024
View reviewed changes
compiler/rustc_privacy/src/lib.rs
@@ -988,7 +988,10 @@ impl<'tcx> Visitor<'tcx> for NamePrivacyVisitor<'tcx> {
fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) {
if let hir::ExprKind::Struct(qpath, fields, ref base) = expr.kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we ge an exprkind struct in the example you gave?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The range somehow becomes a struct. Here's partial output with -Zunpretty=hir-tree:

Expr {
    hir_id: HirId(DefId(0:3 ~ unreachable_issue_121455[470e]::test).8),
    kind: Struct(
        LangItem(
            Range,
            tests/ui/privacy/unreachable-issue-121455.rs:3:14: 3:18 (#0),
        ),  
        [   
            ExprField {
                ... 
            },  
            ExprField {
                ... 
            },  
        ],  
        None,
    ),  
    span: tests/ui/privacy/unreachable-issue-121455.rs:3:14: 3:18 (#0),
},

oli-obk
oli-obk reviewed Feb 23, 2024
View reviewed changes
compiler/rustc_privacy/src/lib.rs
@@ -988,7 +988,10 @@ impl<'tcx> Visitor<'tcx> for NamePrivacyVisitor<'tcx> {
fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) {
if let hir::ExprKind::Struct(qpath, fields, ref base) = expr.kind {
let res = self.typeck_results().qpath_res(qpath, expr.hir_id);
let adt = self.typeck_results().expr_ty(expr).ty_adt_def().unwrap();
let Some(adt) = self.typeck_results().expr_ty(expr).ty_adt_def() else {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this error case only reachable for ty::Error? If so, I'm wondering if we should return Resut<Option<AdtDef>, ErrorGuaranteed>. The churn may be useless, or it exposes more such cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, and yes, the result of self.typeck_results().expr_ty(expr) is ty::Error. I don't know if that's the only thing that can reach here; it's the only thing we've seen reach here because this is the first time the the adt_def has ever failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for ty_adt_def's return type... it has ~100 call sites. I looked through some of them, I don't see any patterns where having an ErrorGuaranteed would be useful. There were a couple of places where a normal error (not a span_delayed_bug) was emitted in the case where it returned None.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 23, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 23, 2024

📌 Commit 21bb1a4 has been approved by oli-obk

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 Feb 23, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 23, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
@bors
Auto merge of rust-lang#121491 - matthiaskrgr:rollup-wkzqawy, r=matth…
fdf8b29 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121434 (Fix rust-lang#121208 fallout)
 - rust-lang#121471 (When encountering `<&T as Clone>::clone(x)` because `T: Clone`, suggest `#[derive(Clone)]`)
 - rust-lang#121476 (remove `llvm.assertions=true` in compiler profile)
 - rust-lang#121479 (fix generalizer unsoundness)
 - rust-lang#121480 (Fix more rust-lang#121208 fallout)
 - rust-lang#121482 (Allow for a missing `adt_def` in `NamePrivacyVisitor`.)
 - rust-lang#121484 (coverage: Use variable name `this` in `CoverageGraph::from_mir`)
 - rust-lang#121487 (Explicitly call `emit_stashed_diagnostics`.)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
@bors
Auto merge of rust-lang#121491 - matthiaskrgr:rollup-wkzqawy, r=matth…
52cea08 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#121434 (Fix rust-lang#121208 fallout)
 - rust-lang#121471 (When encountering `<&T as Clone>::clone(x)` because `T: Clone`, suggest `#[derive(Clone)]`)
 - rust-lang#121476 (remove `llvm.assertions=true` in compiler profile)
 - rust-lang#121479 (fix generalizer unsoundness)
 - rust-lang#121480 (Fix more rust-lang#121208 fallout)
 - rust-lang#121482 (Allow for a missing `adt_def` in `NamePrivacyVisitor`.)
 - rust-lang#121484 (coverage: Use variable name `this` in `CoverageGraph::from_mir`)
 - rust-lang#121487 (Explicitly call `emit_stashed_diagnostics`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 52805b0 into rust-lang:master Feb 23, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
@rust-timer
Unrolled build for rust-lang#121482
a72b236 
Rollup merge of rust-lang#121482 - nnethercote:fix-121455, r=oli-obk

Allow for a missing `adt_def` in `NamePrivacyVisitor`.

This was caused by 72b172b in rust-lang#121206. That commit removed an early return from `analysis` when there are stashed errors. As a result, it's possible to reach privacy analysis when there are stashed errors, which means more code paths can be reached. One such code path was handled in that commit, where a `span_bug` was changed to a `span_delayed_bug`.

This commit handles another such code path uncovered by fuzzing, in much the same way.

Fixes rust-lang#121455.

r? `@oli-obk`
@nnethercote nnethercote deleted the fix-121455 branch February 25, 2024 21:08
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 27, 2024
@nnethercote
Mark some once-again-unreachable paths as unreachable.
3411625 
This undoes the changes from rust-lang#121482 and rust-lang#121586, now that stashed errors
will trigger more early aborts.
@nnethercote nnethercote mentioned this pull request Feb 27, 2024
Merged
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 27, 2024
@nnethercote
Mark some once-again-unreachable paths as unreachable.
a180ae4 
This undoes the changes from rust-lang#121482 and rust-lang#121586, now that stashed errors
will trigger more early aborts.
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 27, 2024
@nnethercote
Mark some once-again-unreachable paths as unreachable.
5e9cd1f 
This undoes the changes from rust-lang#121482 and rust-lang#121586, now that stashed errors
will trigger more early aborts.
nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 29, 2024
@nnethercote
Mark some once-again-unreachable paths as unreachable.
b4e9f93 
This undoes the changes from rust-lang#121482 and rust-lang#121586, now that stashed errors
will trigger more early aborts.
nnethercote added a commit to nnethercote/rust that referenced this pull request Mar 1, 2024
@nnethercote
Mark some once-again-unreachable paths as unreachable.
e1da990 
This undoes the changes from rust-lang#121482 and rust-lang#121586, now that stashed errors
will trigger more early aborts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
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
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.

ICE None privacy
4 participants
@nnethercote @oli-obk @bors @rustbot