Actually use the `#[do_not_recommend]` attribute if present #124708

weiznich · 2024-05-04T12:41:58Z

This change tweaks the error message generation to actually use the #[do_not_recommend] attribute if present by just skipping the marked trait impl in favour of the parent impl. It also adds a compile test for this behaviour. Without this change the test would output the following error:

error[E0277]: the trait bound `&str: Expression` is not satisfied
  --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:53:15
   |
LL |     SelectInt.check("bar");
   |               ^^^^^ the trait `Expression` is not implemented for `&str`, which is required by `&str: AsExpression<Integer>`
   |
   = help: the following other types implement trait `Expression`:
             Bound<T>
             SelectInt
note: required for `&str` to implement `AsExpression<Integer>`
  --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:26:13
   |
LL | impl<T, ST> AsExpression<ST> for T
   |             ^^^^^^^^^^^^^^^^     ^
LL | where
LL |     T: Expression<SqlType = ST>,
   |        ------------------------ unsatisfied trait bound introduced here

Note how that mentions &str: Expression before and now mentions &str: AsExpression<Integer> instead which is much more helpful for users.

Open points for further changes before stabilization:

We likely want to move the attribute to the #[diagnostic] namespace to relax the guarantees given?
How does it interact with the new trait solver?

r? @estebank

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

weiznich · 2024-05-04T15:37:38Z

The tests seem to pass locally with ./x.py test. Any idea why might be the problem that causes the to fail on CI?

compiler-errors · 2024-05-04T16:08:43Z

Can you open a separate PR to move do_not_recommend to the diagnostic namespace? There's no reason for it to be a top-level attribute now that we have it.

compiler-errors · 2024-05-04T16:09:49Z

How does it interact with the new trait solver?

I'll put up a PR to implement do_not_recommend in the new solver. It's basically a one-liner in the new solver.

compiler-errors · 2024-05-04T22:05:01Z

Also, this line should fix your ICEs:

https://github.com/rust-lang/rust/pull/124717/files#diff-09c366d3ad3ec9a42125253b610ca83cad6b156aa2a723f6c7e83eddef7b1e8fR521

I believe you should enable debug assertions in your local compiler:

(config.toml)

rust.debug = true

compiler-errors · 2024-05-04T22:13:50Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

