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

BoxyUwU · 2021-09-07T01:03:28Z

Changes AbstractConst building to use thir instead of mir so that there's less chance of consts unifying when they shouldn't because lowering to mir dropped information (see abstract-consts-as-cast-5.rs test)

r? @lcnr

lcnr

💖 💖 💖

a bunch of small nits which might even be slightly too pedantic.

The error messages for overly complex constants are a bit worse now, but improving them can be done in followup PRs.

compiler/rustc_middle/src/mir/query.rs

compiler/rustc_middle/src/thir/visit.rs

lcnr · 2021-09-07T08:31:56Z

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

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.

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

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

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

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

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

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

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

compiler/rustc_trait_selection/src/traits/const_evaluatable.rs

bors · 2021-09-09T00:26:29Z

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

compiler/rustc_metadata/src/rmeta/encoder.rs

lcnr · 2021-09-09T07:43:41Z

src/test/ui/const-generics/generic_const_exprs/unify-op-with-fn-call.rs

Suggested change

fn foo<const N: Foo>(a: Evaluatable<{ N + N }>) {

bar::<{ std::ops::Add::add(N, N) }>();

}

fn bar<const N: Foo>() {}

fn foo<const N: Foo>(a: Evaluatable<{ N + N }>) {

bar::<{ std::ops::Add::add(N, N) }>();

}

fn bar<const N: Foo>() {}

fn baz<const N: usize>(a: [u8; N + N]) {

biz::<{ std::ops::Add::add(N, N) }>();

}

fn biz<const N: usize>() {}

interested in seeing this tested for builtin types

hmm yeah Add::add(N, N) doesnt unify with N + N for builtin types

I think we could totally support this by just making Node::BinOp(..) for operator functions but probably ought to do that in a separate PR rather than alongside this mir -> thir refactor (or alternatively get rid of Node::BinOp and just lower to Node::Function(..) but either way there is probably solutions)

lcnr · 2021-09-09T07:48:44Z

some more nits and the match in IsThirPolymorphic, after that this seems ready for merge to me

lcnr · 2021-09-10T08:35:59Z

@rust-lang/project-const-generics we're now using thir instead of mir for the unification of generic constants

@bors r+

bors · 2021-09-10T08:36:01Z

📌 Commit 8295e4a has been approved by lcnr

bors · 2021-09-10T08:36:01Z

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

generic_const_exprs: use thir for abstract consts instead of mir not sure if this is handling some of the weirder `thir::ExprKind` correctly in `recurse_build` (it probably isnt) r? `@lcnr`

generic_const_exprs: use thir for abstract consts instead of mir not sure if this is handling some of the weirder `thir::ExprKind` correctly in `recurse_build` (it probably isnt) r? ``@lcnr``

generic_const_exprs: use thir for abstract consts instead of mir Changes `AbstractConst` building to use `thir` instead of `mir` so that there's less chance of consts unifying when they shouldn't because lowering to mir dropped information (see `abstract-consts-as-cast-5.rs` test) r? `@lcnr`

compiler/rustc_mir_build/src/thir/mod.rs

…arth Rollup of 7 pull requests Successful merges: - rust-lang#88336 ( Detect stricter constraints on gats where clauses in impls vs trait) - rust-lang#88677 (rustc: Remove local variable IDs from `Export`s) - rust-lang#88699 (Remove extra unshallow from cherry-pick checker) - rust-lang#88709 (generic_const_exprs: use thir for abstract consts instead of mir) - rust-lang#88711 (Rework DepthFirstSearch API) - rust-lang#88810 (rustdoc: Cleanup `clean` part 1) - rust-lang#88813 (explicitly link to external `ena` docs) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Manishearth · 2021-09-12T18:47:36Z

The rollup caused an instruction count increase on perf, do you think that's due to this PR?

#88881

https://perf.rust-lang.org/compare.html?start=9ef27bf7dc50a8b51435579b4f2e86f7ee3f7a94&end=c7dbe7a830100c70d59994fd940bf75bb6e39b39

