-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Fix tail calls to #[track_caller]
functions
#144865
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
base: master
Are you sure you want to change the base?
Conversation
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
e747cdd
to
a487936
Compare
This comment has been minimized.
This comment has been minimized.
a487936
to
6fff1d6
Compare
This comment has been minimized.
This comment has been minimized.
I do not get it, both tests pass for me locally ?_? |
6fff1d6
to
15219a0
Compare
Update: @jieyouxu figured this out. It looks like the test suite that failed was running with |
not too familiar with tail calls but the diff looks good to me r? compiler |
r? lqd maybe (since you were excited about tail calls ^^') |
It also LGTM -- though to be clear, I'm not familiar with the Also, do we have to involve t-lang for new lints? |
Would this be a silent downgrade? I think that would be a terrible idea, since for me the main usage of |
@lqd the wording is certainly not ideal, although I'm not entirely sure what would be the proper way to describe this situation. I don't think T-lang needs to be involved, since this lint is triggered only by an unstable/experimental feature (but they'll certainly have to look into the lint before stabilizing the feature). @VorpalBlade it will not be silent, as this PR also adds a lint. However, note that functions marked with |
Ok I wasn't aware of the procedure for new lints. |
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.
👍
|
||
declare_lint! { | ||
/// The `tail_call_track_caller` lint detects usage of `become` attempting to tail call | ||
/// function marked with `#[track_caller]`. |
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.
nit, "a function" or "functions"
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.
Fixed
/// is no different than a normal call (except for changes in drop order). | ||
pub TAIL_CALL_TRACK_CALLER, | ||
Warn, | ||
"detects tail calls of functions marked with `#[track_caller]`" |
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.
Could you gate this behind the tail call feature?
I can't remember the syntax but I think it's something like @feature = ...
.
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 did not know this is something that can be done!
The syntax is apparently @feature_gate = ident;
. I've gated the lint.
15219a0
to
85d1c89
Compare
🚀 @bors r+ |
Fix tail calls to `#[track_caller]` functions We want `#[track_caller]` to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls. The only way to achieve this that I can see is: - we forbid tail calls in functions which are marked with `#[track_caller]` (already implemented) - tail-calling a `#[track_caller]` marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr) Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc `@saethlin,` rust-lang#131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated) Additionally I added a lint, although I don't think it's strictly necessary. Alternative to rust-lang#144762 (and thus closes rust-lang#144762) Fixes rust-lang#144755
Rollup of 22 pull requests Successful merges: - #118087 (Add Ref/RefMut try_map method) - #122661 (Change the desugaring of `assert!` for better error output) - #140740 (Add `-Zindirect-branch-cs-prefix`) - #142640 (Implement autodiff using intrinsics) - #143075 (compiler: Allow `extern "interrupt" fn() -> !`) - #144865 (Fix tail calls to `#[track_caller]` functions) - #144944 (E0793: Clarify that it applies to unions as well) - #144947 (Fix description of unsigned `checked_exact_div`) - #145004 (Couple of minor cleanups) - #145005 (strip prefix of temporary file names when it exceeds filesystem name length limit) - #145012 (Tail call diagnostics to include lifetime info) - #145065 (resolve: Introduce `RibKind::Block`) - #145120 (llvm: Accept new LLVM lifetime format) - #145189 (Weekly `cargo update`) - #145235 (Minor `[const]` tweaks) - #145275 (fix(compiler/rustc_codegen_llvm): apply `target-cpu` attribute) - #145322 (Resolve the prelude import in `build_reduced_graph`) - #145331 (Make std use the edition 2024 prelude) - #145369 (Do not ICE on private type in field of unresolved struct) - #145378 (Add `FnContext` in parser for diagnostic) - #145389 ([rustdoc] Revert "rustdoc search: prefer stable items in search results") - #145392 (coverage: Remove intermediate data structures from mapping creation) r? `@ghost` `@rustbot` modify labels: rollup
Fix tail calls to `#[track_caller]` functions We want `#[track_caller]` to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls. The only way to achieve this that I can see is: - we forbid tail calls in functions which are marked with `#[track_caller]` (already implemented) - tail-calling a `#[track_caller]` marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr) Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc ``@saethlin,`` rust-lang#131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated) Additionally I added a lint, although I don't think it's strictly necessary. Alternative to rust-lang#144762 (and thus closes rust-lang#144762) Fixes rust-lang#144755
Fix tail calls to `#[track_caller]` functions We want `#[track_caller]` to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls. The only way to achieve this that I can see is: - we forbid tail calls in functions which are marked with `#[track_caller]` (already implemented) - tail-calling a `#[track_caller]` marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr) Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc ```@saethlin,``` rust-lang#131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated) Additionally I added a lint, although I don't think it's strictly necessary. Alternative to rust-lang#144762 (and thus closes rust-lang#144762) Fixes rust-lang#144755
We want
#[track_caller]
to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls.The only way to achieve this that I can see is:
#[track_caller]
(already implemented)#[track_caller]
marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr)Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc @saethlin, #131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated)
Additionally I added a lint, although I don't think it's strictly necessary.
Alternative to #144762 (and thus closes #144762)
Fixes #144755