Skip reborrows in AbstractConstBuilder #90529

b-naber · 2021-11-03T14:41:08Z

Temporary fix to prevent confusing diagnostics that refer to implicit borrows and derefs until we allow borrows and derefs on constant expressions.

r? @oli-obk

oli-obk · 2021-11-19T11:05:08Z

cc @BoxyUwU @lcnr

This is just about reborrows, right? I think I'd prefer to treat Rvalue::Ref(Place::Deref(_x)) as the same as a use of _x, but there may be implications I'm missing

oli-obk · 2021-11-19T11:06:30Z

Unless we never want these adjustments in general as they may just be noise

@bors try @rust-timer queue

rust-timer · 2021-11-19T11:06:31Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-19T11:06:41Z

⌛ Trying commit a50e129ce9c87622402210074bcec26b357c4834 with merge 74bfdbfa136949bcfff0227d9f9e3a76f6ff2402...

lcnr · 2021-11-19T12:02:09Z

compiler/rustc_mir_build/src/thir/cx/expr.rs

@@ -93,6 +94,20 @@ impl<'tcx> Cx<'tcx> {
    ) -> Expr<'tcx> {
        let Expr { temp_lifetime, .. } = expr;

+        // FIXME(generic_const_exprs): Once expressions of kind `AdressOf`,
+        // `Borrow` and `Deref` are supported, we can allow adjustments here
+        if let ExprKind::Literal {


I would assume that &*expr is used for borrowck reasons 🤔

While I am not too familiar with this code, this impl seems worrying to me because:

in places where we only have one of the adjustments, i.e. either &CONST or *CONST this is incorrect. Don't know if something like that is possible rn but it seems likely to me.

this probably causes some subtle changes to borrowck, though considering that const params can currently only be &'static Ty this is probably not too much of a problem 🤷

I completely misunderstood the issue, I initially thought that we would reject these borrows and derefs anyway, and this seems to be the case inside anonymous const blocks when generic_const_exprs is used, where we always throw an error when we encounter them during the construction of the AbstractConst (here), but treating all Borrow and Deref exprs as nops on all constants in general is not what we want obviously.

If we do want to allow reborrows, then the current implementation of raising the bug during the construction of the AbstractConst seems to actually be what we want here right? And we should just keep this until we allow Borrows and Derefs inside anonymous constant.

Maybe we should try to give a better error message during typeck here that makes the implicit adjustments explicit to the user?

bors · 2021-11-19T12:37:18Z

☀️ Try build successful - checks-actions
Build commit: 74bfdbfa136949bcfff0227d9f9e3a76f6ff2402 (74bfdbfa136949bcfff0227d9f9e3a76f6ff2402)

rust-timer · 2021-11-19T12:37:20Z

Queued 74bfdbfa136949bcfff0227d9f9e3a76f6ff2402 with parent e8423e6, future comparison URL.

rust-timer · 2021-11-19T14:07:57Z

Finished benchmarking commit (74bfdbfa136949bcfff0227d9f9e3a76f6ff2402): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

b-naber · 2021-11-30T12:19:52Z

@oli-obk and @lcnr

I changed this PR, we now don't apply reborrow adjustments whenever we're inside an anonconst context. I think this is what we want to do here, since we would reject these adjustments during the const evaluatable checks anyway.

I'm not sure whether solving this issue warrants the changes I had to make, introducing an expression check context solely for this might be considered unnecessary. Also I'm not sure whether this could result in performance problems, we would definitely need another timer run here.

Also I would clean this up a little, just wanted to see whether the idea in general would be viable.

oli-obk · 2021-12-02T08:07:43Z

This is just about reborrows, right? I think I'd prefer to treat Rvalue::Ref(Place::Deref(_x)) as the same as a use of _x, but there may be implications I'm missing

This is still true. For a temporary fix this PR adds way too much logic that needs to go away again. Can't we "just" make the abstract-const builder handle this case instead?

b-naber · 2021-12-02T11:53:29Z

@oli-obk Implemented skipping re-borrows during AbstractConst construction. This does work (I tried this without the previous commit, but decided to leave that commit here just in case). We would unify reborrowed expressions with expressions that don't contain a reborrow now, I think this should be fine, but maybe I'm missing something. Could this be a problem, @lcnr?

lcnr · 2021-12-05T10:20:27Z

I am fine with skipping reborrows, maybe add a FIXME(generic_const_exprs): Verify/explain why this is sound to the abstract const builder, than r=me on that change by itself. This PR still contains the previous changes, does it? Can these now be removed?

oli-obk · 2021-12-05T12:10:35Z

@bors r=lcnr

bors · 2021-12-05T12:10:36Z

📌 Commit 1777f43 has been approved by lcnr

oli-obk · 2021-12-05T12:10:48Z

@bors rollup

…askrgr Rollup of 4 pull requests Successful merges: - rust-lang#90529 (Skip reborrows in AbstractConstBuilder) - rust-lang#91437 (Pretty print empty blocks as {}) - rust-lang#91450 (Don't suggest types whose inner type is erroneous) - rust-lang#91535 (Stabilize `-Z emit-future-incompat` as `--json future-incompat`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2021-12-05T18:35:37Z

⌛ Testing commit 1777f43 with merge 772d51f...

rust-highfive assigned oli-obk Nov 3, 2021

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

b-naber mentioned this pull request Nov 3, 2021

Missing unconstrained generic constant error if adt_const_params feature is included #90531

Closed

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 19, 2021

lcnr reviewed Nov 19, 2021

View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 19, 2021

This comment has been minimized.

Sign in to view

b-naber force-pushed the reborrows-consts branch from 99b8f85 to c6fc1ac Compare November 30, 2021 12:28

b-naber force-pushed the reborrows-consts branch from 937c2fd to a07a33d Compare December 2, 2021 11:56

This comment has been minimized.

Sign in to view

add test

9520925

b-naber force-pushed the reborrows-consts branch from a07a33d to f09b549 Compare December 5, 2021 10:47

This comment has been minimized.

Sign in to view

b-naber added 2 commits December 5, 2021 12:15

skip reborrows during AbstractConst building

8ff50fe

bless tests

1777f43

b-naber force-pushed the reborrows-consts branch from f09b549 to 1777f43 Compare December 5, 2021 11:15

b-naber changed the title ~~Don't apply adjustments on constant parameter literals~~ Skip reborrows in AbstractConstBuilder Dec 5, 2021

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 Dec 5, 2021

matthiaskrgr mentioned this pull request Dec 5, 2021

Rollup of 4 pull requests #91555

Merged

bors merged commit 214b2a1 into rust-lang:master Dec 5, 2021

rustbot added this to the 1.59.0 milestone Dec 5, 2021

lcnr added the A-const-generics Area: const generics (parameters and arguments) label Dec 11, 2021

lcnr mentioned this pull request Dec 11, 2021

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

b-naber mentioned this pull request May 16, 2022

ICE with generic_const_exprs and adt_const_params #97047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip reborrows in AbstractConstBuilder #90529

Skip reborrows in AbstractConstBuilder #90529

b-naber commented Nov 3, 2021

oli-obk commented Nov 19, 2021

oli-obk commented Nov 19, 2021

rust-timer commented Nov 19, 2021

bors commented Nov 19, 2021

lcnr Nov 19, 2021

b-naber Nov 19, 2021

b-naber Nov 19, 2021 •

edited

Loading

bors commented Nov 19, 2021

rust-timer commented Nov 19, 2021

rust-timer commented Nov 19, 2021

This comment has been minimized.

b-naber commented Nov 30, 2021

oli-obk commented Dec 2, 2021

b-naber commented Dec 2, 2021 •

edited

Loading

This comment has been minimized.

lcnr commented Dec 5, 2021

This comment has been minimized.

oli-obk commented Dec 5, 2021

bors commented Dec 5, 2021

oli-obk commented Dec 5, 2021

bors commented Dec 5, 2021

Skip reborrows in AbstractConstBuilder #90529

Skip reborrows in AbstractConstBuilder #90529

Conversation

b-naber commented Nov 3, 2021

oli-obk commented Nov 19, 2021

oli-obk commented Nov 19, 2021

rust-timer commented Nov 19, 2021

bors commented Nov 19, 2021

lcnr Nov 19, 2021

Choose a reason for hiding this comment

b-naber Nov 19, 2021

Choose a reason for hiding this comment

b-naber Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

bors commented Nov 19, 2021

rust-timer commented Nov 19, 2021

rust-timer commented Nov 19, 2021

This comment has been minimized.

b-naber commented Nov 30, 2021

oli-obk commented Dec 2, 2021

b-naber commented Dec 2, 2021 • edited Loading

This comment has been minimized.

lcnr commented Dec 5, 2021

This comment has been minimized.

oli-obk commented Dec 5, 2021

bors commented Dec 5, 2021

oli-obk commented Dec 5, 2021

bors commented Dec 5, 2021

b-naber Nov 19, 2021 •

edited

Loading

b-naber commented Dec 2, 2021 •

edited

Loading