Separate unsized fn params from unsized locals #74971

JohnTitor · 2020-07-31T08:23:00Z

Taking over #72029 in favor of #72029 (comment).
Fixes #71694, closes #72029

spastorino · 2020-07-31T13:53:14Z

@JohnTitor thanks for taking this over because I couldn't check this issue again due to lack of time. This looks good to me but let's see what @RalfJung and @pnkfelix say about it.

bjorn3 · 2020-08-02T09:45:41Z

src/librustc_typeck/check/mod.rs

+            } else {
+                if !self.fcx.tcx.features().unsized_locals {
+                    self.fcx.require_type_is_sized(var_ty, p.span, traits::VariableType(p.hir_id));
+                }
            }


What about fn a(box box b: Box<Box<[u8]>>) {}? I think that should keep using unsized_locals and not unsized_fn_params.

Yes, I assume that pattern will generate MIR with unsized locals.

Hmm yeah, confirmed that it won't work well in this PR. How can we catch such a case?

I have no familiarity with HIR, but it must be possible to somehow check if the function argument is a "trivial" pattern, right?

The params should all have a pat that's immediately Binding.

I have no familiarity with HIR, but it must be possible to somehow check if the function argument is a "trivial" pattern, right?

I think p.simple_ident could be used here as well as https://github.com/rust-lang/rust/pull/74971/files#diff-1d1b0d29a2e8da97c6bfb6e364d920c7R1378-R1381?

Maybe?^^ I'm really the wrong person to ask such questions.

Hmm, anyway thanks for pointing out :) Let's move this topic to Zulip so that the we can get some eyes from other members...

(summarizing my discussion with @spastorino on Zulip)
I think a bigger issue than special-casing the shape of the pattern, is that within_fn_param applies recursively to every nested pattern in a fn parameter, instead of to the parameter pattern itself.

So my suggestion is to replace it with a "shallow" version (bikeshed name: outermost_fn_param_pat) that visit_pat temporarily sets back to false around its walk_pat call, so that the #![feature(unsized_fn_params)] path is only taken for fn parameters themselves, not any other patterns nested in them.

RalfJung · 2020-08-05T13:04:43Z

The key test for this PR is a file that enables unsized_fn_params but not unsized_locals, and then checks that all uses of unsizd locals except for fn params error. I can't see this PR add such a test, but maybe I missed it in the various test suite changes?

bors · 2020-08-08T04:13:00Z

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

JohnCSimon · 2020-08-25T05:13:33Z

@JohnTitor - Can you please address the merge conflict so it can be reviewed? Thank you.

JohnTitor · 2020-08-25T05:15:51Z

This still can be reviewed even if it has a merge conflict. I'll resolve it once we come up with a solution for the above point. See also the Zulip topic.

RalfJung · 2020-08-25T07:44:50Z

Which Zulip topic?

AFAIK this just needs a check ensuring that the pattern is sufficiently simple.

Alternatively, is there a way to check this on the MIR? The pattern has been compiled away there, so that makes it trivial to check if there are just arguments or also locals that are unsized.

Dylan-DPC-zz · 2020-08-25T15:01:13Z

@JohnTitor generally, reviewers are more likely not to review pull requests if it has conflicts

spastorino · 2020-08-26T18:05:02Z

It sounded to me that this is more waiting on author than waiting on team, @JohnTitor can you confirm that?.

Dylan-DPC-zz · 2020-08-27T01:16:18Z

@spastorino yeah it seemed like the author wanted some help from compiler team, hence marked it as that but after that reading through the conversation, it should be waiting on author :D

crlf0710 · 2020-09-11T16:04:24Z

I think zulip topic is this:
https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Separate.20unsized.20fn.20param.20from.20unsized.20locals.20.2372029

And there's even more merge conflicts now...

Dylan-DPC-zz · 2020-09-24T00:03:46Z

I'm currently reworking this, I should have a PR up in a few days.

spastorino · 2020-09-24T22:50:54Z

