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

trait_added_supertrait should eventually use importable paths, not supertrait names #922

Open
obi1kenobi opened this issue Sep 8, 2024 · 0 comments
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Sep 8, 2024

Due to #638, the trait_added_supertrait lint (#441, #892) is unable to do precise-enough analysis of foreign traits, and can have both false positives and false negatives.

Foreign crate's supertrait

An easy false-negative is the case where a foreign crate's supertrait is added:

// old:
pub trait Example {}

// new:
pub trait Example: another_crate::OtherTrait {}

We can't see the info for another_crate so this bound is "invisible" to cargo-semver-checks. There's a limited exception for common built-in traits (Debug, Clone, Send, Sync, PartialOrd, Ord, PartialEq, Eq, Hash, UnwindSafe, RefUnwindSafe, Unpin, Sized) which we "manually inline" to make them available for analysis. But any other trait bound on another crate's trait is invisible.

Name comparisons

The best way to currently implement the trait_added_supertrait lint is by comparing supertrait names (the ImplementedTrait.name property. This is insufficiently precise, and the correct way (blocked by #638) would be to go ImplementedTrait -> Trait -> ImportablePath and compare based on those names instead.

Here's a false-negative caused by this problem:

// old:
pub trait Example: std::fmt::Debug {}

// new:
pub trait Debug {}

pub trait Example: Debug {}

The newly-defined local Debug trait here has the same name as std::fmt::Debug (so the lint won't trigger), but obviously isn't the same trait.

Here's a false-positive caused by the same problem:

// old:
pub mod inner {
    pub trait Super {}
}

pub trait Example: inner::Super {}

// new:
pub mod inner {
    pub trait Upper {}

    pub use Upper as Super;
}

pub trait Example: inner::Upper {}

Here, the trait isn't named the same across old & new, but is the same trait since it's available at the same importable path.

@obi1kenobi obi1kenobi added A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations labels Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

No branches or pull requests

1 participant