Refactor #[diagnostic::do_not_recommend] support
#125717
Conversation
rustbot
added
S-waiting-on-review
| mut trait_predicate: ty::Binder<'tcx, ty::TraitPredicate<'tcx>>, | ||
| obligation: &'_ mut PredicateObligation<'tcx>, | ||
| ) -> ty::Binder<'tcx, ty::TraitPredicate<'tcx>> { | ||
| fn apply_do_not_recommend(&self, obligation: &'_ mut PredicateObligation<'tcx>) -> bool { |
There was a problem hiding this comment.
| fn apply_do_not_recommend(&self, obligation: &'_ mut PredicateObligation<'tcx>) -> bool { | |
| fn apply_do_not_recommend(&self, obligation: &mut PredicateObligation<'tcx>) -> bool { |
'_ is redundant
| root_obligation: error.root_obligation.clone(), | ||
| }; | ||
| if self.apply_do_not_recommend(&mut error.obligation) { | ||
| error.code = FulfillmentErrorCode::Select(SelectionError::Unimplemented); |
There was a problem hiding this comment.
I don't think this is correct Select(Unimplemented) is only meant for trait goals, and only if the goal is truly unimplemented (e.g. not ambiguity, not overflow, not a cycle).
I think we need to look at the obligation to re-derive the FulfillmentErrorCode -- perhaps look at how the FulfillmentErrorCode is created in the first place in the old solver?
There was a problem hiding this comment.
For the record, I haven't looked deep enough to see if this is possible to turn into a weird diagnostic (or an ICE), so you may need to do a bit of research yourself.
There was a problem hiding this comment.
I think we need to look at the obligation to re-derive the
FulfillmentErrorCode-- perhaps look at how the FulfillmentErrorCode is created in the first place in the old solver?
I'm not sure if that is easily possible with the old solver. As far as I understand these FulfillmentErrorCode are produced somewhat ad hoc in the code here that does the actual solving:
rust/compiler/rustc_trait_selection/src/traits/fulfill.rs
Lines 745 to 750 in f2e1a3a
At this point we know the following things about the passed obligation:
- If it changes it does contain a unimplemented trait requirement somewhere in the stack as
#[do_not_recommend]an only applied to trait impls (That's asserted at other places). - I think we can only ever end up with changing anything here for
FulfillmentErrorCode::ProjectorFulfillmentErrorCode::Project. In both cases it will change it to the a trait requirement not implemented obligation due to wheredo_not_recommendcan be placed.
Maybe it's a good idea to just explicitly check the FulfillmentErrorCode before if its FulfillmentErrorCode::Select(Unimplementd) or FulfillmentErrorCode::Project so that it always matches that assumptions?
For the record, I haven't looked deep enough to see if this is possible to turn into a weird diagnostic (or an ICE), so you may need to do a bit of research yourself.
At least all of the compiler test suite (via ./x.py test) runs fine with this change. That said I will try to produce a few edge cases with the #[do_not_recommend] attribute added and add them as well to the tests.
There was a problem hiding this comment.
I've tried to think about potential edge cases in the last few days, but I did not came up with something interesting with the current set of restrictions.
We only walk the tree now for FulfillmentErrorCode::Select(Unimplementd) or FulfillmentErrorCode::Project. We only change anything if we hit a trait impl that is marked as #[do_not_recommend] at all. As these are enforced to be on impls I cannot see a different case than just having an Obligation that just says that a certain trait is not implemented.
@rustbot ready
rustbot
added
S-waiting-on-author
This commit refactors the `#[do_not_recommend]` support in the old parser to also apply to projection errors and not only to selection errors. This allows the attribute to be used more widely.
weiznich
force-pushed
the
move/do_not_recommend_to_diganostic_namespace
branch
from
ffe6487
to
f9adc1e
Compare
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#125667 (Silence follow-up errors directly based on error types and regions) - rust-lang#125717 (Refactor `#[diagnostic::do_not_recommend]` support) - rust-lang#125795 (Improve renaming suggestion for names with leading underscores) - rust-lang#125865 (Fix ICE caused by ignoring EffectVars in type inference) - rust-lang#125953 (Streamline `nested` calls.) - rust-lang#125959 (Reduce `pub` exposure in `rustc_mir_build`) - rust-lang#125967 (Split smir `Const` into `TyConst` and `MirConst`) - rust-lang#125968 (Store the types of `ty::Expr` arguments in the `ty::Expr`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#125717 - weiznich:move/do_not_recommend_to_diganostic_namespace, r=compiler-errors Refactor `#[diagnostic::do_not_recommend]` support This commit refactors the `#[do_not_recommend]` support in the old parser to also apply to projection errors and not only to selection errors. This allows the attribute to be used more widely. Part of rust-lang#51992 r? `@compiler-errors` <!-- If this PR is related to an unstable feature or an otherwise tracked effort, please link to the relevant tracking issue here. If you don't know of a related tracking issue or there are none, feel free to ignore this. This PR will get automatically assigned to a reviewer. In case you would like a specific user to review your work, you can assign it to them by using r? <reviewer name> -->
This commit refactors the
#[do_not_recommend]support in the old parser to also apply to projection errors and not only to selection errors. This allows the attribute to be used more widely.Part of #51992
r? @compiler-errors