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

Allow for a missing adt_def in NamePrivacyVisitor. #121482

Merged
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

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
@@ -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),
},  

@@ -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

@nnethercote nnethercote Feb 23, 2024

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
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
…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
…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
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
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
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
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
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
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
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
Development

Successfully merging this pull request may close these issues.

ICE None privacy
4 participants