[MIR] use mir::repr::Constant in ExprKind::Repeat, close #29789

[fhahn]

This PR for #29789 uses rustc::repr::mir::Constant in ExprKind::Repeat, which seems to fit quite nicely. Is there a reason for not re-using that type?

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[nagisa]

Is there a reason for not re-using that type?

Well the whole point of that issue was that you’d have a type-system assisted guarantee to get Literal::Value and never see a Literal::Item. Reusing repr::Constant essentially changes nothing.

Which, thinking now, also points to a very trivial solution of simply using ConstVal for the count field.

[nagisa]

r? @nagisa

[nagisa]

Probably should just use the ConstVal here.

[fhahn]

Makes sense I guess. Thought the same thing, but this is my first contact with the MIR stuff. I'll update the PR

[fhahn]

The problem with chaning it to ConstVal is that this leads to a problem with the MIR visitor, which does only have a visit_constant function. Of course we could just try to construct a Constant object there, but that seems not too nice, but so does the alternative, adding a new method to the visitor.

[nagisa]

MIR visitor does not really walk through hair::ExprKinds, does it? There’s no pressing reason to visit the ConstVal repetition count either, were you put it into repr::Rvalue::Repeat as well, because it can be inspected as part of the whole Rvalue::Repeat inside the visit_rvalue (similarly as it is done with the visit_literal(Literal::Value)).

The issues with using ConstVal I could think of are loss of span and constant type information (later being not really important now, because repetition count is always usize, but might be important in the future, for e.g. analysis passes). With @oli-obk’s work, ConstVal is expected to eventually become typed, so loss of type would also, eventually, be fixed in time, leaving us with the span problem.

[fhahn]

Then after all, maybe it still make sense to add a new hair::Constant struct which contains the value as u64 as suggested by @oli-obk and the span (we could also store the type of the constant there of course)

[fhahn]

While using Constant instead of ExprRef does not provide as strong a guarantee as using ConstVal it still is a stronger garantuee type wise (excludes all other expr nodes)

[nagisa]

While using Constant instead of ExprRef does not provide as strong a guarantee as using ConstVal it still is a stronger garantuee type wise (excludes all other expr nodes)

That is certainly true, and, I think, a pretty good change to make now (provided we decide against ConstVal), but does not necessarily resolve the linked issue in full.

(That being said, I probably should update the aforementioned issue with more information on why it exists in the first place.)

[oli-obk]

Why not simply const eval and extract the integer and store it as a u64? Once I have typestrong const ints, I will put the usize enum there which will respect the target bitwidth.

[nagisa]

So I've thought (and dug around my memory) about it more and here’s some clarifications/thoughts:

Well the whole point of that issue was that you’d have a type-system assisted guarantee to get Literal::Value and never see a Literal::Item.

The original point (as indicated in this PR) was that repeat count must be a constant expression. Any of the proposals currently floating around (Constant, ConstVal and u64) seem to encode this constraint well enough.

Why not simply const eval and extract the integer and store it as a u64? Once I have typestrong const ints, I will put the usize enum there which will respect the target bitwidth.

Then after all, maybe it still make sense to add a new hair::Constant struct which contains the value as u64 as suggested by @oli-obk and the span

A possible option as well, but I don’t see any real improvement over just storing a ConstVal. Moreover using a u64 now would mean having to remember to replace it with a ConstVal later, once @oli-obk’s typed ConstVals happen… thus ending up as a superfluous code shuffle in the end.

Using u64 now would also involve doing more extensive changes (i.e. unpacking and repacking ConstVals or chainging repr::Rvalue::Repeat to use u64 as well). You cannot avoid creating hair::Constant to carry the type and span in either ( ConstVal/u64) case as well.

While using Constant instead of ExprRef does not provide as strong a guarantee as using ConstVal it still is a stronger garantuee type wise (excludes all other expr nodes)

I also am a little bit reluctant about making hair depend on repr for non-trivial structures more. General flow lowering into MIR is something like HIR (Exprs) → HAIR (ExprKinds) → MIR (repr::*) and the less dependent HAIR structures are on MIR, the better, I think.

---

cc @nikomatsakis, because MIR.

[oli-obk]

Moreover using a u64 now would mean having to remember to replace it with a ConstVal later, once @oli-obk’s typed ConstVals happen… thus ending up as a superfluous code shuffle in the end.

Actually, that makes it simpler to remember, since the extraction will break (there'll be no more ConstVal::Int or ConstVal::Uint)

[fhahn]

You cannot avoid creating hair::Constant to carry the type and span in either ( ConstVal/u64) case as well.

@nagisa do I understand correctly that creating a hair::Constant with a ConstVal, span and type is the prefered way?

[nagisa]

That seems to be the best option, yes.
On Jan 18, 2016 4:13 PM, "Florian Hahn" notifications@github.com wrote:

You cannot avoid creating hair::Constant to carry the type and span in
either ( ConstVal/u64) case as well.

@nagisa https://github.com/nagisa do I understand correctly that
creating a hair::Constant with a ConstVal, span and type is the prefered
way?

—
Reply to this email directly or view it on GitHub
#30944 (comment).

[fhahn]

I've pushed a new commit, which introduces mir::repr::TypedConstVal, which is used in mir::repr::Repeat and hair::Repeat. Currently we have to store the type and the constval in mir::repr::Repeat, because we need it here. The other alternativ would be to create a mir::repr::Constant here, which would require to create a literal again, which seems besides the point of the issue.

[bors]

☔ The latest upstream changes (presumably #31010) made this pull request unmergeable. Please resolve the merge conflicts.

[nagisa]

Since a Ty is stored in TypedConstVal, you’ll have to erase regions in the type. Erasing them inline (similarly to what’s done in SwitchTy case above) is fine, I think.

[nagisa]

I think things certainly look better than they did before.

r=me with the two comments resolved.

[fhahn]

@nagisa thanks for the feedback. I've pushed a new commit adressing the two comments.

[nagisa]

@bors r+ 9884ff1

[bors]

⌛ Testing commit 9884ff1 with merge 18b851b...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt