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

typeck: silence lint for "collision safe" items #99898

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3666,6 +3666,7 @@ name = "rustc_hir_typeck"
version = "0.1.0"
dependencies = [
"rustc_ast",
"rustc_attr",
"rustc_data_structures",
"rustc_errors",
"rustc_graphviz",
Expand Down
54 changes: 54 additions & 0 deletions compiler/rustc_attr/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,50 @@ pub enum StabilityLevel {
/// fn foobar() {}
/// ```
implied_by: Option<Symbol>,
/// When an unstable method exists on a type and the user is invoking a method that would
/// no longer have priority when the unstable method is stabilized, an "unstable name
/// collision" lint is emitted. In the vast majority of circumstances, this is desirable.
///
/// However, when adding a new inherent method to the standard library, which deliberately
/// shadows the name of a method in the `Deref` target, then this lint can be triggered,
/// affecting users on stable who don't even know about the new unstable inherent method.
/// As the new method is being added to the standard library, by the library team, it can
/// be known that the lint isn't necessary, as the library team can ensure that the new
/// inherent method has the same behaviour and won't cause any problems for users when it
/// is stabilized.
///
/// ```pseudo-Rust
/// pub struct Foo;
/// pub struct Bar;
///
/// impl std::ops::Deref for Foo {
/// type Target = Bar;
/// fn deref(&self) -> &Self::Target { &Bar }
/// }
///
/// impl Foo {
/// #[unstable(feature = "new_feature", issue = "none", collision_safe)]
/// pub fn example(&self) -> u32 { 4 }
/// }
///
/// impl Bar {
/// #[stable(feature = "old_feature", since = "1.0.0")]
/// pub fn example(&self) -> u32 { 3 }
/// }
///
/// // ..in another crate..
/// fn main() {
/// let foo = Foo;
/// assert_eq!(foo.example(), 3);
/// // still invokes `Bar`'s `example`, as the `new_feature` isn't enabled, but doesn't
/// // trigger a name collision lint (in practice, both `example` functions should
/// // have identical behaviour)
/// }
/// ```
///
/// Without this addition, the new inherent method would need to be insta-stable in order
/// to avoid breaking stable users.
collision_safe: bool,
},
/// `#[stable]`
Stable {
Expand Down Expand Up @@ -328,6 +372,7 @@ where
let mut issue = None;
let mut issue_num = None;
let mut is_soft = false;
let mut collision_safe = false;
let mut implied_by = None;
for meta in metas {
let Some(mi) = meta.meta_item() else {
Expand Down Expand Up @@ -383,6 +428,14 @@ where
}
is_soft = true;
}
sym::collision_safe => {
if !mi.is_word() {
sess.emit_err(session_diagnostics::CollisionSafeNoArgs {
span: mi.span,
});
}
collision_safe = true;
}
sym::implied_by => {
if !get(mi, &mut implied_by) {
continue 'outer;
Expand Down Expand Up @@ -417,6 +470,7 @@ where
issue: issue_num,
is_soft,
implied_by,
collision_safe,
};
if sym::unstable == meta_name {
stab = Some((Stability { level, feature }, attr.span));
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_attr/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,13 @@ pub(crate) struct SoftNoArgs {
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(attr_collision_safe_no_args)]
pub(crate) struct CollisionSafeNoArgs {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(attr_unknown_version_literal)]
pub(crate) struct UnknownVersionLiteral {
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_error_messages/locales/en-US/attr.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,8 @@ attr_expects_features =
attr_soft_no_args =
`soft` should not have any arguments

attr_collision_safe_no_args =
`collision_safe` should not have any arguments

attr_unknown_version_literal =
unknown version literal format, assuming it refers to a future version
1 change: 1 addition & 0 deletions compiler/rustc_hir_typeck/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ edition = "2021"
smallvec = { version = "1.8.1", features = ["union", "may_dangle"] }
tracing = "0.1"
rustc_ast = { path = "../rustc_ast" }
rustc_attr = { path = "../rustc_attr" }
rustc_data_structures = { path = "../rustc_data_structures" }
rustc_errors = { path = "../rustc_errors" }
rustc_graphviz = { path = "../rustc_graphviz" }
Expand Down
28 changes: 27 additions & 1 deletion compiler/rustc_hir_typeck/src/method/probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use super::NoMatchData;

use crate::errors::MethodCallOnUnknownType;
use crate::FnCtxt;

use rustc_attr::{Stability, StabilityLevel};
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_hir as hir;
Expand Down Expand Up @@ -1365,9 +1367,33 @@ impl<'tcx> Pick<'tcx> {
span: Span,
scope_expr_id: hir::HirId,
) {
if self.unstable_candidates.is_empty() {
if self.unstable_candidates.is_empty()
|| !{
let has_collision_unsafe_candidate =
self.unstable_candidates.iter().any(|(candidate, _)| {
let stab = tcx.lookup_stability(candidate.item.def_id);
!matches!(
stab,
Some(Stability {
level: StabilityLevel::Unstable { collision_safe: true, .. },
..
})
)
});
// `collision_safe` shouldn't silence the lint when the selected candidate is from
// user code, only when it is a collision with an item from std (where the
// assumption is that t-libs have confirmed that such a collision is okay). Use the
// existence of a stability attribute on the selected candidate has a heuristic
// for item from std.
let chosen_candidate_has_stability =
tcx.lookup_stability(self.item.def_id).is_some();
debug!(?has_collision_unsafe_candidate, ?chosen_candidate_has_stability, ?self);
has_collision_unsafe_candidate || !chosen_candidate_has_stability
}
{
return;
}

let def_kind = self.item.kind.as_def_kind();
tcx.struct_span_lint_hir(
lint::builtin::UNSTABLE_NAME_COLLISIONS,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/middle/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ impl<'tcx> TyCtxt<'tcx> {

match stability {
Some(Stability {
level: attr::Unstable { reason, issue, is_soft, implied_by },
level: attr::Unstable { reason, issue, is_soft, implied_by, .. },
feature,
..
}) => {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_passes/src/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,7 @@ fn stability_index(tcx: TyCtxt<'_>, (): ()) -> Index {
issue: NonZeroU32::new(27812),
is_soft: false,
implied_by: None,
collision_safe: false,
},
feature: sym::rustc_private,
};
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_resolve/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,8 @@ impl<'a> Resolver<'a> {
) {
let span = path.span;
if let Some(stability) = &ext.stability {
if let StabilityLevel::Unstable { reason, issue, is_soft, implied_by } = stability.level
if let StabilityLevel::Unstable { reason, issue, is_soft, implied_by, .. } =
stability.level
{
let feature = stability.feature;

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ symbols! {
coerce_unsized,
cold,
collapse_debuginfo,
collision_safe,
column,
compare_exchange,
compare_exchange_weak,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
#![crate_type = "lib"]
#![feature(staged_api)]
#![stable(feature = "old_feature", since = "1.0.0")]

#[stable(feature = "old_feature", since = "1.0.0")]
pub struct Foo;

#[stable(feature = "old_feature", since = "1.0.0")]
pub struct Bar;

#[stable(feature = "old_feature", since = "1.0.0")]
impl std::ops::Deref for Foo {
type Target = Bar;

fn deref(&self) -> &Self::Target {
&Bar
}
}

impl Foo {
#[unstable(feature = "new_feature", issue = "none")]
pub fn example(&self) -> u32 {
4
}

#[unstable(feature = "new_feature", issue = "none", collision_safe)]
pub fn example_safe(&self) -> u32 {
2
}
}

impl Bar {
#[stable(feature = "old_feature", since = "1.0.0")]
pub fn example(&self) -> u32 {
3
}

#[stable(feature = "old_feature", since = "1.0.0")]
pub fn example_safe(&self) -> u32 {
2
}
}

#[stable(feature = "trait_feature", since = "1.0.0")]
pub trait Trait {
#[unstable(feature = "new_trait_feature", issue = "none")]
fn not_safe(&self) -> u32 {
0
}

#[unstable(feature = "new_trait_feature", issue = "none", collision_safe)]
fn safe(&self) -> u32 {
0
}

#[unstable(feature = "new_trait_feature", issue = "none", collision_safe)]
fn safe_and_shadowing_a_stable_item(&self) -> u32 {
0
}
}

#[stable(feature = "trait_feature", since = "1.0.0")]
pub trait OtherTrait {
#[stable(feature = "trait_feature", since = "1.0.0")]
fn safe_and_shadowing_a_stable_item(&self) -> u32 {
4
}
}

#[stable(feature = "trait_feature", since = "1.0.0")]
impl Trait for char { }

#[stable(feature = "trait_feature", since = "1.0.0")]
impl OtherTrait for char { }
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#![crate_type = "lib"]
#![feature(staged_api)]
#![stable(feature = "old_feature", since = "1.0.0")]

#[stable(feature = "old_feature", since = "1.0.0")]
pub struct Foo;

impl Foo {
#[unstable(feature = "new_feature", issue = "none", collision_safe = "foo")]
//~^ ERROR `collision_safe` should not have any arguments
pub fn bad(&self) -> u32 {
2
}

#[unstable(feature = "new_feature", issue = "none", collision_safe("foo"))]
//~^ ERROR `collision_safe` should not have any arguments
pub fn also_bad(&self) -> u32 {
2
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: `collision_safe` should not have any arguments
--> $DIR/stability-attribute-collision-safe-not-word.rs:9:57
|
LL | #[unstable(feature = "new_feature", issue = "none", collision_safe = "foo")]
| ^^^^^^^^^^^^^^^^^^^^^^

error: `collision_safe` should not have any arguments
--> $DIR/stability-attribute-collision-safe-not-word.rs:15:57
|
LL | #[unstable(feature = "new_feature", issue = "none", collision_safe("foo"))]
| ^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// aux-build:stability-attribute-collision-safe.rs
#![deny(unstable_name_collisions)]

extern crate stability_attribute_collision_safe;
use stability_attribute_collision_safe::{Trait, OtherTrait};

pub trait LocalTrait {
fn not_safe(&self) -> u32 {
1
}

fn safe(&self) -> u32 {
1
}
}

impl LocalTrait for char {}


fn main() {
// Despite having `collision_safe` on defn, the fn chosen doesn't have a stability attribute,
// thus could be user code (and is in this test), so the lint is still appropriate..
assert_eq!('x'.safe(), 1);
//~^ ERROR an associated function with this name may be added to the standard library in the future
//~^^ WARN once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!

// ..but with `collision_safe` on defn, if the chosen item has a stability attribute, then
// assumed to be from std or somewhere that's been checked to be okay, so no lint..
assert_eq!('x'.safe_and_shadowing_a_stable_item(), 4); // okay!

// ..and not safe functions should, of course, still lint..
assert_eq!('x'.not_safe(), 1);
//~^ ERROR an associated function with this name may be added to the standard library in the future
//~^^ WARN once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
error: an associated function with this name may be added to the standard library in the future
--> $DIR/stability-attribute-collision-safe-user.rs:23:20
|
LL | assert_eq!('x'.safe(), 1);
| ^^^^
|
= warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
= note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
= help: call with fully qualified syntax `LocalTrait::safe(...)` to keep using the current method
= help: add `#![feature(new_trait_feature)]` to the crate attributes to enable `safe`
note: the lint level is defined here
--> $DIR/stability-attribute-collision-safe-user.rs:2:9
|
LL | #![deny(unstable_name_collisions)]
| ^^^^^^^^^^^^^^^^^^^^^^^^

error: an associated function with this name may be added to the standard library in the future
--> $DIR/stability-attribute-collision-safe-user.rs:32:20
|
LL | assert_eq!('x'.not_safe(), 1);
| ^^^^^^^^
|
= warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
= note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
= help: call with fully qualified syntax `LocalTrait::not_safe(...)` to keep using the current method
= help: add `#![feature(new_trait_feature)]` to the crate attributes to enable `not_safe`

error: aborting due to 2 previous errors

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// aux-build:stability-attribute-collision-safe.rs
// check-pass
#![feature(new_feature)]

extern crate stability_attribute_collision_safe;
use stability_attribute_collision_safe::Foo;

fn main() {
let f = Foo;
assert_eq!(f.example_safe(), 2); // okay! has `collision_safe` on defn

assert_eq!(f.example(), 4); // okay! have feature!
}
Loading