@@ -464,6 +464,22 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
                                root_obligation,
                            )
                        } else {
+                            let mut trait_predicate = trait_predicate;
+                            while matches!(


This doesn't take into account obligation stacks that are like A -requires> B -requires> C where A and B come from impls marked with #[do_not_recommend] but C does not.

I believe we need to basically need to pop #[do_not_recommend]s until there are none left in the stack anywhere, even if it isn't the last obligation in the stack.

For example, consider we put #[do_not_recommend] on this IntoIterator blanket, and then consider the following code:

trait OtherTrait {} struct It<T>(T); impl<T> Iterator for It<T> where T: OtherTrait { type Item = (); fn next(&mut self) -> Option<Self::Item> { todo!() } } fn main() { for x in It(()) {} }

I would not expect the compiler to mention (): OtherTrait, but it would in this case, right? IMO it should just mention It<()>: IntoIterator.

From another perspective, considering the full tree of solving a goal like It<()>: IntoIterator, we simply wouldn't walk into the obligations that come from considering the blanket implementation marked with #[do_not_recommend].

I know that such a tree is kind of inverted from our perspective here, but that's only a consequence of how we record derived obligations currently, and shouldn't limit our implementation IMO.

Doesn't that loop not already do that?

We reset the oblication.cause.code() field in line 475, so we should pop these obligations as long as we encounter ones that are marked with #[do_not_recommend], right?

Not if the top obligation is not a ImplDerivedObligation, right? For example, a BuiltinDerivedObligation that is stacked onto a ImplDerivedObligation (e.g. via a T: Send where clause).

Thanks for the clarification. That sounds like I should first do some reading on what's actually represented by the Obligation struct. Is there any resource on this other than the source code?

Hopefully it's now closer to what you imagined, although I'm still not sure about the details of Obligation.

weiznich · 2024-05-05T09:07:08Z

Can you open a separate PR to move do_not_recommend to the diagnostic namespace? There's no reason for it to be a top-level attribute now that we have it.

I will open a PR for that next week. That PR should keep the #![feature(do_not_recommend)] but just move the attribute to #[diagnostic::on_unimplemented], right?

Also, this line should fix your ICEs:

https://github.com/rust-lang/rust/pull/124717/files#diff-09c366d3ad3ec9a42125253b610ca83cad6b156aa2a723f6c7e83eddef7b1e8fR521

I rebased the PR to include your change. Hopefully that fixes the issue.

weiznich · 2024-05-05T09:12:02Z

Seems like the CI is currently failing for unrelated reasons (Network error while downloading LLVM?)

compiler-errors · 2024-05-05T13:43:29Z

~~You messed up your commit history and have a bunch of merges.~~(edit: see below)

compiler-errors · 2024-05-05T14:51:18Z

Actually, sorry, now that I look on this on my own web browser (not phone) I see there was a force-push to the master branch. Please do still rebase to fix that, though.

weiznich · 2024-05-06T05:44:02Z

Actually, sorry, now that I look on this on my own web browser (not phone) I see there was a force-push to the master branch. Please do still rebase to fix that, though.

Whatever I managed to do there…
I've cleaned up that mess now.

estebank · 2024-05-07T22:34:31Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

-                            (trait_predicate, &obligation)
+                            let mut temp_trait_predicate = trait_predicate;
+                            let mut loop_code = obligation.cause.code().clone();
+                            loop{


Can you run ./x.py fmt? This might not be the only thing that CI will complain about.

Suggested change

loop{

loop {

It's probably that rustfmt bailed in this method because of something else, like a string that's too long or a where clause somewhere that it can't handle, because otherwise tidy would have failed.

Probably the matches!

estebank · 2024-05-07T22:50:50Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

+                            let mut temp_trait_predicate = trait_predicate;
+                            let mut loop_code = obligation.cause.code().clone();
+                            loop{
+                                let (code, base) = loop_code.peel_derives_with_predicate();
+                                if matches!(loop_code, ObligationCauseCode::ImplDerivedObligation(ref c) if tcx.has_attr(c.impl_or_alias_def_id, sym::do_not_recommend)) {
+                                    if let Some(base) = base {
+                                        let code = code.clone();
+                                        obligation.cause.map_code(|_| code);
+                                        obligation.predicate = ty::PredicateKind::Clause(ty::ClauseKind::Trait(base.skip_binder().clone())).to_predicate(tcx);
+                                        temp_trait_predicate = base.clone();
+                                        trait_predicate = base;
+                                    }
+                                }
+                                if loop_code == *code {
+                                    break;
+                                } else {
+                                    loop_code = code.clone();
+                                }
+                            }


Can we add a comment in front of this? When skimming this code it will be really hard to figure out what's going on without paying a lot of attention. What, and more importantly, why.

It really should be extracted into a helper function, I think.

I've fixed the formatting and extracted it into a helper function in https://github.com/rust-lang/rust/compare/eca373a073222883772da9c30616b3c746e3d6f2..f0279abd052e1ec4fa39ab71edbd7484dbc84c7b

compiler-errors

I'd like to take another look at this later before it lands.

compiler-errors · 2024-05-07T22:54:49Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

+                            let mut temp_trait_predicate = trait_predicate;
+                            let mut loop_code = obligation.cause.code().clone();
+                            loop{
+                                let (code, base) = loop_code.peel_derives_with_predicate();


I don't think it's correct to use peel_derives_with_predicate here. We need to be peeling these one at a time.

Consider a where clause where the first impl we have to go through isn't marked with #[do_not_recommend] but the second is.

Hopefully it's more correct with https://github.com/rust-lang/rust/compare/eca373a073222883772da9c30616b3c746e3d6f2..f0279abd052e1ec4fa39ab71edbd7484dbc84c7b now. If that's not the case I would appreciate bit more guidance of how to address that.

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

compiler-errors

I'd like to see more tests that exercise the cases that I've alluded to previously in review, since I'm not totally certain if the loop you've implemented is right 🤔. For example, here's one test that I'd like to see works correctly:

#![feature(do_not_recommend)]

trait Root {}
trait DontRecommend {}
trait Other {}

#[do_not_recommend]
impl<T> Root for T where T: DontRecommend {}

impl<T> DontRecommend for T where T: Other {}

fn needs_root<T: Root>() {}

fn main() {
    needs_root::<()>();
    // Should not mention `(): Other` or `(): DontRecommend`
}

I'd also like to see if you can think of any other tests...

You should also be able to compare the tests against the implementation in the new solver by using revisions, tagging your tests with:

//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

They should look the same.

compiler-errors · 2024-05-12T03:20:49Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

+                use std::ops::Deref;
+
+                let code = c.derived.parent_code.deref().clone();


nit:

Suggested change

use std::ops::Deref;

let code = c.derived.parent_code.deref().clone();

let code = (*c.derived.parent_code).clone();

compiler-errors · 2024-05-12T03:21:23Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

@@ -1000,6 +1001,51 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
        err.emit()
    }

+    fn apply_do_not_recommend<'b>(
+        &self,
+        trait_predicate: &'_ mut ty::Binder<'b, ty::TraitPredicate<'tcx>>,


This 'b lifetime is unnecessary. It should always be 'tcx. If not, then there's almost certainly some other lifetime in the caller that needs to be turned into a 'tcx.

compiler-errors · 2024-05-12T03:26:27Z

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs

+            }
+        }
+
+        // later we need to walk the parent tree


Why does this need to be a separate loop? The logic that applies above (we need to peel until we find the obligation preceding the first¹ do_not_recommend in the stack, if it exists) should also apply below -- should be able to just have one loop.

Footnotes

first from the root obligation, or last, if we're going down from the leaf like we're doing here. ↩

bors · 2024-05-18T19:26:47Z

⌛ Testing commit 023ba29 with merge bf6e74a...

… r=compiler-errors,estebank Actually use the `#[do_not_recommend]` attribute if present This change tweaks the error message generation to actually use the `#[do_not_recommend]` attribute if present by just skipping the marked trait impl in favour of the parent impl. It also adds a compile test for this behaviour. Without this change the test would output the following error: ``` error[E0277]: the trait bound `&str: Expression` is not satisfied --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:53:15 | LL | SelectInt.check("bar"); | ^^^^^ the trait `Expression` is not implemented for `&str`, which is required by `&str: AsExpression<Integer>` | = help: the following other types implement trait `Expression`: Bound<T> SelectInt note: required for `&str` to implement `AsExpression<Integer>` --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:26:13 | LL | impl<T, ST> AsExpression<ST> for T | ^^^^^^^^^^^^^^^^ ^ LL | where LL | T: Expression<SqlType = ST>, | ------------------------ unsatisfied trait bound introduced here ``` Note how that mentions `&str: Expression` before and now mentions `&str: AsExpression<Integer>` instead which is much more helpful for users. Open points for further changes before stabilization: * We likely want to move the attribute to the `#[diagnostic]` namespace to relax the guarantees given? * How does it interact with the new trait solver? r? `@estebank`

bors · 2024-05-18T19:34:18Z

💔 Test failed - checks-actions

compiler-errors · 2024-05-18T20:28:34Z

@weiznich: to_predicate has been renamed to upcast in a recent commit of mine. Should be a quick fix, so I will just push it here and re-r+ it.

compiler-errors · 2024-05-18T20:35:06Z

@bors r=compiler-errors,estebank

bors · 2024-05-18T20:35:09Z

📌 Commit a08e09f has been approved by compiler-errors,estebank

It is now in the queue for this repository.

compiler-errors · 2024-05-18T20:53:37Z

Ah, tests need blessing too. I'll leave this up to you then. r=me when rebased.

@bors r-

This change tweaks the error message generation to actually use the `#[do_not_recommend]` attribute if present by just skipping the marked trait impl in favour of the parent impl. It also adds a compile test for this behaviour. Without this change the test would output the following error: ``` error[E0277]: the trait bound `&str: Expression` is not satisfied --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:53:15 | LL | SelectInt.check("bar"); | ^^^^^ the trait `Expression` is not implemented for `&str`, which is required by `&str: AsExpression<Integer>` | = help: the following other types implement trait `Expression`: Bound<T> SelectInt note: required for `&str` to implement `AsExpression<Integer>` --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:26:13 | LL | impl<T, ST> AsExpression<ST> for T | ^^^^^^^^^^^^^^^^ ^ LL | where LL | T: Expression<SqlType = ST>, | ------------------------ unsatisfied trait bound introduced here ``` Note how that mentions `&str: Expression` before and now mentions `&str: AsExpression<Integer>` instead which is much more helpful for users. Open points for further changes before stabilization: * We likely want to move the attribute to the `#[diagnostic]` namespace to relax the guarantees given? * How does it interact with the new trait solver?

weiznich · 2024-05-19T06:32:42Z

I've adjusted the tests. It should work now.

I will try to open a PR for moving the attribute to the #[diagnostic] namespace next week after this is merged. The branch for that already exists, but I need to rebase it on top of the final version accepted here.

compiler-errors · 2024-05-19T13:59:54Z

@bors r=compiler-errors,estebank

bors · 2024-05-19T13:59:57Z

📌 Commit 9b45cfd has been approved by compiler-errors,estebank

It is now in the queue for this repository.

…mpiler-errors Rollup of 7 pull requests Successful merges: - rust-lang#123709 (Update documentation related to the recent cmd.exe fix) - rust-lang#124304 (revise the interpretation of ReadDir for HermitOS) - rust-lang#124708 (Actually use the `#[do_not_recommend]` attribute if present) - rust-lang#125252 (Add `#[inline]` to float `Debug` fallback used by `cfg(no_fp_fmt_parse)`) - rust-lang#125261 (crashes: add more) - rust-lang#125270 (Followup fixes from rust-lang#123344) - rust-lang#125275 (Migrate `run-make/rustdoc-scrape-examples-test` to new `rmake.rs`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#124708 - weiznich:implement_do_not_recommend, r=compiler-errors,estebank Actually use the `#[do_not_recommend]` attribute if present This change tweaks the error message generation to actually use the `#[do_not_recommend]` attribute if present by just skipping the marked trait impl in favour of the parent impl. It also adds a compile test for this behaviour. Without this change the test would output the following error: ``` error[E0277]: the trait bound `&str: Expression` is not satisfied --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:53:15 | LL | SelectInt.check("bar"); | ^^^^^ the trait `Expression` is not implemented for `&str`, which is required by `&str: AsExpression<Integer>` | = help: the following other types implement trait `Expression`: Bound<T> SelectInt note: required for `&str` to implement `AsExpression<Integer>` --> /home/weiznich/Documents/rust/rust/tests/ui/diagnostic_namespace/do_not_recommend.rs:26:13 | LL | impl<T, ST> AsExpression<ST> for T | ^^^^^^^^^^^^^^^^ ^ LL | where LL | T: Expression<SqlType = ST>, | ------------------------ unsatisfied trait bound introduced here ``` Note how that mentions `&str: Expression` before and now mentions `&str: AsExpression<Integer>` instead which is much more helpful for users. Open points for further changes before stabilization: * We likely want to move the attribute to the `#[diagnostic]` namespace to relax the guarantees given? * How does it interact with the new trait solver? r? `@estebank`

rustbot assigned estebank May 4, 2024

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 May 4, 2024

weiznich commented May 4, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs Outdated Show resolved Hide resolved

weiznich force-pushed the implement_do_not_recommend branch from ee93d22 to a6ceae9 Compare May 4, 2024 15:15

compiler-errors reviewed May 4, 2024

View reviewed changes

weiznich force-pushed the implement_do_not_recommend branch from a6ceae9 to 4b50243 Compare May 5, 2024 08:56

This comment has been minimized.

Sign in to view

weiznich force-pushed the implement_do_not_recommend branch from 4b50243 to 240f678 Compare May 5, 2024 09:05

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the master branch from 69ffc0d to 02f7806 Compare May 5, 2024 12:01

weiznich force-pushed the implement_do_not_recommend branch from 240f678 to 1917f3a Compare May 6, 2024 05:42

weiznich force-pushed the implement_do_not_recommend branch from 1917f3a to eca373a Compare May 7, 2024 18:15

estebank reviewed May 7, 2024

View reviewed changes

estebank approved these changes May 7, 2024

View reviewed changes

compiler-errors requested changes May 7, 2024

View reviewed changes

compiler-errors self-assigned this May 7, 2024

weiznich force-pushed the implement_do_not_recommend branch from eca373a to f0279ab Compare May 10, 2024 08:05

compiler-errors reviewed May 12, 2024

View reviewed changes

weiznich force-pushed the implement_do_not_recommend branch from f0279ab to ec1dd21 Compare May 15, 2024 13:07

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 May 18, 2024

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 18, 2024

compiler-errors force-pushed the implement_do_not_recommend branch from 023ba29 to a08e09f Compare May 18, 2024 20:34

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 May 18, 2024

This comment has been minimized.

Sign in to view

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 May 18, 2024

weiznich force-pushed the implement_do_not_recommend branch from a08e09f to 9b45cfd Compare May 19, 2024 06:29

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2024

compiler-errors mentioned this pull request May 19, 2024

Rollup of 7 pull requests #125280

Merged

bors merged commit e940ca7 into rust-lang:master May 19, 2024
6 checks passed

rustbot added this to the 1.80.0 milestone May 19, 2024

		use std::ops::Deref;

		let code = c.derived.parent_code.deref().clone();

Actually use the #[do_not_recommend] attribute if present #124708

Actually use the #[do_not_recommend] attribute if present #124708

Conversation

weiznich commented May 4, 2024

weiznich commented May 4, 2024

compiler-errors commented May 4, 2024

compiler-errors commented May 4, 2024

compiler-errors commented May 4, 2024

compiler-errors May 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors May 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

weiznich commented May 5, 2024

This comment has been minimized.

weiznich commented May 5, 2024

compiler-errors commented May 5, 2024 • edited

compiler-errors commented May 5, 2024

weiznich commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors May 12, 2024 • edited

Choose a reason for hiding this comment

Footnotes

bors commented May 18, 2024

This comment has been minimized.

bors commented May 18, 2024

compiler-errors commented May 18, 2024 • edited

compiler-errors commented May 18, 2024

bors commented May 18, 2024

This comment has been minimized.

compiler-errors commented May 18, 2024

weiznich commented May 19, 2024

compiler-errors commented May 19, 2024

bors commented May 19, 2024

Actually use the `#[do_not_recommend]` attribute if present #124708

Actually use the `#[do_not_recommend]` attribute if present #124708

compiler-errors May 4, 2024 •

edited

compiler-errors May 5, 2024 •

edited

compiler-errors commented May 5, 2024 •

edited

estebank May 7, 2024 •

edited

compiler-errors May 12, 2024 •

edited

compiler-errors commented May 18, 2024 •

edited