-
Notifications
You must be signed in to change notification settings - Fork 14k
remove support for typeof
#148256
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: main
Are you sure you want to change the base?
remove support for typeof
#148256
Conversation
typeof
compiler/rustc_ast/src/token.rs
Outdated
| !ident_token.is_reserved_ident() | ||
| || ident_token.is_path_segment_keyword() | ||
| || [kw::Underscore, kw::For, kw::Impl, kw::Fn, kw::Unsafe, kw::Extern, kw::Typeof, kw::Dyn] | ||
| || [kw::Underscore, kw::For, kw::Impl, kw::Fn, kw::Unsafe, kw::Extern, kw::Dyn] |
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.
q: is there a chance this affects macros?
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.
Yes, the following artificial snippet will go from pass→fail:
macro_rules! ck { ($ty:ty) => {}; }
ck!(typeof(0));After this PR it'll fail with no rules expected keyword `typeof` (and hypothetically if typeof were to be demoted to a normal identifier, it would start failing with expected type, found `0`).
However, it's very unlikely anybody is relying on it. So it's not really a concern. MBE is notoriously forward incompatible.
| | ty::CoroutineWitness(..) | ||
| | ty::Alias(ty::Free, _) | ||
| | ty::Bound(..) | ||
| | ty::Placeholder(_) | ||
| | ty::Infer(_) => { |
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.
q: How is this related to this PR?
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.
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.
With the removal of typeof inference, it should now be impossible to sneak these unnameable types into an impl (even with TAIT you can't do that IINM)
Yes, there's currently no way to get unnameable types into impl headers or encounter them in coherence, and it's nice to be able to reason about that
| // Reference | ||
| self.expect_and()?; | ||
| self.parse_borrowed_pointee()? | ||
| } else if self.eat_keyword_noexpect(kw::Typeof) { |
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 may be nice to keep a precise error message here and recover into an error type
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.
how do you do that? It's not immediately obvious to me how to impl that and don't want to spend much time on figuring this out 🤔
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.
how do you do that? It's not immediately obvious to me how to impl that
Instead of removing this eat_keyword_noexpect branch + fn parse_typeof_ty, keep both but change parse_typeof_ty to no longer return a TyKind::Typeof(expr) but instead a TyKind::Err(guar) (this variant already exists) where obv. let guar = dcx.struct_span_err(…, "…");.
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.
This made me realize that we do accept this on stable (and have been since ~forever), lol:
#[cfg(false)]
type X = typeof(0);So forget what I said about MBEs, this is a far more obvious demonstration why removing typeof is a theoretical breaking change, not that anybody would care ^^.
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.
done that I think? @fmease do you think this is relevant enough to do a crater run or sth like that here?
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 the most likely scenario would be something like #[mydsl] fn f() { … typeof(…) … } but even that seems farfetched (esp. since this would still compile if mydsl is a proc-macro attr).
I've also skimmed the results of GH Search: /(?-i)\btypeof\([^)"]/ language:Rust but couldn't find anything that would break.
So, I don't think this warrants a crater run, let's just ship it.
|
This looks good to me, |
|
HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
see rust-lang/compiler-team#940 closes #148700
This also enables checks for invariants previously broken by
typeofagain.r? types