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

Add new UNSTABLE_INTRINSICS_WITH_STABLE_WRAPPER lint #12226

Conversation

GuillaumeGomez
Copy link
Member

Fixes #2997.

To prevent having to keep the list of intrinsics with stable counterpart up-to-date on clippy side, I instead check if the documentation contains the sentence "does not have a stable counterpart". To prevent running this check more than once, I store all the non-matching intrinsics DefId into a static.

r? @llogiq

changelog: Add new UNSTABLE_INTRINSICS_WITH_STABLE_WRAPPER lint

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 4, 2024
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Feb 4, 2024

Interestingly enough, we cannot differentiate between the intrinsics item and the "reexported" one (like for std::ptr::write_bytes). I'll try to extract more information to ensure which call is actually performed.

@GuillaumeGomez GuillaumeGomez force-pushed the UNSTABLE_INTRINSICS_WITH_STABLE_WRAPPER branch 5 times, most recently from 98e64e3 to 5808e25 Compare February 5, 2024 15:24
@GuillaumeGomez
Copy link
Member Author

Took me a while to realize that the error was coming from the doc example... ^^'

PR is now ready!

@GuillaumeGomez GuillaumeGomez force-pushed the UNSTABLE_INTRINSICS_WITH_STABLE_WRAPPER branch from 5808e25 to f241339 Compare February 5, 2024 17:44
Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I think this should be a rustc lint instead, that way there can be a "stabilized counterpart" attribute it checks for instead.

fn_name: &str,
segments: &[PathSegment<'_>],
) {
static FNS: OnceLock<Vec<DefId>> = OnceLock::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a DefIdMap?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think having a Vec will be a problem here, we don't have that many intrinsics anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess. A benchmark is always handy, I just think it's cleaner at least and more greppable

Res::Def(DefKind::Fn, fn_def_id) => Some(fn_def_id),
_ => None,
})
.filter(|fn_def_id| !get_doc(cx.tcx, *fn_def_id).contains("does not have a stable counterpart"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some intrinsics don't have this. For example is_val_statically_known or const_eval_select

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably best to at least check for "the stabilized version of this intrinsic is" after this too

@bors
Copy link
Contributor

bors commented Feb 19, 2024

☔ The latest upstream changes (presumably #12306) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -1,5 +1,5 @@
error: transmute from a reference to a pointer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this lint should be allowed in other tests.

@J-ZhengLi
Copy link
Member

Hey @GuillaumeGomez , this is a ping from triage. There hasn't been any activity for some time, perhaps it was forgotten?

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 16, 2024
@GuillaumeGomez
Copy link
Member Author

I'll close it because it should likely be a rustc lint directly.

@GuillaumeGomez GuillaumeGomez deleted the UNSTABLE_INTRINSICS_WITH_STABLE_WRAPPER branch May 16, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint against calling intrinsics that have stable or safe wrappers
7 participants