-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
implement #[panic_implementation] #50338
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
e44ad61
implement #[panic_implementation]
japaric 63f18e1
s/panic_fmt/panic_impl/g in docs
japaric eaef110
format payload if possible instead of returning "Box<Any>"
japaric b442348
remove unused `struct NoPayload`
japaric eb19361
document that `panic_impl` never passes the FFI boundary
japaric 430ad76
undo payload in core::panic! changes
japaric 86a0769
fix after rebase
japaric 4c84d38
remove #[unwind(allowed)]
japaric da2ee5d
reject `fn panic_impl<T>(_: &PanicInfo) -> !`
japaric a174f2a
add more tests
japaric 948ed24
fix tidy error
japaric 8ad15de
turn run-make test into a run-make-fulldeps test
japaric File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm personally more of a fan of generating AST nodes which correspond to the structure we want which avoids reaching into the internals of the typechecker where possible. For example I don't think this disallows something like this:
right?
I was thinking that we'd basically expand
#[panic_implementation]
to#[lang = "panic_impl"]
where the lang item internally dispatches to#[panic_implementation]
(automatically typechecking it along the way). That may be slightly more difficult to architect, though, but I'm curious what you thinkThere 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.
The language items are not type-checked at all at the moment, so there is nothing to hand the type-checking over to.
panic_fmt
has no benefits over the new language item and IIRC (from when I wrote the instructions) complicates the implementation of this feature unnecessarily.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.
Right yeah, my point is that we generate a language item which internally dispatches to the function annotated
#[panic_implementation]
, which by construction will typecheck the panic implementation and not require typechekcing of lang ites.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.
@alexcrichton you mean something like this?
That would work typechecking wise, but will it report error messages with meaningful spans? (My experience with proc macros says that most errors will wrongly report the whole #[panic_implementation] item as their span)
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 can add a check and test to reject that.
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.
@japaric yeah that latter idea is what I was thinking, where basically the compiler manufactures the actual lang item which is hooked up to call the
#[panic_implementation]
. That way the compiler has full control over ABI and whatnotThere 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.
Sadly in many cases this approach results in kind-of suboptimal errors.
For example
gives
but if expanded within a macro it would be something like this instead:
which is just… incomprehensible.
That being said, I have a long time wish to eventually implement universal checking of the language item signatures (similar to one we do for intrinsics), which would resolve this issue universally. Thus, I’d recommend going for the simplest complete approach possible for now.