@Dylan-DPC have rebased this here https://github.com/spastorino/rust/tree/separate-unsized-locals2, please let me know if you're going to continue with this, otherwise I can continue.

Dylan-DPC-zz · 2020-09-25T00:10:22Z

@spastorino since you have the headstart, you can proceed not a problem

@RalfJung

… r=oli-obk Separate unsized locals Closes rust-lang#71694 Takes over again rust-lang#72029 and rust-lang#74971 cc @RalfJung @oli-obk @pnkfelix @eddyb as they've participated in previous reviews of this PR.

spastorino and others added 8 commits July 31, 2020 13:45

Add unsized_fn_params feature

45f03da

Add unsized_locals to INCOMPLETE_FEATURES list

5392249

This flag is not really needed in the test

116357c

Make tidy happy

7cf3ac9

Using unsized_local feature is not needed in these tests

6f6db07

Fix unstable-book doc tests

659db8e

Fix rebase-fallout and bless tests

39716f1

Do not depend on unsized_locals except for bootstrap

93b5a7e

rust-highfive assigned pnkfelix Jul 31, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2020

spastorino approved these changes Jul 31, 2020

View reviewed changes

Dylan-DPC-zz mentioned this pull request Aug 1, 2020

Separate unsized fn param from unsized locals #72029

Closed

bjorn3 reviewed Aug 2, 2020

View reviewed changes

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2020

JohnTitor added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 25, 2020

Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2020

spastorino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 26, 2020

Dylan-DPC-zz removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Aug 27, 2020

Dylan-DPC-zz added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 27, 2020

Dylan-DPC-zz closed this Sep 24, 2020

spastorino mentioned this pull request Oct 20, 2020

Separate unsized locals #78152

Merged

JohnTitor deleted the separate-unsized-locals branch June 7, 2021 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate unsized fn params from unsized locals #74971

Separate unsized fn params from unsized locals #74971

JohnTitor commented Jul 31, 2020 •

edited

Loading

spastorino commented Jul 31, 2020

bjorn3 Aug 2, 2020

RalfJung Aug 5, 2020

JohnTitor Aug 5, 2020

RalfJung Aug 6, 2020

RalfJung Aug 6, 2020

JohnTitor Aug 6, 2020

RalfJung Aug 6, 2020

JohnTitor Aug 6, 2020

eddyb Oct 20, 2020

RalfJung commented Aug 5, 2020

bors commented Aug 8, 2020

JohnCSimon commented Aug 25, 2020

JohnTitor commented Aug 25, 2020 •

edited

Loading

RalfJung commented Aug 25, 2020

Dylan-DPC-zz commented Aug 25, 2020

spastorino commented Aug 26, 2020

Dylan-DPC-zz commented Aug 27, 2020

crlf0710 commented Sep 11, 2020

Dylan-DPC-zz commented Sep 24, 2020

spastorino commented Sep 24, 2020 •

edited

Loading

Dylan-DPC-zz commented Sep 25, 2020

Separate unsized fn params from unsized locals #74971

Separate unsized fn params from unsized locals #74971

Conversation

JohnTitor commented Jul 31, 2020 • edited Loading

spastorino commented Jul 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 5, 2020

bors commented Aug 8, 2020

JohnCSimon commented Aug 25, 2020

JohnTitor commented Aug 25, 2020 • edited Loading

RalfJung commented Aug 25, 2020

Dylan-DPC-zz commented Aug 25, 2020

spastorino commented Aug 26, 2020

Dylan-DPC-zz commented Aug 27, 2020

crlf0710 commented Sep 11, 2020

Dylan-DPC-zz commented Sep 24, 2020

spastorino commented Sep 24, 2020 • edited Loading

Dylan-DPC-zz commented Sep 25, 2020

JohnTitor commented Jul 31, 2020 •

edited

Loading

JohnTitor commented Aug 25, 2020 •

edited

Loading

spastorino commented Sep 24, 2020 •

edited

Loading