generic_const_exprs: use thir for abstract consts instead of mir by BoxyUwU · Pull Request #88709 · rust-lang/rust (original) (raw)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExprKind::Cast also contains a Ty and afaict you don't look at thir::Pat at all 🤔

maybe it makes sense to add a fn visit_ty to the Visitor trait and only manually implement visit_ty and visit_const here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExprKind::Cast just contains an ExprId the type of the cast is from expr.ty so that's correct.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did forget about thir::Pat i think I thought it was unnecessary as i dont think you can currently use a pattern in a generic constant but that doesn't matter for checking this

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that visiting patterns is actually important, I cant think of anything in patterns that could be generic that wouldnt be handled by either visit_const from when a pattern is a constant or visit_expr for what's being matched on/let = on

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woups, meant ExprKind::Call 😅

I personally don't think thir::Pat is too important either, or well, only for is_polymorphic (which we should be able to remove from mir::Body now). But the current impls seems fragile if we ever add a Ty to some other ExprKind or something, so i would like a safer approach here

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, I assumed that the ty in ExprKind::Call was the same as the type of the fun expr which is why I didnt visit that

edit: looking at code it does infact seem so, it's fine to ignore the ty in ExprKind::Call when checking if the thir is polymorphic as we'll visit that ty when we visit the fun expr

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed how we check if thir is polymorphic to explicitly match on ExprKind and PatKind some of the arms are doing definitely_has_param_types_or_consts but im not really sure it actually makes any difference to be doing so

edit: yeah as far as I can tell these match arms arent doing anything so I've removed them which leaves us with two match statements that exhaustively match on ExprKind and PatKind without actually doing anything which seems incredibly silly but does solve the problem of "what happens if someone adds a Ty field to an ExprKind variant"

think it might just be smarter to add tests that we give an "unsupported operation in generic constant" error for each of these variants

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two match statements that exhaustively match on ExprKind and PatKind without actually doing anything which seems incredibly silly

🤔 that doesn't make me too happy. not sure how easy and useful it is to add tests for this, but I think that having an exhaustive match here is more annoying than helpful '^^

imo its fine to just remove the match, we have enough time on nightly to see whether this is a problem