Stream: t-compiler

Topic: Separate unsized fn param from unsized locals #72029


Santiago Pastorino (May 09 2020 at 13:47, on Zulip):

Hey @oli @RalfJ opening this thread here to talk about #72029

Santiago Pastorino (May 09 2020 at 13:47, on Zulip):

won't be around today

Santiago Pastorino (May 09 2020 at 13:48, on Zulip):

just wanted to mention this part ...

Santiago Pastorino (May 09 2020 at 13:48, on Zulip):

There's a problem right now, I'm getting the following error ...

 Expected a gate test for the feature 'unsized_fn_params'.
Hint: create a failing test file named 'feature-gate-unsized_fn_params.rs'
      in the 'ui' test suite, with its failures due to
      missing usage of `#![feature(unsized_fn_params)]`.
Hint: If you already have such a test and don't want to rename it,
      you can also add a // gate-test-unsized_fn_params line to the test file.

But I'm not sure how can I add this test given that this thing is turned on, in liballoc and used there so unsure how I can create a test that doesn't use the flag and produce an error.

Santiago Pastorino (May 09 2020 at 13:48, on Zulip):

or if I should just do something to ignore this check or what

Santiago Pastorino (May 09 2020 at 13:49, on Zulip):

also cc @nikomatsakis @eddyb

Hanna Kruppe (May 09 2020 at 13:55, on Zulip):

I don't see the problem? Lots of feature are turned on in libcore/alloc/std and none of them should "leak" into other crates. In this case liballoc uses the feature gate to provide an impl that can be used on stable, but IIUC the feature gate controls the ability to declare e.g. fn foo(x: dyn SomeTrait) {}, and this should be testable.

pnkfelix (May 19 2020 at 20:11, on Zulip):

ping @Santiago Pastorino : I was looking over PR #72029

Santiago Pastorino (May 19 2020 at 20:12, on Zulip):

hey

pnkfelix (May 19 2020 at 20:12, on Zulip):

From the most recent comments you've posted there, it seems like you are not certain on why enablingfeature(unsized_fn_params) is not "fixing" one of the tests?

pnkfelix (May 19 2020 at 20:12, on Zulip):

I checked out your branch and did a grep of the code

pnkfelix (May 19 2020 at 20:13, on Zulip):

one thing that may be worth noting: fn arguments are a special case of pattern bindings

pnkfelix (May 19 2020 at 20:13, on Zulip):

and so I noticed a code path guarded by unsized_locals

pnkfelix (May 19 2020 at 20:13, on Zulip):

within a fn visit_pat method

pnkfelix (May 19 2020 at 20:14, on Zulip):

and that may need to also be enabled by your feature(unsized_fn_params) code. Or you may need to revise that bit of code to make sure that your new feature enables that path if and only if you are looking at a pat that happens to be a fn param

pnkfelix (May 19 2020 at 20:15, on Zulip):

I'm speaking of this line here

Santiago Pastorino (May 19 2020 at 20:16, on Zulip):

yeah, I thought that that may have been involved

pnkfelix (May 19 2020 at 20:17, on Zulip):

see also this other line here

pnkfelix (May 19 2020 at 20:17, on Zulip):