BoxyUwU · 2021-09-12T19:10:36Z

Im not sure how this would cause a perf regression given all the changed code is gated behind feature(generic_const_exprs)

rust-highfive assigned lcnr Sep 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2021

BoxyUwU force-pushed the thir-abstract-const branch from f0064e4 to 578387d Compare September 7, 2021 02:13

lcnr reviewed Sep 7, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

BoxyUwU force-pushed the thir-abstract-const branch 2 times, most recently from d08155a to afa9b39 Compare September 8, 2021 19:01

BoxyUwU added 17 commits September 9, 2021 01:32

WIP state

2987f4b

as casts and block exprs

9b29138

dont support blocks

4483c2b

bless stderr

47b16f4

tidy

c170dcf

move thir visitor to rustc_middle

08e8644

dont build abstract const for monomorphic consts

fc63e9a

handle ExprKind::NeverToAny

4cbcb09

remove WorkNode

1f57f8b

remove debug stmts

15101c8

rename mir -> thir around abstract consts

406d2ab

remove comment

79be080

nits

955e2b2

add a CastKind to Node::Cast

8c7954d

resolve from_hir_call FIXME

3212734

fmt

cd2915e

CI please

fd9bb30

BoxyUwU force-pushed the thir-abstract-const branch from afa9b39 to 25c5033 Compare September 9, 2021 00:44

lcnr reviewed Sep 9, 2021

View reviewed changes

BoxyUwU force-pushed the thir-abstract-const branch from 25c5033 to ee48c18 Compare September 9, 2021 14:42

add test for builtin types N + N unifying with fn call

8295e4a

BoxyUwU force-pushed the thir-abstract-const branch from ee48c18 to 8295e4a Compare September 9, 2021 14:44

BoxyUwU marked this pull request as ready for review September 9, 2021 17:54

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2021

This was referenced Sep 11, 2021

Rollup of 11 pull requests #88853

Closed

Rollup of 11 pull requests #88854

Closed

Manishearth mentioned this pull request Sep 12, 2021

Rollup of 8 pull requests #88874

Closed

cjgillot reviewed Sep 12, 2021

View reviewed changes

compiler/rustc_mir_build/src/thir/mod.rs Show resolved Hide resolved

Manishearth mentioned this pull request Sep 12, 2021

Rollup of 7 pull requests #88881

Merged

bors merged commit f5ac5ca into rust-lang:master Sep 12, 2021

rustbot added this to the 1.57.0 milestone Sep 12, 2021

lcnr added A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` labels Dec 11, 2021

lcnr mentioned this pull request Dec 11, 2021

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

-fn foo<const N: Foo>(a: Evaluatable<{ N + N }>) {
-    bar::<{ std::ops::Add::add(N, N) }>();
-}
-fn bar<const N: Foo>() {}
+fn foo<const N: Foo>(a: Evaluatable<{ N + N }>) {
+    bar::<{ std::ops::Add::add(N, N) }>();
+}
+fn bar<const N: Foo>() {}
+fn baz<const N: usize>(a: [u8; N + N]) {
+    biz::<{ std::ops::Add::add(N, N) }>();
+}
+fn biz<const N: usize>() {}

Uh oh!

Conversation

BoxyUwU commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Sep 9, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcnr commented Sep 9, 2021

Uh oh!

lcnr commented Sep 10, 2021

Uh oh!

bors commented Sep 10, 2021

Uh oh!

bors commented Sep 10, 2021

Uh oh!

Uh oh!

Manishearth commented Sep 12, 2021

Uh oh!

BoxyUwU commented Sep 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

BoxyUwU commented Sep 7, 2021 •

edited

Loading

BoxyUwU Sep 7, 2021 •

edited

Loading

BoxyUwU Sep 8, 2021 •

edited

Loading

BoxyUwU Sep 9, 2021 •

edited

Loading