From c121a26ab978681db9133865089a67a3d9723eda Mon Sep 17 00:00:00 2001 From: Tyler Mandry Date: Tue, 27 Feb 2024 17:08:48 -0800 Subject: [PATCH] Split refining_impl_trait lint into _reachable, _internal variants --- compiler/rustc_hir_analysis/messages.ftl | 1 + .../src/check/compare_impl_item/refine.rs | 33 +++---- compiler/rustc_hir_analysis/src/errors.rs | 1 + compiler/rustc_lint/src/lib.rs | 6 ++ compiler/rustc_lint_defs/src/builtin.rs | 90 ++++++++++++++++--- src/tools/lint-docs/src/groups.rs | 4 + .../async-example-desugared-boxed.stderr | 2 + .../async-example-desugared-manual.stderr | 2 + .../bad-item-bound-within-rpitit.stderr | 3 +- tests/ui/impl-trait/in-trait/foreign.rs | 30 ++++++- tests/ui/impl-trait/in-trait/foreign.stderr | 39 ++++++++ tests/ui/impl-trait/in-trait/refine.rs | 3 + tests/ui/impl-trait/in-trait/refine.stderr | 58 +++++++++++- 13 files changed, 239 insertions(+), 33 deletions(-) create mode 100644 tests/ui/impl-trait/in-trait/foreign.stderr diff --git a/compiler/rustc_hir_analysis/messages.ftl b/compiler/rustc_hir_analysis/messages.ftl index e376411cd95..41e3005545e 100644 --- a/compiler/rustc_hir_analysis/messages.ftl +++ b/compiler/rustc_hir_analysis/messages.ftl @@ -347,6 +347,7 @@ hir_analysis_rpitit_refined = impl trait in impl method signature does not match .label = return type from trait method defined here .unmatched_bound_label = this bound is stronger than that defined on the trait .note = add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + .feedback_note = we are soliciting feedback, see issue #121718 for more information hir_analysis_self_in_impl_self = `Self` is not valid in the self type of an impl block diff --git a/compiler/rustc_hir_analysis/src/check/compare_impl_item/refine.rs b/compiler/rustc_hir_analysis/src/check/compare_impl_item/refine.rs index 29dc434ab45..a3edf625c96 100644 --- a/compiler/rustc_hir_analysis/src/check/compare_impl_item/refine.rs +++ b/compiler/rustc_hir_analysis/src/check/compare_impl_item/refine.rs @@ -2,7 +2,7 @@ use rustc_hir as hir; use rustc_hir::def_id::DefId; use rustc_infer::infer::{outlives::env::OutlivesEnvironment, TyCtxtInferExt}; -use rustc_lint_defs::builtin::REFINING_IMPL_TRAIT; +use rustc_lint_defs::builtin::{REFINING_IMPL_TRAIT_INTERNAL, REFINING_IMPL_TRAIT_REACHABLE}; use rustc_middle::traits::{ObligationCause, Reveal}; use rustc_middle::ty::{ self, Ty, TyCtxt, TypeFoldable, TypeFolder, TypeSuperVisitable, TypeVisitable, TypeVisitor, @@ -24,26 +24,23 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>( if !tcx.impl_method_has_trait_impl_trait_tys(impl_m.def_id) { return; } + // unreachable traits don't have any library guarantees, there's no need to do this check. - if trait_m + let is_internal = trait_m .container_id(tcx) .as_local() .is_some_and(|trait_def_id| !tcx.effective_visibilities(()).is_reachable(trait_def_id)) - { - return; - } + // If a type in the trait ref is private, then there's also no reason to do this check. + || impl_trait_ref.args.iter().any(|arg| { + if let Some(ty) = arg.as_type() + && let Some(self_visibility) = type_visibility(tcx, ty) + { + return !self_visibility.is_public(); + } + false + }); - // If a type in the trait ref is private, then there's also no reason to do this check. let impl_def_id = impl_m.container_id(tcx); - for arg in impl_trait_ref.args { - if let Some(ty) = arg.as_type() - && let Some(self_visibility) = type_visibility(tcx, ty) - && !self_visibility.is_public() - { - return; - } - } - let impl_m_args = ty::GenericArgs::identity_for_item(tcx, impl_m.def_id); let trait_m_to_impl_m_args = impl_m_args.rebase_onto(tcx, impl_def_id, impl_trait_ref.args); let bound_trait_m_sig = tcx.fn_sig(trait_m.def_id).instantiate(tcx, trait_m_to_impl_m_args); @@ -86,6 +83,7 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>( trait_m.def_id, impl_m.def_id, None, + is_internal, ); return; }; @@ -105,6 +103,7 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>( trait_m.def_id, impl_m.def_id, None, + is_internal, ); return; } @@ -199,6 +198,7 @@ pub(super) fn check_refining_return_position_impl_trait_in_trait<'tcx>( trait_m.def_id, impl_m.def_id, Some(span), + is_internal, ); return; } @@ -239,6 +239,7 @@ fn report_mismatched_rpitit_signature<'tcx>( trait_m_def_id: DefId, impl_m_def_id: DefId, unmatched_bound: Option, + is_internal: bool, ) { let mapping = std::iter::zip( tcx.fn_sig(trait_m_def_id).skip_binder().bound_vars(), @@ -291,7 +292,7 @@ fn report_mismatched_rpitit_signature<'tcx>( let span = unmatched_bound.unwrap_or(span); tcx.emit_node_span_lint( - REFINING_IMPL_TRAIT, + if is_internal { REFINING_IMPL_TRAIT_INTERNAL } else { REFINING_IMPL_TRAIT_REACHABLE }, tcx.local_def_id_to_hir_id(impl_m_def_id.expect_local()), span, crate::errors::ReturnPositionImplTraitInTraitRefined { diff --git a/compiler/rustc_hir_analysis/src/errors.rs b/compiler/rustc_hir_analysis/src/errors.rs index 5330260fbf5..dac62c22f89 100644 --- a/compiler/rustc_hir_analysis/src/errors.rs +++ b/compiler/rustc_hir_analysis/src/errors.rs @@ -1072,6 +1072,7 @@ pub struct UnusedAssociatedTypeBounds { #[derive(LintDiagnostic)] #[diag(hir_analysis_rpitit_refined)] #[note] +#[note(hir_analysis_feedback_note)] pub(crate) struct ReturnPositionImplTraitInTraitRefined<'tcx> { #[suggestion(applicability = "maybe-incorrect", code = "{pre}{return_ty}{post}")] pub impl_return_span: Span, diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 250c4adb948..31c80c4d75b 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -313,6 +313,12 @@ macro_rules! add_lint_group { // MACRO_USE_EXTERN_CRATE ); + add_lint_group!( + "refining_impl_trait", + REFINING_IMPL_TRAIT_REACHABLE, + REFINING_IMPL_TRAIT_INTERNAL + ); + // Register renamed and removed lints. store.register_renamed("single_use_lifetime", "single_use_lifetimes"); store.register_renamed("elided_lifetime_in_path", "elided_lifetimes_in_paths"); diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 94f8bbe2437..e2ede2c25cf 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -77,7 +77,8 @@ PROC_MACRO_BACK_COMPAT, PROC_MACRO_DERIVE_RESOLUTION_FALLBACK, PUB_USE_OF_PRIVATE_EXTERN_CRATE, - REFINING_IMPL_TRAIT, + REFINING_IMPL_TRAIT_INTERNAL, + REFINING_IMPL_TRAIT_REACHABLE, RENAMED_AND_REMOVED_LINTS, REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS, RUST_2021_INCOMPATIBLE_CLOSURE_CAPTURES, @@ -4310,8 +4311,10 @@ } declare_lint! { - /// The `refining_impl_trait` lint detects usages of return-position impl - /// traits in trait signatures which are refined by implementations. + /// The `refining_impl_trait_reachable` lint detects `impl Trait` return + /// types in method signatures that are refined by a publically reachable + /// trait implementation, meaning the implementation adds information about + /// the return type that is not present in the trait. /// /// ### Example /// @@ -4333,7 +4336,7 @@ /// fn main() { /// // users can observe that the return type of /// // `<&str as AsDisplay>::as_display()` is `&str`. - /// let x: &str = "".as_display(); + /// let _x: &str = "".as_display(); /// } /// ``` /// @@ -4341,13 +4344,80 @@ /// /// ### Explanation /// - /// Return-position impl trait in traits (RPITITs) desugar to associated types, - /// and callers of methods for types where the implementation is known are + /// Callers of methods for types where the implementation is known are /// able to observe the types written in the impl signature. This may be - /// intended behavior, but may also pose a semver hazard for authors of libraries - /// who do not wish to make stronger guarantees about the types than what is - /// written in the trait signature. - pub REFINING_IMPL_TRAIT, + /// intended behavior, but may also lead to implementation details being + /// revealed unintentionally. In particular, it may pose a semver hazard + /// for authors of libraries who do not wish to make stronger guarantees + /// about the types than what is written in the trait signature. + /// + /// `refining_impl_trait` is a lint group composed of two lints: + /// + /// * `refining_impl_trait_reachable`, for refinements that are publically + /// reachable outside a crate, and + /// * `refining_impl_trait_internal`, for refinements that are only visible + /// within a crate. + /// + /// We are seeking feedback on each of these lints; see issue + /// [#121718](https://github.com/rust-lang/rust/issues/121718) for more + /// information. + pub REFINING_IMPL_TRAIT_REACHABLE, + Warn, + "impl trait in impl method signature does not match trait method signature", +} + +declare_lint! { + /// The `refining_impl_trait_internal` lint detects `impl Trait` return + /// types in method signatures that are refined by a trait implementation, + /// meaning the implementation adds information about the return type that + /// is not present in the trait. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// #![deny(refining_impl_trait)] + /// + /// use std::fmt::Display; + /// + /// trait AsDisplay { + /// fn as_display(&self) -> impl Display; + /// } + /// + /// impl<'s> AsDisplay for &'s str { + /// fn as_display(&self) -> Self { + /// *self + /// } + /// } + /// + /// fn main() { + /// // users can observe that the return type of + /// // `<&str as AsDisplay>::as_display()` is `&str`. + /// let _x: &str = "".as_display(); + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// Callers of methods for types where the implementation is known are + /// able to observe the types written in the impl signature. This may be + /// intended behavior, but may also lead to implementation details being + /// revealed unintentionally. In particular, it may pose a semver hazard + /// for authors of libraries who do not wish to make stronger guarantees + /// about the types than what is written in the trait signature. + /// + /// `refining_impl_trait` is a lint group composed of two lints: + /// + /// * `refining_impl_trait_reachable`, for refinements that are publically + /// reachable outside a crate, and + /// * `refining_impl_trait_internal`, for refinements that are only visible + /// within a crate. + /// + /// We are seeking feedback on each of these lints; see issue + /// [#121718](https://github.com/rust-lang/rust/issues/121718) for more + /// information. + pub REFINING_IMPL_TRAIT_INTERNAL, Warn, "impl trait in impl method signature does not match trait method signature", } diff --git a/src/tools/lint-docs/src/groups.rs b/src/tools/lint-docs/src/groups.rs index c5cd30ccc34..0c39f2fa601 100644 --- a/src/tools/lint-docs/src/groups.rs +++ b/src/tools/lint-docs/src/groups.rs @@ -16,6 +16,10 @@ ("rust-2018-compatibility", "Lints used to transition code from the 2015 edition to 2018"), ("rust-2021-compatibility", "Lints used to transition code from the 2018 edition to 2021"), ("rust-2024-compatibility", "Lints used to transition code from the 2021 edition to 2024"), + ( + "refining-impl-trait", + "Detects refinement of `impl Trait` return types by trait implementations", + ), ]; type LintGroups = BTreeMap>; diff --git a/tests/ui/async-await/in-trait/async-example-desugared-boxed.stderr b/tests/ui/async-await/in-trait/async-example-desugared-boxed.stderr index 54aba77cc05..36f90c7bcd5 100644 --- a/tests/ui/async-await/in-trait/async-example-desugared-boxed.stderr +++ b/tests/ui/async-await/in-trait/async-example-desugared-boxed.stderr @@ -8,11 +8,13 @@ LL | fn foo(&self) -> Pin + '_>> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information note: the lint level is defined here --> $DIR/async-example-desugared-boxed.rs:13:12 | LL | #[warn(refining_impl_trait)] | ^^^^^^^^^^^^^^^^^^^ + = note: `#[warn(refining_impl_trait_reachable)]` implied by `#[warn(refining_impl_trait)]` help: replace the return type so that it matches the trait | LL | fn foo(&self) -> impl Future { diff --git a/tests/ui/async-await/in-trait/async-example-desugared-manual.stderr b/tests/ui/async-await/in-trait/async-example-desugared-manual.stderr index d94afd92c56..8e39559071f 100644 --- a/tests/ui/async-await/in-trait/async-example-desugared-manual.stderr +++ b/tests/ui/async-await/in-trait/async-example-desugared-manual.stderr @@ -8,11 +8,13 @@ LL | fn foo(&self) -> MyFuture { | ^^^^^^^^ | = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information note: the lint level is defined here --> $DIR/async-example-desugared-manual.rs:21:12 | LL | #[warn(refining_impl_trait)] | ^^^^^^^^^^^^^^^^^^^ + = note: `#[warn(refining_impl_trait_reachable)]` implied by `#[warn(refining_impl_trait)]` help: replace the return type so that it matches the trait | LL | fn foo(&self) -> impl Future { diff --git a/tests/ui/impl-trait/in-trait/bad-item-bound-within-rpitit.stderr b/tests/ui/impl-trait/in-trait/bad-item-bound-within-rpitit.stderr index c898d17f4b7..022df6f906c 100644 --- a/tests/ui/impl-trait/in-trait/bad-item-bound-within-rpitit.stderr +++ b/tests/ui/impl-trait/in-trait/bad-item-bound-within-rpitit.stderr @@ -22,7 +22,8 @@ LL | fn iter(&self) -> impl 'a + Iterator> { | ^^ this bound is stronger than that defined on the trait | = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate - = note: `#[warn(refining_impl_trait)]` on by default + = note: we are soliciting feedback, see issue #121718 for more information + = note: `#[warn(refining_impl_trait_reachable)]` on by default help: replace the return type so that it matches the trait | LL | fn iter(&self) -> impl Iterator::Item<'_>> + '_ { diff --git a/tests/ui/impl-trait/in-trait/foreign.rs b/tests/ui/impl-trait/in-trait/foreign.rs index e28bc9e00cb..ca759afc2e6 100644 --- a/tests/ui/impl-trait/in-trait/foreign.rs +++ b/tests/ui/impl-trait/in-trait/foreign.rs @@ -17,9 +17,29 @@ fn bar(self) -> Arc { } } -struct LocalIgnoreRefining; -impl Foo for LocalIgnoreRefining { - #[deny(refining_impl_trait)] +struct LocalOnlyRefiningA; +impl Foo for LocalOnlyRefiningA { + #[warn(refining_impl_trait)] + fn bar(self) -> Arc { + //~^ WARN impl method signature does not match trait method signature + Arc::new(String::new()) + } +} + +struct LocalOnlyRefiningB; +impl Foo for LocalOnlyRefiningB { + #[warn(refining_impl_trait)] + #[allow(refining_impl_trait_reachable)] + fn bar(self) -> Arc { + //~^ WARN impl method signature does not match trait method signature + Arc::new(String::new()) + } +} + +struct LocalOnlyRefiningC; +impl Foo for LocalOnlyRefiningC { + #[warn(refining_impl_trait)] + #[allow(refining_impl_trait_internal)] fn bar(self) -> Arc { Arc::new(String::new()) } @@ -34,5 +54,7 @@ fn main() { let &() = Foreign.bar(); let x: Arc = Local.bar(); - let x: Arc = LocalIgnoreRefining.bar(); + let x: Arc = LocalOnlyRefiningA.bar(); + let x: Arc = LocalOnlyRefiningB.bar(); + let x: Arc = LocalOnlyRefiningC.bar(); } diff --git a/tests/ui/impl-trait/in-trait/foreign.stderr b/tests/ui/impl-trait/in-trait/foreign.stderr new file mode 100644 index 00000000000..1a5a4f2432b --- /dev/null +++ b/tests/ui/impl-trait/in-trait/foreign.stderr @@ -0,0 +1,39 @@ +warning: impl trait in impl method signature does not match trait method signature + --> $DIR/foreign.rs:23:21 + | +LL | fn bar(self) -> Arc { + | ^^^^^^^^^^^ + | + = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information +note: the lint level is defined here + --> $DIR/foreign.rs:22:12 + | +LL | #[warn(refining_impl_trait)] + | ^^^^^^^^^^^^^^^^^^^ + = note: `#[warn(refining_impl_trait_internal)]` implied by `#[warn(refining_impl_trait)]` +help: replace the return type so that it matches the trait + | +LL | fn bar(self) -> impl Deref { + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +warning: impl trait in impl method signature does not match trait method signature + --> $DIR/foreign.rs:33:21 + | +LL | fn bar(self) -> Arc { + | ^^^^^^^^^^^ + | + = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information +note: the lint level is defined here + --> $DIR/foreign.rs:31:12 + | +LL | #[warn(refining_impl_trait)] + | ^^^^^^^^^^^^^^^^^^^ +help: replace the return type so that it matches the trait + | +LL | fn bar(self) -> impl Deref { + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +warning: 2 warnings emitted + diff --git a/tests/ui/impl-trait/in-trait/refine.rs b/tests/ui/impl-trait/in-trait/refine.rs index 100e6da06a8..6813c3bbf27 100644 --- a/tests/ui/impl-trait/in-trait/refine.rs +++ b/tests/ui/impl-trait/in-trait/refine.rs @@ -25,6 +25,7 @@ fn bar() -> () {} struct Private; impl Foo for Private { fn bar() -> () {} + //~^ ERROR impl method signature does not match trait method signature } pub trait Arg { @@ -32,6 +33,7 @@ pub trait Arg { } impl Arg for A { fn bar() -> () {} + //~^ ERROR impl method signature does not match trait method signature } pub trait Late { @@ -52,6 +54,7 @@ pub trait UnreachablePub { struct E; impl UnreachablePub for E { fn bar() {} + //~^ ERROR impl method signature does not match trait method signature } } diff --git a/tests/ui/impl-trait/in-trait/refine.stderr b/tests/ui/impl-trait/in-trait/refine.stderr index 96a9bc059c8..8d30b035921 100644 --- a/tests/ui/impl-trait/in-trait/refine.stderr +++ b/tests/ui/impl-trait/in-trait/refine.stderr @@ -8,11 +8,13 @@ LL | fn bar() -> impl Copy {} | ^^^^ this bound is stronger than that defined on the trait | = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information note: the lint level is defined here --> $DIR/refine.rs:1:9 | LL | #![deny(refining_impl_trait)] | ^^^^^^^^^^^^^^^^^^^ + = note: `#[deny(refining_impl_trait_reachable)]` implied by `#[deny(refining_impl_trait)]` help: replace the return type so that it matches the trait | LL | fn bar() -> impl Sized {} @@ -28,6 +30,7 @@ LL | fn bar() {} | ^^^^^^^^ | = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information help: replace the return type so that it matches the trait | LL | fn bar()-> impl Sized {} @@ -43,13 +46,47 @@ LL | fn bar() -> () {} | ^^ | = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information help: replace the return type so that it matches the trait | LL | fn bar() -> impl Sized {} | ~~~~~~~~~~ error: impl trait in impl method signature does not match trait method signature - --> $DIR/refine.rs:43:27 + --> $DIR/refine.rs:27:17 + | +LL | fn bar() -> impl Sized; + | ---------- return type from trait method defined here +... +LL | fn bar() -> () {} + | ^^ + | + = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information + = note: `#[deny(refining_impl_trait_internal)]` implied by `#[deny(refining_impl_trait)]` +help: replace the return type so that it matches the trait + | +LL | fn bar() -> impl Sized {} + | ~~~~~~~~~~ + +error: impl trait in impl method signature does not match trait method signature + --> $DIR/refine.rs:35:17 + | +LL | fn bar() -> impl Sized; + | ---------- return type from trait method defined here +... +LL | fn bar() -> () {} + | ^^ + | + = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information +help: replace the return type so that it matches the trait + | +LL | fn bar() -> impl Sized {} + | ~~~~~~~~~~ + +error: impl trait in impl method signature does not match trait method signature + --> $DIR/refine.rs:45:27 | LL | fn bar<'a>(&'a self) -> impl Sized + 'a; | --------------- return type from trait method defined here @@ -58,10 +95,27 @@ LL | fn bar(&self) -> impl Copy + '_ {} | ^^^^ this bound is stronger than that defined on the trait | = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information help: replace the return type so that it matches the trait | LL | fn bar(&self) -> impl Sized + '_ {} | ~~~~~~~~~~~~~~~ -error: aborting due to 4 previous errors +error: impl trait in impl method signature does not match trait method signature + --> $DIR/refine.rs:56:9 + | +LL | fn bar() -> impl Sized; + | ---------- return type from trait method defined here +... +LL | fn bar() {} + | ^^^^^^^^ + | + = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate + = note: we are soliciting feedback, see issue #121718 for more information +help: replace the return type so that it matches the trait + | +LL | fn bar()-> impl Sized {} + | +++++++++++++ + +error: aborting due to 7 previous errors