(of course if one goes too far down this path, all you'll manage to do is rename unsized_locals to a new name, without actually splitting it. :smile: )

Santiago Pastorino (May 19 2020 at 20:17, on Zulip):

hehe yeah

pnkfelix (May 19 2020 at 20:17, on Zulip):

anyway I think I'll mark this PR as waiting-on-author if you don't mind

Santiago Pastorino (May 19 2020 at 20:18, on Zulip):

sure, go ahead

pnkfelix (May 19 2020 at 20:18, on Zulip):

maybe first I'll transcribe this same note into the PR itself for people in general to see

Santiago Pastorino (May 19 2020 at 20:18, on Zulip):

pnkfelix said:

see also this other line here

what about this line in particular, sorry?

Santiago Pastorino (May 19 2020 at 20:18, on Zulip):

unsure if I got exactly what you meant

pnkfelix (May 19 2020 at 20:22, on Zulip):

that other line is another case where the unsized_locals feature is being checked

pnkfelix (May 19 2020 at 20:22, on Zulip):

but it is specifically about the inputs to some fn

pnkfelix (May 19 2020 at 20:22, on Zulip):

(although I'll admit I'm not sure what the impact of the .simple_ident().is_none() check is there)

pnkfelix (May 19 2020 at 20:23, on Zulip):

anyway my point is that its another spot you might need to inspect.

pnkfelix (May 19 2020 at 20:23, on Zulip):

I imagine you did already inspect all of these points

pnkfelix (May 19 2020 at 20:23, on Zulip):

but the fact that this code is specifically about the handling of formal fn params led me to think that it might be relevant.

Santiago Pastorino (May 19 2020 at 20:34, on Zulip):

yep, thanks for those pointers

pnkfelix (May 20 2020 at 16:43, on Zulip):

(You know, @Santiago Pastorino , one way to get unblocked here, and maybe just "the right thing" over all, could be to force the developer to opt into both flags. I.e., if you turn #[feature(unsized_fn_params)] on, maybe you have to turn on #[feature(unsized_locals)] too ...)

pnkfelix (May 20 2020 at 16:43, on Zulip):

I'm not sure how much I actually believe in the aforementioned approach, but it might be worth considering...

Santiago Pastorino (May 20 2020 at 17:04, on Zulip):

this was my first attempt

Santiago Pastorino (May 20 2020 at 17:05, on Zulip):

but I believe this is not really splitting things as we want

Santiago Pastorino (May 20 2020 at 17:05, on Zulip):

I'm going to investigate on this issue tomorrow probably

pnkfelix (May 20 2020 at 20:46, on Zulip):

@Santiago Pastorino : I just wanted to confirm: in the current state of your branch, you haven't added #[feature(unsized_fn_params)] to any of the tests, right?

pnkfelix (May 20 2020 at 20:47, on Zulip):

(nor even replaced #[feature(unsized_locals)] with #[feature(unsized_fn_params)] in any of the tests)

pnkfelix (May 20 2020 at 21:34, on Zulip):

also I think I have a straight-forward way to deal with the "am I within a pat that is part of a fn param" issue: add a flag to the visitor that is set/unset by an overridden fn visit_param on that same visitor. Something like this:

    fn visit_param(&mut self, param: &'tcx hir::Param<'tcx>) {
        self.within_fn_param = true;
        intravisit::walk_param(self, param);
        self.within_fn_param = false;
    }
pnkfelix (May 20 2020 at 21:38, on Zulip):

(I am playing with a local copy of your branch and may be able to post a suggestion commit in the near future)

Santiago Pastorino (May 21 2020 at 13:25, on Zulip):

pnkfelix said:

(nor even replaced #[feature(unsized_locals)] with #[feature(unsized_fn_params)] in any of the tests)

nope

Santiago Pastorino (May 21 2020 at 13:25, on Zulip):

pnkfelix said:

(I am playing with a local copy of your branch and may be able to post a suggestion commit in the near future)

:+1:

Santiago Pastorino (May 21 2020 at 13:25, on Zulip):

today I may have time for this

pnkfelix (May 22 2020 at 01:48, on Zulip):

hey @Santiago Pastorino , I have put some commits illustrating ways you can try to address your problems over here: https://github.com/pnkfelix/rust/tree/separate-unsized-locals

Santiago Pastorino (May 22 2020 at 13:10, on Zulip):

@pnkfelix :+1:, one question though, why did you added unsized_fn_params to INCOMPLETE_FEATURES here?

Santiago Pastorino (May 22 2020 at 13:11, on Zulip):

I thought we were just making unsized_params an INCOMPLETE_FEATURE but not unsized_fn_params

pnkfelix (May 22 2020 at 13:46, on Zulip):

because I don't know which one is the incomplete feature

pnkfelix (May 22 2020 at 13:47, on Zulip):

changing that back should reduce the size of the diff, since the tests won't need to be changed as much

pnkfelix (May 22 2020 at 13:48, on Zulip):

@Santiago Pastorino I'll let you take care of changing that back

pnkfelix (May 22 2020 at 13:49, on Zulip):

(or feel free to adapt whatever parts of my commits that you like. In any case, we definitely need more tests.)

pnkfelix (May 22 2020 at 13:50, on Zulip):

(since all this does is adapt existing tests; there are new cases that need to be tested specifically, I think...)

Santiago Pastorino (May 22 2020 at 15:02, on Zulip):

I'm going to take care of this today

Santiago Pastorino (May 22 2020 at 15:02, on Zulip):

thanks for the investigation

pnkfelix (May 22 2020 at 15:02, on Zulip):

at the very least, I did manage to do a bootstrap build and test with the branch I linked above

pnkfelix (May 22 2020 at 15:03, on Zulip):

also, by the way

pnkfelix (May 22 2020 at 15:03, on Zulip):

we should consider still adding unsized_fn_params to INCOMPLETE_FEATURES

pnkfelix (May 22 2020 at 15:04, on Zulip):

and have taking it out of INCOMPLETE_FEATURES be a separate PR

pnkfelix (May 22 2020 at 15:04, on Zulip):

or ... well

pnkfelix (May 22 2020 at 15:04, on Zulip):

was unsized_locals already in INCOMPLETE_FEATURES ?

pnkfelix (May 22 2020 at 15:04, on Zulip):

oh it wasn't

pnkfelix (May 22 2020 at 15:04, on Zulip):

okay then never mind

Santiago Pastorino (May 22 2020 at 16:28, on Zulip):

:+1:

Santiago Pastorino (May 22 2020 at 19:37, on Zulip):

@pnkfelix just had some minutes for this and should be ready now

pnkfelix (May 22 2020 at 19:45, on Zulip):

@Santiago Pastorino did you want to remove the draft tag from the PR?

Santiago Pastorino (May 22 2020 at 19:45, on Zulip):

yes and I'm rebasing

Santiago Pastorino (May 22 2020 at 19:45, on Zulip):

there are conflicts with master

Santiago Pastorino (May 22 2020 at 19:45, on Zulip):

running tests again on ui and all that :)

Santiago Pastorino (May 22 2020 at 21:11, on Zulip):

@pnkfelix this is ready for review

RalfJ (May 23 2020 at 09:40, on Zulip):

Santiago Pastorino said:

I thought we were just making unsized_params an INCOMPLETE_FEATURE but not unsized_fn_params

what is unsized_params, I thought there is just unsized_locals and unsized_fn_params?

Santiago Pastorino (May 23 2020 at 12:12, on Zulip):

I meant unsized_locals :)

Santiago Pastorino (May 23 2020 at 21:10, on Zulip):

@RalfJ addessed your comments but didn't get the one about enabling the feature in liballoc and libcore, tests should also be passing now. Let's wait for the CI.

pnkfelix (May 26 2020 at 18:29, on Zulip):

@Santiago Pastorino did you want to talk about the PR here?

Santiago Pastorino (May 26 2020 at 18:31, on Zulip):

yes

Santiago Pastorino (May 26 2020 at 18:31, on Zulip):

couldn't read and think about Ralf's comments yet

Santiago Pastorino (May 26 2020 at 18:31, on Zulip):

let me check those again

pnkfelix (May 26 2020 at 18:31, on Zulip):

I don't think I have bandwidth to try to look at the code itself

Santiago Pastorino (May 26 2020 at 18:31, on Zulip):

but it seems that this is close modulo Ralf last comment

pnkfelix (May 26 2020 at 18:32, on Zulip):

but what I would recommend, perhaps, is some methodologies for narrowing down which part of the change was wrong

pnkfelix (May 26 2020 at 18:32, on Zulip):

namely, if you add a Z flag (or set of Z flags) for controlling how each changed boolean test is handled

pnkfelix (May 26 2020 at 18:32, on Zulip):

then you will be able to quickly experiment, on the command line, with different semantics

pnkfelix (May 26 2020 at 18:32, on Zulip):

without having to wait for a rebuild in between each one

pnkfelix (May 26 2020 at 18:33, on Zulip):

Its sort of overkill, arguably. Like using a bazooka to kill a moth

Santiago Pastorino (May 26 2020 at 18:33, on Zulip):

hehehe

pnkfelix (May 26 2020 at 18:34, on Zulip):

Usually I would prefer to try to sit down and understand the code directly, perhaps with some debug! instrumentation added. But sometimes the best option is to add some way to do the kind of direct experimentation I am suggesting.

pnkfelix (May 26 2020 at 18:36, on Zulip):

Like, there are something like five or six if statements that were modified here to look at a different feature flag. And my memory is that there weren't that many others that were left unchanged

pnkfelix (May 26 2020 at 18:38, on Zulip):

so if you add a -Z take-control-of-tests=<number>, and then use the individual bits of <number> to dictate whether each of those if-statements (including the ones currently unchanged) use .unsized_locals alone, or .unsized_fn_params alone, or the OR of them, then you can feed things like -Z take-control-of-tests=0x2cafe or some such to control them.

pnkfelix (May 26 2020 at 18:39, on Zulip):

(am I unhappy that I'm making this suggestion? Yes, I am. I would prefer if the rebuild time were fast enough that such a suggestion would be laughable...)

Santiago Pastorino (May 26 2020 at 18:43, on Zulip):

hehehe

Santiago Pastorino (May 26 2020 at 18:43, on Zulip):

but yeah good points

pnkfelix (May 26 2020 at 18:44, on Zulip):

alternatively, you could just start by adding debug! instrumentation to all of the aforementioned points, and then try to dissect the log on the cases of interst

Santiago Pastorino (May 26 2020 at 19:08, on Zulip):

yep

Santiago Pastorino (May 26 2020 at 19:08, on Zulip):

will try that as soon as I finish other stuff I'm doing

Last update: May 29 2020 at 17:15UTC