Revise docs for thir::PatKind::ExpandedConstant by Zalathar · Pull Request #136612 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
One of the reasons I pursued #136529 was that I found the existing docs for ExpandedConstant to be not very helpful when I was trying to understand why that pattern node kind exists, and what it does.
These new docs try to explain things in more detail, with a particular focus on the points that I personally found confusing.
r? Nadrieril
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
I have mixed feelings about whether to mention const_to_pat.
On one hand, it's useful to know that if an ExpandedConstant exists, then it typically wraps the result of const_to_pat (except in the case of range patterns, where the story is a bit more subtle).
On the other hand, there are plenty of cases where the result of const_to_pat doesn't get an ExpandedConstant wrapper, so I'm not sure how to describe the connection without accidentally giving the wrong impression.
On the other hand, there are plenty of cases where the result of const_to_pat doesn't get an ExpandedConstant wrapper
I was not aware of that, do you have an example? Could we add the wrapper consistently?
Literals are lowered via const_to_pat, but they don't have a wrapper.
(There is an existing diagnostic for numeric literals in irrefutable patterns that could theoretically want such a wrapper, but in practice it can get away with just detecting PatKind::Constant and inspecting the source snippet.)
I think that when I said “plenty of cases”, I was also thinking of paths like Option::None, which are lowered by a similar code path (lower_lit). But those don't actually involve const_to_pat, so I was partly mistaken there.
Hmm, it's fine by me if we don't count a literal as an "expanded constant" since there was no expansion done: it becomes a PatKind::Constant directly. I don't know if that would be clear to everyone though
| Constant { |
|---|
| value: mir::Const<'tcx>, |
| }, |
| /// Pattern obtained by converting a constant (inline or named) to its pattern |
| /// representation using `const_to_pat`. |
| /// Marker node that wraps the result of converting some kind of "constant" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like:
/// A constant in a pattern gets expanded into a thir::Pat that exactly matches its value. For
/// non-trivial constants (i.e. those that aren't literals handled by PatKind::Constant), we wrap
/// it in an ExpandedConstant node. Unsafeck and some diagnostics rely on the presence of this
/// node.
and then the note about range patterns.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since apparently we're not adding this node for all constants, we could add "byte string literals are another exception: we don't add an ExpandedConstant node today".
Hmm, it's fine by me if we don't count a literal as an "expanded constant" since there was no expansion done: it becomes a PatKind::Constant directly. I don't know if that would be clear to everyone though
Also there is at least one kind of literal that gets lowered/expanded to a non-trivial non-Constant pattern: byte string literals like b"foo" become an array pattern or slice pattern filled with individual constants for each byte.
(I don't think we have any diagnostics or static checks that specifically care about that at the moment, but it's not hard to imagine someone wanting that in the future.)
Good point! feels ok to add an ExpandedConstant node to that, it might come useful for exhaustiveness diagnostics maybe?
Side note: Something we can't do right now, but could do in the future if we had thir::PatId, is to put this kind of “side data” in a separate hashtable indexed by pattern node ID.
If that were the case, we wouldn't even need to worry about peeling off these wrapper nodes in code that wants to pattern-match on node type. Code that cares about the side data would check it, and code that doesn't care would just ignore it.
Interesting point yes. I'm not sure why we chose to box thir::Pat when the rest of Thir uses ids.
Interesting point yes. I'm not sure why we chose to box thir::Pat when the rest of Thir uses ids.
I think it's just an implementation detail that has historically been hard to change.
When I have tried to switch over in the past, I always got stuck on some messy detail. Some of my THIR cleanups have been motivated by trying to resolve those details, so that switching over to PatId gradually becomes more feasible.
@Zalathar I'm happy to merge this as-is, or with some of my suggestions. What would you prefer?
Labels
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.