Perform autoref/autoderef on .await
#111773
Closed
Conversation
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
|
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
compiler/rustc_hir/src/hir.rs
Outdated
| } | ||
|
|
||
| impl PathSegmentRes { | ||
| pub fn get(&self, lang_items: &LanguageItems) -> Res { |
There was a problem hiding this comment.
This should just take TyCtxt
There was a problem hiding this comment.
Actually no, see my comment below about just using Res::Def by itself.
There was a problem hiding this comment.
TyCtxt is defined in rustc_middle, which rustc_hir doesn't depend on.
Comment on lines
850
to
1043
| res: hir::PathSegmentRes::LangItem( | ||
| hir::def::DefKind::AssocFn, | ||
| hir::LangItem::IntoFutureIntoFuture, | ||
| ), |
There was a problem hiding this comment.
Why do we need this? Why can't we just use Res::Def by itself?
Suggested change
| res: hir::PathSegmentRes::LangItem( | |
| hir::def::DefKind::AssocFn, | |
| hir::LangItem::IntoFutureIntoFuture, | |
| ), | |
| res: Res::Def(DefKind::AssocFn, tcx.require_lang_item(hir::LangItem::IntoFutureIntoFuture, Some(span))), |
There was a problem hiding this comment.
This would only break if, e.g., we had an .await call inside of core itself, which I guess may happen. But this really seems complicated for such a special use case.
There was a problem hiding this comment.
Using Res::def directly was my initial approach, but the compiler would crash when trying to get the lang items. I didn't investigate in detail, but I think lang items just aren't available from tcx at that point in compilation.
There was a problem hiding this comment.
Sigh, I forgot that there are calls to await in core.
|
This is a bit contrived, but introducing autoderef here does mean that inference is slightly affected: async fn test() {
let mut x = None;
loop {
if let Some(x) = x.as_mut() {
x.await;
}
x = Some(std::future::ready(1u32));
}
}Goes from passing to failing. This isn't necessarily a dealbreaker, but it should be kept in mind. We probably need to crater this to understand the impact. Also, we probably need to discuss whether this is behavior we want. I'll nominate this for wg-async I guess. |
compiler-errors
added
the
I-async-nominated
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
compiler-errors
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #112755) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Jules-Bertholet any updates on this? |
|
@Dylan-DPC This is blocked on wg-async coming to a decision on the desired behavior. |
Dylan-DPC
added
S-waiting-on-team
|
We discuss it in today's triage meeting and my idea that needs to be confirmed by some other of the wg is to discuss this in some open discussion that we are having for this kind of problem |
|
We have a discussion scheduled for this Thursday to go over this question. |
cjgillot
reviewed
Aug 24, 2023
compiler/rustc_hir/src/hir.rs
Outdated
| pub enum PathSegmentRes { | ||
| Res(Res), | ||
| LangItem(DefKind, LangItem), | ||
| } |
There was a problem hiding this comment.
Would it be simpler to add LangItem variant to Res?
| ProbeScope::DefId(self.tcx.lang_items().get(item).expect("missing lang item")) | ||
| } | ||
| _ => ProbeScope::TraitsInScope, | ||
| }; |
There was a problem hiding this comment.
I'm not really fond of this. What about editing the traits_in_scope map from HIR to ensure that Future is the only trait in scope for that HirId?
| // `match <expr>.into_future() { ... }` | ||
| let into_future_path = self.arena.alloc(hir::PathSegment { | ||
| ident: Ident::new(sym::into_future, self.lower_span(span)), | ||
| hir_id: expr_hir_id, |
There was a problem hiding this comment.
This should not reuse expr.hir_id. HirIds must be unique.
eholk
removed
the
I-async-nominated
|
@vincenzopalazzo @tmandry just checking progress: was this issue discussed? Anything to report here? thanks |
apiraino
removed
the
S-waiting-on-team
Dylan-DPC
added
S-blocked
This comment has been minimized.
This comment has been minimized.
Jules-Bertholet
force-pushed
the
await-autoref
branch
from
267fe19 to
1381a6b
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #118847) made this pull request unmergeable. Please resolve the merge conflicts. |
Jules-Bertholet
force-pushed
the
await-autoref
branch
from
1381a6b to
c4f35e5
Compare
This comment has been minimized.
This comment has been minimized.
Jules-Bertholet
force-pushed
the
await-autoref
branch
from
c4f35e5 to
2b4b416
Compare
This comment has been minimized.
This comment has been minimized.
|
We discussed this in the async triage meeting today. One thing we wanted to see here that we haven't yet is a full analysis of the crater run. We'd want to see all of the root regressions analyzed in the report. And for each of those, we'd be interested in seeing what the suggested fix or workaround would be. Would the plan be to target Rust 2024? If so, what would the migration strategy be? And, to confirm, would the plan be to maintain indefinitely the different desugaring in the older editions? |
traviscross
added
A-async-await
Jules-Bertholet
force-pushed
the
await-autoref
branch
from
2b4b416 to
62df79d
Compare
This comment has been minimized.
This comment has been minimized.
Jules-Bertholet
force-pushed
the
await-autoref
branch
from
62df79d to
33ae058
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
I'll put together something more complete when I have time, but to start with, here's an analysis of the single root regression responsible for ~95% of crater failures: let mut retry_delay = None;
for i in 1u32.. {
/* ... */
if let Some((delay, sleep)) = retry_delay.take() {
debug!("delaying for {delay:?}");
sleep.await;
}
/* ... */
retry_delay = Some((delay, sleep_impl.sleep(delay))); // type of `retry_delay` is constrained here
}Without this change, it compiles fine. With this change, we get: Adding a type annotation as suggested by the compiler makes the code compile again.
That's up to the relevant teams to decide. However, I am skeptical, as maintaining two implementations of
Automated migration would be difficult I think (don't know enough about type system implementation to say for sure). However, as the example above shows, the compiler error is pretty good, and it's easy to apply the fix manually. From the triage meeting minutes:
I have a mental model here that is perhaps slightly different. Non-type-directed postifx macros as proposed in rust-lang/rfcs#2442, as well as postfix match, don't impose any restrictions on the type of their receiver. They are analogous to a method of a trait with a blanket impl for any |
For the record, I don't see such a future being possible, at least not with an incredible amount of work and churn to the compiler implementation. Macro expansion happens before name resolution and operates on the AST level, and type checking operates on HIR only after macro expansion. I don't think we should be |
I don't see the implementation feasibility (or lack of such) of type-directed postfix macros as relevant to my argument. My point is, analogizing between |
What do you mean by that? You can write a macro_rules macro today that does the equivalent of |
Hmm, you're right I guess. But if you were to write such a postfix macro, you would probably use method call syntax ( |
no, that is not possible in general, because it won't know that it should pick up |
Where there's a will, there's a way: // On stable
use ::core::future::IntoFuture;
#[doc(hidden)]
pub trait IntoFutureUnambiguous: IntoFuture {
fn __into_futureඞunambiguousඞ(self) -> Self::IntoFuture;
}
impl<T: IntoFuture> IntoFutureUnambiguous for T {
fn __into_futureඞunambiguousඞ(self) -> Self::IntoFuture {
self.into_future()
}
}
#[macro_export]
macro_rules! into_future_with_autoref {
($expr:expr) => {
use $crate::IntoFutureUnambiguous;
($expr).__into_futureඞunambiguousඞ()
};
}// On nightly, with `decl_macro`
#![feature(decl_macro)]
#[doc(hidden)]
pub use core::future::IntoFuture;
pub macro into_future_with_autoref($expr:expr) {
use $crate::IntoFuture;
$expr.into_future()
}// On nightly, with `proc_macro_def_site`
#![feature(proc_macro_def_site)]
extern crate proc_macro;
use proc_macro::{Delimiter, Group, Ident, Punct, Spacing, Span, TokenStream, TokenTree};
#[proc_macro]
pub fn into_future_autoref(ts: TokenStream) -> TokenStream {
let def_span = Span::def_site();
TokenStream::from_iter([
TokenTree::Ident(Ident::new("use", def_span)),
TokenTree::Punct(Punct::new(':', Spacing::Joint)),
TokenTree::Punct(Punct::new(':', Spacing::Alone)),
TokenTree::Ident(Ident::new("core", def_span)),
TokenTree::Punct(Punct::new(':', Spacing::Joint)),
TokenTree::Punct(Punct::new(':', Spacing::Alone)),
TokenTree::Ident(Ident::new("future", def_span)),
TokenTree::Punct(Punct::new(':', Spacing::Joint)),
TokenTree::Punct(Punct::new(':', Spacing::Alone)),
TokenTree::Ident(Ident::new("IntoFuture", def_span)),
TokenTree::Punct(Punct::new(';', Spacing::Alone)),
TokenTree::Group(Group::new(Delimiter::None, ts)),
TokenTree::Punct(Punct::new('.', Spacing::Alone)),
TokenTree::Ident(Ident::new(
"into_future",
def_span.located_at(Span::call_site()),
)),
TokenTree::Group(Group::new(Delimiter::Parenthesis, TokenStream::new())),
])
} |
|
From the complexity of the solutions, I am wondering if this is worth it for the niche use case that it satisfies. If we have this complexity for single digit usages in crates.io, then it's not worth it imo. As Esteban noted, we can just produce high quality diagnostics instead. Not every use case needs lang support. A simpler language with diagnostics catching all the hypothetical language features and suggesting existing lang features is also nice |
|
The complexity of a good diagnostic would be comparable to or greater than that of a full implementation, no? |
|
No. We'd be extending an existing diagnostic, and it would not be part of the type system, so its complexity is not as relevent, as we can change it at any time. |
|
☔ The latest upstream changes (presumably #122317) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to close this, because I believe it is adding too much complexity to the compiler, while having not a motivating enough (in quantity) language use case. I am fine revisiting this implementation work should T-lang decide this is desirable, even if I personally do not think it is, or that T-lang should spend time on this, considering everything else they have on their plate. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds support for autoref/autoderef of the receiver of an
.await(before callingIntoFuture::into_future()).This PR is not ready to merge yet—the feature works, but diagnostics are regressed. I would like to get some feedback on my current approach, before investing more effort into it.
Fixes #111546.