-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Fix an ICE observed with an explicit tail-call in a default trait method #145270
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
This comment has been minimized.
This comment has been minimized.
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 don't think this is the right approach. Frankly, using caller_ty
seems a bit unnecessary throughout check_tail_calls
, though that's preexisting.
I think it's better if we refactor check_tail_calls
to no longer rely at all on caller_ty
and instead just have a body def id which corresponds to the item that has the THIR. We shouldn't need substs either -- those are always identity.
Then we shouldn't need to use Instance::expect_resolve
at all here.
The logic determining whether the relevant function is marked as `#[track_caller]` only worked with functions that could be resolved to a concrete instance, which default trait methods cannot. We need to check the codegen attribute on the default method itself.
…a default trait method's implementation.
…t with a case of a default trait method.
09eede7
to
f41a618
Compare
Thank you for your feedback! I addressed it. There's a test failing in LLVM codegen, but this may be a pre-existing issue. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Unfortunately, that's a legitimate bug. I think the problem with I'm gonna mark this as blocked on #145321 (edit: actually #144755) @rustbot blocked As a side-note, please make sure your commits are squashed. It's not a requirement that you have 1 commit per PR, but this PR definitely shouldn't need 4 commits for a single change like this :) |
r? compiler-errors |
|
@compiler-errors, thanks for elaborating on this! So, as I gather from #145321 and #144755, if this is a pre-existing issue, would it be okay for us to move forward with this PR if we change the relevant test to be
Sure. I tend to make a commit per a logical chunk of work, and then in the course of PR’s review, only add new commits to make it clear what new changes have been made. But, of course, I’ll adjust and squash this, if you can confirm that converting the test to |
☔ The latest upstream changes (presumably #145423) made this pull request unmergeable. Please resolve the merge conflicts. |
Right now, explicit tail-calls cannot be used in functions that hold the
#[track_caller]
attribute. This check is performed on a resolved concrete instance of a function, which would be absent for a tail-call performed from the body of a default trait method. This PR fixes the issue by checking for the relevant attribute in default trait methods separately, without expecting a specific resolved instance.Closes #144985.