-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Forbid tail calling intrinsics #144851
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
Forbid tail calling intrinsics #144851
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using Instance::try_resolve
is a bit overkill since we can just use tcx.instance(def_id).is_some()
, but I think resolve is ok.
ty::Instance::try_resolve(self.tcx, self.typing_env, did, args) | ||
&& let InstanceKind::Intrinsic(_) = instance.def |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
importing InstanceKind
but not Instance
is a bit weird 🤔
r=me unless you want a specific review |
3454cee
to
38a1b07
Compare
if let Ok(Some(instance)) = | ||
ty::Instance::try_resolve(self.tcx, self.typing_env, did, args) | ||
&& let ty::InstanceKind::Intrinsic(_) = instance.def |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need the resolve here? Can probably just do https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.intrinsic.is_some()?
(Since you never get to an intrinsic through a trait impl or whatever.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, I somehow completely missed that c-e already said this 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well, with or without the change I mentioned.
I like the approach of giving a real error here -- since intrinsics are just expanded directly, there's no need to ever become
them, as you say.
38a1b07
to
c539890
Compare
@bors r=compiler-errors,scottmcm rollup |
…mpiler-errors,scottmcm Forbid tail calling intrinsics There is only one intrinsic that can be called on stable, as far as I can find, (`transmute`). And in general tail calling intrinsics doesn't make much sense. Alternative to rust-lang#144815 (and thus closes rust-lang#144815) Fixes rust-lang#144806 r? `@scottmcm`
Rollup of 12 pull requests Successful merges: - #142678 (Misc cleanups of `generic_arg_infer` related HIR logic) - #144070 (Implement `hash_map` macro ) - #144738 (Remove the omit_gdb_pretty_printer_section attribute) - #144790 (Multiple bounds checking elision failures) - #144805 (compiletest: Preliminary cleanup of `ProcRes` printing/unwinding) - #144808 (`Interner` arg to `EarlyBinder` does not affect auto traits) - #144816 (Update E0562 to account for the new impl trait positions) - #144822 (Return a struct with named fields from `hash_owner_nodes`) - #144824 (Updated test links in compiler) - #144829 (Use full flag name in strip command for Darwin) - #144843 (Weekly `cargo update`) - #144851 (Forbid tail calling intrinsics) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #144851 - WaffleLapkin:instrinsic-deny, r=compiler-errors,scottmcm Forbid tail calling intrinsics There is only one intrinsic that can be called on stable, as far as I can find, (`transmute`). And in general tail calling intrinsics doesn't make much sense. Alternative to #144815 (and thus closes #144815) Fixes #144806 r? ``@scottmcm``
There is only one intrinsic that can be called on stable, as far as I can find, (
transmute
). And in general tail calling intrinsics doesn't make much sense.Alternative to #144815 (and thus closes #144815)
Fixes #144806
r? @scottmcm