-
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
add a Callable trait that is implemented for unsafe functions, too #107123
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
☔ The latest upstream changes (presumably #107343) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
b6d8e66
to
17a06e2
Compare
error[E0277]: the trait bound `for<'a> fn(&'a <dyn Setup<From = T> as Setup>::From) -> <dyn Setup<From = T> as Setup>::From {copy::<dyn Setup<From = T>>}: Callable<(&T,)>` is not satisfied | ||
--> $DIR/object-unsafety.rs:12:5 | ||
| | ||
LL | copy::<dyn Setup<From = T>>(t) | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Callable<(&T,)>` is not implemented for fn item `for<'a> fn(&'a <dyn Setup<From = T> as Setup>::From) -> <dyn Setup<From = T> as Setup>::From {copy::<dyn Setup<From = T>>}` |
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.
Not exactly sure what happened here, but I'm assuming we just don't explain that Callable
isn't implemented because Setup
isn't implemented for dyn Setup
, but just fail the outer obligation.
Orthogonally to that I can make it less confusing by adding a rustc_on_unimplemented
to Callable
to give a more targeted message.
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.
It's because &'a <dyn Setup<From = T> as Setup>::From
isn't normalizing to &T
(because it's not WF). It's probably not possible to make this into a better error message... I wonder if we could suppress this somehow (delayed as a bug), but I'd rather just leave it be confusing until someone complains about it.
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
The Miri subtree was changed cc @rust-lang/miri Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
r? @lcnr |
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
43c1673
to
b6a9527
Compare
match *self_ty.kind() { | ||
// keep this in sync with assemble_fn_pointer_candidates until the old solver is removed. | ||
ty::FnDef(def_id, substs) => { | ||
let sig = tcx.fn_sig(def_id); | ||
if sig.skip_binder().is_fn_trait_compatible() | ||
&& tcx.codegen_fn_attrs(def_id).target_features.is_empty() | ||
if matches!(mode, Callable) |
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 it's easier to understand if we just match on (*self_ty.kind(), mode)
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 that for the closures, but I don't see how we can improve anything for FnDef
or FnPtr
.
compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #108080) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors try |
⌛ Trying commit 27891c5 with merge f82303294ca6add47eea966608ce02688928f965... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
🚨 Error: missing desired crates: {"https://crater-reports.s3.amazonaws.com/pr-107123/retry-regressed-list.txt"} 🆘 If you have any trouble with Crater please ping |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
The job Click to see the possible cause of the failure (guessed by this bot)
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
☔ The latest upstream changes (presumably #113260) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot author |
ping from triage - can you post your status on this PR? This PR has not received an update in a few months. Thank you! |
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
This trait will be used to mark functions as const evaluable (possibly by giving this trait a const generic bool parameter that is true for const and false otherwise)
I do not expect this trait to be ever visible to users, but to be a low level implementation detail
I have not moved
FnOnce::Output
toCallable
yet as that is very invasive. I think we may want to delay that until the old solver is gone.With appropriately improved (via
rustc_on_unimplemented
) error messages for whenCallable
doesn't hold, we could replace most, if not all, logic around number of argument mismatch and type of argument mismatch.pulled out of #101900