Revise docs for thir::PatKind::ExpandedConstant

[Zalathar]

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

[Zalathar]

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.

[Nadrieril]

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?

[Zalathar]

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.

[Nadrieril]

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

[Nadrieril]

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.

[Nadrieril]

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".

[Zalathar]

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.)

[Nadrieril]

Good point! feels ok to add an ExpandedConstant node to that, it might come useful for exhaustiveness diagnostics maybe?

[Zalathar]

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.

[Nadrieril]

Interesting point yes. I'm not sure why we chose to box thir::Pat when the rest of Thir uses ids.

[Zalathar]

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.

[Nadrieril]

@Zalathar I'm happy to merge this as-is, or with some of my suggestions. What would you prefer?