Stream: t-compiler/wg-polonius

Topic: move error reporting


nikomatsakis (Aug 03 2020 at 14:50, on Zulip):

Hey @Albin Stjerna

Albin Stjerna (Aug 03 2020 at 14:51, on Zulip):

hey

Albin Stjerna (Aug 03 2020 at 14:52, on Zulip):

Ok, so I started looking at how move errors work currently, and it's the same kind of "moving balls under cups" type of situation as I remember from before

Albin Stjerna (Aug 03 2020 at 14:52, on Zulip):

Various bits of data going into various data structures, etc

nikomatsakis (Aug 03 2020 at 14:52, on Zulip):

heh

nikomatsakis (Aug 03 2020 at 14:52, on Zulip):

yes

Albin Stjerna (Aug 03 2020 at 14:53, on Zulip):

But as far as I understand it, move errors are generated early, before even the Polonius facts are assembled

nikomatsakis (Aug 03 2020 at 14:53, on Zulip):

I think at some point I was trying to keep a line so that the move error reporting largely recomputed details that it needed,

nikomatsakis (Aug 03 2020 at 14:53, on Zulip):

partly to make polonius error reporting easier

Albin Stjerna (Aug 03 2020 at 14:53, on Zulip):

And I haven't figured out where they are actually emitted yet

nikomatsakis (Aug 03 2020 at 14:53, on Zulip):

but that may have given way to expediency

nikomatsakis (Aug 03 2020 at 14:53, on Zulip):

there are also perhaps a few different phases

Albin Stjerna (Aug 03 2020 at 14:53, on Zulip):

It seems like something like that to me

nikomatsakis (Aug 03 2020 at 14:53, on Zulip):

i.e., I know we report some errors when we create "move paths", like a move of a field of a struct w/ dtor

Albin Stjerna (Aug 03 2020 at 14:54, on Zulip):

hmm

Albin Stjerna (Aug 03 2020 at 14:55, on Zulip):

I wouldn't say that was apparent from what I read

nikomatsakis (Aug 03 2020 at 14:56, on Zulip):

I could be wrong but I'm pretty sure :)

nikomatsakis (Aug 03 2020 at 14:56, on Zulip):

I'm loading up the code now

Albin Stjerna (Aug 03 2020 at 14:58, on Zulip):

It seems to me like the reporting happens somewhere in borrow_check, in the function do_mir_borrowck, possibly only on lines 314 and 357

Albin Stjerna (Aug 03 2020 at 14:59, on Zulip):

But they accumulate earlier than that. I'm having trouble following the "promoted" errors

nikomatsakis (Aug 03 2020 at 15:00, on Zulip):

ok so

nikomatsakis (Aug 03 2020 at 15:00, on Zulip):

on line 287 you have

    if let Err((move_data, move_errors)) = move_data_results {
nikomatsakis (Aug 03 2020 at 15:01, on Zulip):

my memory is that the move_errors here refer to paths that were moved but which are not allowed to be moved

nikomatsakis (Aug 03 2020 at 15:01, on Zulip):

I guess those errors aren't reported yet, just gathered

nikomatsakis (Aug 03 2020 at 15:01, on Zulip):

oh, well, then there is this line (314)

        promoted_mbcx.report_move_errors(move_errors);
Albin Stjerna (Aug 03 2020 at 15:02, on Zulip):

It seems like that, but I also think there is some recomputation done as well, on the promoted_errors as well?

nikomatsakis (Aug 03 2020 at 15:02, on Zulip):

but let me check I mean maybne I'm wrong :)

Albin Stjerna (Aug 03 2020 at 15:02, on Zulip):

The move errors also have categories, it seems

nikomatsakis (Aug 03 2020 at 15:02, on Zulip):

well

nikomatsakis (Aug 03 2020 at 15:02, on Zulip):

we do some consolidation I thnk

nikomatsakis (Aug 03 2020 at 15:03, on Zulip):

ok I see

Albin Stjerna (Aug 03 2020 at 15:03, on Zulip):

It seems the first difference is between moves out of a union or an "illegal move"

nikomatsakis (Aug 03 2020 at 15:03, on Zulip):

so the "promoted" MIR is basically inline constants

Albin Stjerna (Aug 03 2020 at 15:03, on Zulip):

ah

nikomatsakis (Aug 03 2020 at 15:03, on Zulip):

so when we compile a function foo

nikomatsakis (Aug 03 2020 at 15:03, on Zulip):

we actually create a little tree of foo plus any inline constants

nikomatsakis (Aug 03 2020 at 15:04, on Zulip):

these are called "promoted" because the idea is that we "promote" some expressions from runtime to compilation time

nikomatsakis (Aug 03 2020 at 15:04, on Zulip):

e.g., let x = 22 + 44 the 22+44 would be promoted

nikomatsakis (Aug 03 2020 at 15:04, on Zulip):

you can see this because let x: &'static u32 = &(22 + 44) is legal

Albin Stjerna (Aug 03 2020 at 15:04, on Zulip):

Ah, I see

nikomatsakis (Aug 03 2020 at 15:04, on Zulip):

anyway for each of those promoted expressions we have a little "attached MIR" that gets borrow checked at the same time

Albin Stjerna (Aug 03 2020 at 15:05, on Zulip):

oh ok, and we have to borrow-check them separately

nikomatsakis (Aug 03 2020 at 15:07, on Zulip):

yeah, they are basically distinct functions

nikomatsakis (Aug 03 2020 at 15:08, on Zulip):

anyway it looks like report_move_errors is only reporting illegal moves

nikomatsakis (Aug 03 2020 at 15:08, on Zulip):

do you know if we are even detecting those presently in polonius?

nikomatsakis (Aug 03 2020 at 15:09, on Zulip):

I think the other errors get reported during this code:

    dataflow::visit_results(
        &body,
        traversal::reverse_postorder(&body).map(|(bb, _)| bb),
        &results,
        &mut mbcx,
    );
Albin Stjerna (Aug 03 2020 at 15:09, on Zulip):

Aah, I see

nikomatsakis (Aug 03 2020 at 15:10, on Zulip):

that code basically walks over the control-flow graph and triggers callbacks for each statement etc

nikomatsakis (Aug 03 2020 at 15:10, on Zulip):

at that point, the various dataflow sets are updated with the location being visited

nikomatsakis (Aug 03 2020 at 15:10, on Zulip):

on line 573 you can see

impl<'cx, 'tcx> dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtxt<'cx, 'tcx> {
nikomatsakis (Aug 03 2020 at 15:10, on Zulip):

which defines those callbacks

nikomatsakis (Aug 03 2020 at 15:10, on Zulip):

for example

nikomatsakis (Aug 03 2020 at 15:10, on Zulip):
    fn visit_statement_before_primary_effect(
        &mut self,
        flow_state: &Flows<'cx, 'tcx>,
        stmt: &'cx Statement<'tcx>,
        location: Location,
    ) {
        debug!("MirBorrowckCtxt::process_statement({:?}, {:?}): {:?}", location, stmt, flow_state);
        let span = stmt.source_info.span;

        self.check_activations(location, span, flow_state);

        match &stmt.kind {
            StatementKind::Assign(box (lhs, ref rhs)) => {
                self.consume_rvalue(location, (rhs, span), flow_state);

                self.mutate_place(location, (*lhs, span), Shallow(None), JustWrite, flow_state);
            }
Albin Stjerna (Aug 03 2020 at 15:11, on Zulip):

nikomatsakis said:

anyway it looks like report_move_errors is only reporting illegal moves

No, I don't think we do, at least not unless they trigger some other kind of error

nikomatsakis (Aug 03 2020 at 15:11, on Zulip):

this is invoked on the statement -- the name "before primary effect" means that the flow_state contains the values before the statement takes eeffect

nikomatsakis (Aug 03 2020 at 15:11, on Zulip):

I also don't think we do, we should probably add this to some list of "things to think about"

nikomatsakis (Aug 03 2020 at 15:11, on Zulip):

anyway, if you click consume_rvalue (say)

nikomatsakis (Aug 03 2020 at 15:12, on Zulip):
    fn consume_rvalue(
        &mut self,
        location: Location,
        (rvalue, span): (&'cx Rvalue<'tcx>, Span),
        flow_state: &Flows<'cx, 'tcx>,
    ) {

on line 1249

nikomatsakis (Aug 03 2020 at 15:12, on Zulip):

it will take you to access_place

nikomatsakis (Aug 03 2020 at 15:13, on Zulip):

the comment claims this checks the initialization state

nikomatsakis (Aug 03 2020 at 15:13, on Zulip):

but I haven't found that code yet :)

nikomatsakis (Aug 03 2020 at 15:13, on Zulip):

ah well

Albin Stjerna (Aug 03 2020 at 15:13, on Zulip):

Well I'm using emacs so there's no clicking

nikomatsakis (Aug 03 2020 at 15:13, on Zulip):

rust-analyzer works with ermacs ;)

nikomatsakis (Aug 03 2020 at 15:13, on Zulip):

but ok

Albin Stjerna (Aug 03 2020 at 15:13, on Zulip):

But I guess I really should have something with a bit better code navigation

Albin Stjerna (Aug 03 2020 at 15:13, on Zulip):

Just for...code navigation

nikomatsakis (Aug 03 2020 at 15:14, on Zulip):

(I've converted to vscode with rust-analyzer, it's pretty nice)

nikomatsakis (Aug 03 2020 at 15:14, on Zulip):

anyway, check_access_permissions does do at least some checking

nikomatsakis (Aug 03 2020 at 15:14, on Zulip):

i.e., it reports an error if you assign to a non-mutable local variable and that variable has ever been assigned before

Albin Stjerna (Aug 03 2020 at 15:15, on Zulip):

I'm seriously considering something like that

nikomatsakis (Aug 03 2020 at 15:15, on Zulip):

I guess that's probably something we left out of the rules too

Albin Stjerna (Aug 03 2020 at 15:15, on Zulip):

haha surprise

Albin Stjerna (Aug 03 2020 at 15:16, on Zulip):

I guess we must have, since we don't have any notion of mutability

nikomatsakis (Aug 03 2020 at 15:18, on Zulip):

check_if_full_path_is_moved

nikomatsakis (Aug 03 2020 at 15:18, on Zulip):

invoked from check_if_path_or_subpath_is_moved

nikomatsakis (Aug 03 2020 at 15:18, on Zulip):

invoked from consume_rvalue

nikomatsakis (Aug 03 2020 at 15:18, on Zulip):

I thik that the comments on access_place are just a bit wrong

nikomatsakis (Aug 03 2020 at 15:18, on Zulip):

anyway, that seems to be the code that actually checks that each place expression (path) that we access is initialized

nikomatsakis (Aug 03 2020 at 15:19, on Zulip):

if it finds that there is an access to a location that was moved, it invokes

nikomatsakis (Aug 03 2020 at 15:19, on Zulip):
            self.report_use_of_moved_or_uninitialized(
                location,
                desired_action,
                (prefix, place_span.0, place_span.1),
                mpi,
            );
nikomatsakis (Aug 03 2020 at 15:19, on Zulip):

so that's probably roughly speaking the code we want to try and hook into

Albin Stjerna (Aug 03 2020 at 15:21, on Zulip):

...this would have taken me days

nikomatsakis (Aug 03 2020 at 15:26, on Zulip):

I guess the question is how difficult it will be :)

nikomatsakis (Aug 03 2020 at 15:26, on Zulip):

I have to go look at those arguments

nikomatsakis (Aug 03 2020 at 15:26, on Zulip):
    pub(in crate::borrow_check) fn report_use_of_moved_or_uninitialized(
        &mut self,
        location: Location,
        desired_action: InitializationRequiringAction,
        (moved_place, used_place, span): (PlaceRef<'tcx>, PlaceRef<'tcx>, Span),
        mpi: MovePathIndex,
    ) {
nikomatsakis (Aug 03 2020 at 15:26, on Zulip):

so the desired_action ...

#[derive(Copy, Clone, Debug)]
enum InitializationRequiringAction {
    Update,
    Borrow,
    MatchOn,
    Use,
    Assignment,
    PartialAssignment,
}
nikomatsakis (Aug 03 2020 at 15:27, on Zulip):

well, let's step back a second, what does a polonius error look like

nikomatsakis (Aug 03 2020 at 15:27, on Zulip):
move_error(Path, TargetNode) :-
    path_maybe_uninitialized_on_exit(Path, SourceNode),
    cfg_edge(SourceNode, TargetNode),
    path_accessed_at(Path, TargetNode).
nikomatsakis (Aug 03 2020 at 15:28, on Zulip):

heh ok so we're missing some info to give truly good errors

nikomatsakis (Aug 03 2020 at 15:28, on Zulip):

but we can fake it a bit to start

nikomatsakis (Aug 03 2020 at 15:28, on Zulip):

we don't know the initialization requiring action

nikomatsakis (Aug 03 2020 at 15:28, on Zulip):

we also don't know the moved place

nikomatsakis (Aug 03 2020 at 15:29, on Zulip):

side note that we should probably change from Path in our nomenclature to Place

nikomatsakis (Aug 03 2020 at 15:29, on Zulip):

even though I prefer Path, I think I've lost that argument and I don't care to keep arguing about it :)

nikomatsakis (Aug 03 2020 at 15:31, on Zulip):

anyway, the "moved place" is, well, the one that was moved

nikomatsakis (Aug 03 2020 at 15:31, on Zulip):

and the "used place" is the one that was used

nikomatsakis (Aug 03 2020 at 15:31, on Zulip):

but in the polonius rules as we wrote them, we kind of eagerly "expand" the set of moved places, right?

nikomatsakis (Aug 03 2020 at 15:31, on Zulip):

i.e, if you move a at location P, we also consider that a move of a.b and a.c

nikomatsakis (Aug 03 2020 at 15:31, on Zulip):

so that by the time we get to the access, we don't know the original moved path

nikomatsakis (Aug 03 2020 at 15:31, on Zulip):

this may also be a source of inefficiency

nikomatsakis (Aug 03 2020 at 15:32, on Zulip):

I'm not sure, but reporting inaccurate information may lead to ICEs during error reporting

nikomatsakis (Aug 03 2020 at 15:32, on Zulip):

worth a try though :)

nikomatsakis (Aug 03 2020 at 15:32, on Zulip):

i.e., we could give a dummy use and use the same path for use/move place

nikomatsakis (Aug 03 2020 at 15:32, on Zulip):

the mpi we ought to be able to just lookup

nikomatsakis (Aug 03 2020 at 15:32, on Zulip):

does that all make sense?

nikomatsakis (Aug 03 2020 at 15:33, on Zulip):

I guess the question then is whether we want to re-formulate the rules to track more information -- for example, we could include the InitializationRequiringAction in the path_accessed_at fact readily enough

nikomatsakis (Aug 03 2020 at 15:34, on Zulip):

the harder bit is the move-path vs used-path

nikomatsakis (Aug 03 2020 at 15:34, on Zulip):

this may be related to the errors around partial initialization, actually

nikomatsakis (Aug 03 2020 at 15:35, on Zulip):

because I think part of what makes it hard is stuff like

Albin Stjerna (Aug 03 2020 at 15:35, on Zulip):

nikomatsakis said:

does that all make sense?

Yes!

nikomatsakis (Aug 03 2020 at 15:35, on Zulip):

the current Polonius code will handle this correctly, but if you just stored the actual moves/initializations, that'd be confusing, because initializing the path p.q kind of "undoes" the effect of moving p but only partially

Albin Stjerna (Aug 03 2020 at 15:36, on Zulip):

ah

Albin Stjerna (Aug 03 2020 at 15:37, on Zulip):

nikomatsakis said:

I guess the question then is whether we want to re-formulate the rules to track more information -- for example, we could include the InitializationRequiringAction in the path_accessed_at fact readily enough

I'm not sure I follow this

Albin Stjerna (Aug 03 2020 at 15:37, on Zulip):

What is InitializationRequiringAction? A "tag"? Also what does it actually mean?

nikomatsakis (Aug 03 2020 at 15:39, on Zulip):

it is used for error reporting

nikomatsakis (Aug 03 2020 at 15:39, on Zulip):

it just describes the kind of thing that the code was doing

nikomatsakis (Aug 03 2020 at 15:39, on Zulip):

so that we can report it to the user

nikomatsakis (Aug 03 2020 at 15:39, on Zulip):

i.e., match foo.bar and drop(foo.bar) wind up getting reported slightly differently

nikomatsakis (Aug 03 2020 at 15:39, on Zulip):

but both access foo.bar

nikomatsakis (Aug 03 2020 at 15:39, on Zulip):

something like that

Albin Stjerna (Aug 03 2020 at 15:44, on Zulip):

Ah, ok I see, so we would need to sort of keep that around as a label

nikomatsakis (Aug 03 2020 at 15:44, on Zulip):

yeah

nikomatsakis (Aug 03 2020 at 15:44, on Zulip):

one other thing is that I imagine we're going to get a lot of duplicate errors

nikomatsakis (Aug 03 2020 at 15:44, on Zulip):

with the current propagation setup the way it is

nikomatsakis (Aug 03 2020 at 15:45, on Zulip):

e.g., if you have something like this:

nikomatsakis (Aug 03 2020 at 15:45, on Zulip):
p = initialize();
&p.x;
&p.y;
drop(p);
drop(p);
nikomatsakis (Aug 03 2020 at 15:45, on Zulip):

I think that we would create paths for p.x and p.y

nikomatsakis (Aug 03 2020 at 15:45, on Zulip):

and when we initialize p we will propagate forward p.x and p.y

nikomatsakis (Aug 03 2020 at 15:46, on Zulip):

but then we will expand drop(p) to an access of p, p.x and p.y

nikomatsakis (Aug 03 2020 at 15:46, on Zulip):

and the second drop will report three errors

nikomatsakis (Aug 03 2020 at 15:46, on Zulip):

so maybe it really is worth thinking about reformulating the rules in a less... eager to expand sort of way

nikomatsakis (Aug 03 2020 at 15:46, on Zulip):

this would probably also do wonders for efficiency

Albin Stjerna (Aug 03 2020 at 15:48, on Zulip):

Hm, yes, I guess so

Albin Stjerna (Aug 03 2020 at 15:49, on Zulip):

It would be interesting to see what that does for performance and the OOM problems

Albin Stjerna (Aug 03 2020 at 15:49, on Zulip):

Probably not much, but one can hope

nikomatsakis (Aug 03 2020 at 15:49, on Zulip):

it'd be a lot fewer facts to propagate

nikomatsakis (Aug 03 2020 at 15:50, on Zulip):

I think though an interesting question is what we would do about partial re-initialization

nikomatsakis (Aug 03 2020 at 15:50, on Zulip):

I remember when we made that an error in the NLL checker, we were basically going for consistency, but we thought that probably we ultimately wanted to support it

nikomatsakis (Aug 03 2020 at 15:50, on Zulip):

Still, I think it's ok if we take advantage of that in polonius for now :P

nikomatsakis (Aug 03 2020 at 15:51, on Zulip):

I would think it s probably not enough to completely "change the game" when it comes to perf, that we would also want to consider adopting a basic-block-based analysis

nikomatsakis (Aug 03 2020 at 15:51, on Zulip):

(Though one could certainly express that in datalog, whether one should is another question)

nikomatsakis (Aug 03 2020 at 15:52, on Zulip):

(When it comes to perf, another interesting question that I don't know the answer to: polonius uses tuples, which are optimized for "sparse" computations. rustc uses bitsets, which are more efficient, but you pay the worst-case cost all the time. Actually I can't remember, rustc may be using sparse bitsets as a kind of hybrid)

Albin Stjerna (Aug 03 2020 at 15:54, on Zulip):

Hm

nikomatsakis (Aug 03 2020 at 15:54, on Zulip):

reading the rules it doesn't seem like it'd be that hard to convert

Albin Stjerna (Aug 03 2020 at 15:54, on Zulip):

I think maybe we would need to change the rules to run initialisation "backwards"

Albin Stjerna (Aug 03 2020 at 15:55, on Zulip):

I.e. let the access propagate transitively down the move path and hit the move, rather than the other way around?

nikomatsakis (Aug 03 2020 at 15:55, on Zulip):

so, if you assume that "partially initialization" a moved path is illegal

nikomatsakis (Aug 03 2020 at 15:55, on Zulip):

i.e., if I move a path P, I cannot then initialize some child path of P, I must initialize P itself

nikomatsakis (Aug 03 2020 at 15:56, on Zulip):

(or an ancestor of P, perhaps)

nikomatsakis (Aug 03 2020 at 15:56, on Zulip):

these rules:

nikomatsakis (Aug 03 2020 at 15:56, on Zulip):
path_maybe_uninitialized_on_exit(Path, Node) :-
    path_moved_at(Path, Node).

path_maybe_uninitialized_on_exit(Path, TargetNode) :-
    path_maybe_uninitialized_on_exit(Path, SourceNode),
    cfg_edge(SourceNode, TargetNode),
    !path_assigned_at(Path, TargetNode).
nikomatsakis (Aug 03 2020 at 15:56, on Zulip):

if you change path_moved_at to path_moved_at_base

nikomatsakis (Aug 03 2020 at 15:57, on Zulip):

I think they "just work"

nikomatsakis (Aug 03 2020 at 15:57, on Zulip):

it gets harder if you permit partial initialization, though not impossible, you'd have to add in some extra facts

nikomatsakis (Aug 03 2020 at 15:57, on Zulip):

at that point, the path_maybe_unitialized_on_exit is a bit of a misnomer though

nikomatsakis (Aug 03 2020 at 15:57, on Zulip):

it's kind of path_maybe_moved_on_exit

nikomatsakis (Aug 03 2020 at 15:58, on Zulip):

and then you need to modify the move_error rule to be something like

nikomatsakis (Aug 03 2020 at 15:58, on Zulip):
move_error(Path, TargetNode) :-
    path_maybe_moved_on_exit(AncestorPath, SourceNode),
    cfg_edge(SourceNode, TargetNode),
    path_accessed_at_base(Path, TargetNode), // <-- changed to base
    path_ancestor(AncestorPath, Path).
nikomatsakis (Aug 03 2020 at 15:59, on Zulip):

maybe path_maybe_directly_moved_on_exit is a better name :P

nikomatsakis (Aug 03 2020 at 15:59, on Zulip):

(you would of course want to remove path_assigned_at entirely, also in the rules above)

nikomatsakis (Aug 03 2020 at 15:59, on Zulip):

the idea would be to only expand the relationships between paths once, independently of any node

Albin Stjerna (Aug 03 2020 at 15:59, on Zulip):

ah, ok I see

nikomatsakis (Aug 03 2020 at 16:00, on Zulip):

we would do a similar thing for path_maybe_initialized_on_exit, I imagine...

nikomatsakis (Aug 03 2020 at 16:00, on Zulip):

is the only role of that to compute var_maybe_partly_initialized_on_exit?

nikomatsakis (Aug 03 2020 at 16:00, on Zulip):

it appears so

nikomatsakis (Aug 03 2020 at 16:01, on Zulip):

because surely we could do that more efficiently too... :)

nikomatsakis (Aug 03 2020 at 16:01, on Zulip):

but I guess that's independent, "just" an optimiztion

Albin Stjerna (Aug 03 2020 at 16:02, on Zulip):

Haha

Albin Stjerna (Aug 03 2020 at 16:02, on Zulip):

I wonder if I should start with the rustc connection anyway, despite knowing it will blow up, and then work on the polonius changes

nikomatsakis (Aug 03 2020 at 16:02, on Zulip):

(side note that I remember putting quite a bit of effort into hand-tuning the liveness computation in rustc, and in particular it takes advantage of the fact that "drop live" is a superset of "use live" )

nikomatsakis (Aug 03 2020 at 16:03, on Zulip):

Albin Stjerna said:

I wonder if I should start with the rustc connection anyway, despite knowing it will blow up, and then work on the polonius changes

yeah I was wondering the same

nikomatsakis (Aug 03 2020 at 16:03, on Zulip):

might make sense

Albin Stjerna (Aug 03 2020 at 16:03, on Zulip):

Just because that would be an easier way of running Polonius

nikomatsakis (Aug 03 2020 at 16:03, on Zulip):

it'd be nice to be able to visualize the errors

nikomatsakis (Aug 03 2020 at 16:03, on Zulip):

even if we just do it for individual tests

Albin Stjerna (Aug 03 2020 at 16:03, on Zulip):

Also lets me wait for the cargo bug to be fixed

nikomatsakis (Aug 03 2020 at 16:03, on Zulip):

right

nikomatsakis (Aug 03 2020 at 16:05, on Zulip):

well I think you should be able to mock something up based on what I said above

nikomatsakis (Aug 03 2020 at 16:05, on Zulip):

I'm curious if it will ICE like crazy but worth a try

Albin Stjerna (Aug 03 2020 at 16:16, on Zulip):

Yes!

lqd (Aug 03 2020 at 16:54, on Zulip):

ah Albin in case you haven't seen it, https://github.com/rust-lang/rust/issues/70797 was the issue I mentioned earlier relating to the move errors false positives, we haven't analyzed it yet but it could eventually be an interesting test for your move errors work -- and also the change that could have caused that (if it's a valid problem) was https://github.com/rust-lang/rust/pull/70546/files#diff-b05a06387184e401e73a1e9da3fdff0f where path accesses were the only path fact on the start point (and maybe that could impact your move errors rules)

Albin Stjerna (Aug 03 2020 at 16:59, on Zulip):

Interesting, thanks!

lqd (Aug 03 2020 at 17:01, on Zulip):

at least it's an easy revert back to its original position in case we need to do so :)

nikomatsakis (Aug 03 2020 at 17:17, on Zulip):

oh yeah I forgot about that

nikomatsakis (Aug 03 2020 at 17:17, on Zulip):

would be worth digging into what is going on there

lqd (Aug 03 2020 at 22:00, on Zulip):

btw NLL rejects this case with the same error

lqd (Aug 03 2020 at 22:27, on Zulip):

(and I would think that the error is correct because the temporary has a destructor)

nikomatsakis (Aug 03 2020 at 22:30, on Zulip):

mm that's not clear

nikomatsakis (Aug 03 2020 at 22:31, on Zulip):

I wonder if rewriting to match would make it work

nikomatsakis (Aug 03 2020 at 22:31, on Zulip):

i.e., on the None path, there are no references

nikomatsakis (Aug 03 2020 at 22:31, on Zulip):

and on the Some path, we move out into pp

lqd (Aug 03 2020 at 22:34, on Zulip):

let's see what rewriting with a match does with polonius, as I don't think it would make a difference in NLL

lqd (Aug 03 2020 at 22:36, on Zulip):

same error

lqd (Aug 03 2020 at 22:36, on Zulip):

interesting in any case

lqd (Aug 03 2020 at 22:52, on Zulip):

but I don't think it's the first time we've encountered possible imprecision with temporaries with Drop impls

Albin Stjerna (Aug 04 2020 at 09:42, on Zulip):

nikomatsakis said:

I also don't think we do, we should probably add this to some list of "things to think about"

Update: I have now added this list to the hackMD.

nikomatsakis (Aug 04 2020 at 14:19, on Zulip):

how goes @Albin Stjerna ?

Albin Stjerna (Aug 04 2020 at 14:25, on Zulip):

Forward!

Albin Stjerna (Aug 04 2020 at 14:27, on Zulip):

I have intercepted the old logic and, I think, replaced it with something, and I'm a coffee break away from the Moment of Truth

lqd (Aug 04 2020 at 14:28, on Zulip):

awesome !

Albin Stjerna (Aug 04 2020 at 14:30, on Zulip):

Essentially I just replace the entire check_if_path_or_subpath_is_moved with "unpack Polonius results"

nikomatsakis (Aug 04 2020 at 14:30, on Zulip):

interesting

Albin Stjerna (Aug 04 2020 at 14:30, on Zulip):

That leaves the move errors we don't implement in, I think, and may possibly use the ones we have

Albin Stjerna (Aug 04 2020 at 14:54, on Zulip):

Ok, so it works for all the errors in smoke-test except this one;

error[E0506]: cannot assign to `*x` because it is borrowed
  --> /home/albin/polonius/inputs/smoke-test/polonius-smoke-test.rs:35:9
   |
30 | pub fn position_dependent_outlives(x: &mut i32, cond: bool) -> &mut i32 {
   |                                       - let's call the lifetime of this reference `'1`
31 |     let y = &mut *x;
   |             ------- borrow of `*x` occurs here
32 |     if cond {
33 |         return y;
   |                - returning this value requires that `*x` is borrowed for `'1`
34 |     } else {
35 |         *x = 0;
   |         ^^^^^^ assignment to borrowed `*x` occurs here
Albin Stjerna (Aug 04 2020 at 14:55, on Zulip):

which is what we expect, right?

nikomatsakis (Aug 04 2020 at 15:02, on Zulip):

@Albin Stjerna define "does not work" I guess?

nikomatsakis (Aug 04 2020 at 15:03, on Zulip):

we don't expect an error for that test case

nikomatsakis (Aug 04 2020 at 15:03, on Zulip):

when using polonius, anyway

Albin Stjerna (Aug 04 2020 at 15:05, on Zulip):

Exactly, that's what I meant

Albin Stjerna (Aug 04 2020 at 15:05, on Zulip):

We get the other errors as far as I can tell by eye exactly the same, including the move error

Albin Stjerna (Aug 04 2020 at 15:05, on Zulip):

So we know move error reporting works in at least one case

Albin Stjerna (Aug 04 2020 at 15:05, on Zulip):

We also know it's really using the Polonius output, because I suppressed all other output

Albin Stjerna (Aug 04 2020 at 15:05, on Zulip):

(just an early return)

Albin Stjerna (Aug 04 2020 at 15:06, on Zulip):

I'll run the tests and see what happens

nikomatsakis (Aug 04 2020 at 15:07, on Zulip):

great

Albin Stjerna (Aug 04 2020 at 15:09, on Zulip):

Uppdate: it might have err OOM-crashed, I’m not sure because my computer is unresponsive

nikomatsakis (Aug 04 2020 at 15:11, on Zulip):

heh

nikomatsakis (Aug 04 2020 at 15:11, on Zulip):

uh oh

Albin Stjerna (Aug 04 2020 at 15:16, on Zulip):

It’s printing a lot of ^[[D:s, whatever that means

lqd (Aug 04 2020 at 15:17, on Zulip):

those are rustc's tests ?

Albin Stjerna (Aug 04 2020 at 15:17, on Zulip):

Yes, in compare mode

lqd (Aug 04 2020 at 15:17, on Zulip):

did you delete the 2 OOMing in fact gen ?

Albin Stjerna (Aug 04 2020 at 15:18, on Zulip):

Err no

Albin Stjerna (Aug 04 2020 at 15:18, on Zulip):

Ok I get a lot of failures on the ui tests

lqd (Aug 04 2020 at 15:18, on Zulip):

could be that :)

lqd (Aug 04 2020 at 15:19, on Zulip):

there should be around 6 or 8 failing (with possibly 2 others being slow/OOMing in move errors or somewhere, which I had to kill yesterday)

nikomatsakis (Aug 04 2020 at 15:20, on Zulip):

@Albin Stjerna are you running with --bless ?

Albin Stjerna (Aug 04 2020 at 15:20, on Zulip):

Definitely more than that failing now, perhaps 20 or so at least

Albin Stjerna (Aug 04 2020 at 15:21, on Zulip):

I'm specifically running $ ./x.py test -i --stage 1 --compare-mode polonius src/test/ui

lqd (Aug 04 2020 at 15:21, on Zulip):

the good thing is that that means you hooked something up correctly

lqd (Aug 04 2020 at 15:21, on Zulip):

and from now on you'll know for sure that it works :)

Albin Stjerna (Aug 04 2020 at 15:21, on Zulip):

Haha yes, I did something well enough to break something

Albin Stjerna (Aug 04 2020 at 15:22, on Zulip):

But more to the point, should I add --bless to the arguments?

Albin Stjerna (Aug 04 2020 at 15:23, on Zulip):

Also, will there be a log of the failing diffs somewhere that I can inspect?

lqd (Aug 04 2020 at 15:23, on Zulip):

that's what blessing will do yeah

lqd (Aug 04 2020 at 15:23, on Zulip):

you'll have .stderr files containing the changed output

nikomatsakis (Aug 04 2020 at 15:23, on Zulip):

with --bless you would then use git diff to see what happened

lqd (Aug 04 2020 at 15:24, on Zulip):

that will also unfortunately contain the 6-8 cases I mentioned

lqd (Aug 04 2020 at 15:24, on Zulip):

probably best if you can avoid the OOMing tests though :)

lqd (Aug 04 2020 at 15:25, on Zulip):

they will be annoying regardless of blessing the output

lqd (Aug 04 2020 at 15:38, on Zulip):

modulo one or two being OOM or killed, the failures as of today, with no polonius modifications should be more or less these:

failures:
    [ui (polonius)] ui/closures/closure-expected-type/expect-region-supply-region-2.rs
    [ui (polonius)] ui/closures/closure-expected-type/expect-region-supply-region.rs
    [ui (polonius)] ui/hrtb/hrtb-just-for-static.rs
    [ui (polonius)] ui/hrtb/hrtb-perfect-forwarding.rs
    [ui (polonius)] ui/issues/issue-26217.rs
    [ui (polonius)] ui/issues/issue-74564-if-expr-stack-overflow.rs
    [ui (polonius)] ui/lub-glb/old-lub-glb-hr-noteq2.rs
    [ui (polonius)] ui/regions/region-multiple-lifetime-bounds-on-fns-where-clause.rs
lqd (Aug 04 2020 at 15:39, on Zulip):

so unless there are obvious move errors differences there which I wouldn't expect, don't worry about them when you see them fail

lqd (Aug 04 2020 at 15:41, on Zulip):

(I'd love it if we could double check our existing blessed expectations, especially the ones with differences in the number of errors, eg with closures, or maybe `static which I don't feel we handle or special-case exactly the same way)

Albin Stjerna (Aug 04 2020 at 15:42, on Zulip):

There are absolutely more errors:

failures:
    [ui (polonius)] ui/async-await/multiple-lifetimes/hrtb.rs
    [ui (polonius)] ui/borrowck/borrowck-asm.rs
    [ui (polonius)] ui/borrowck/borrowck-box-sensitivity.rs
    [ui (polonius)] ui/borrowck/borrowck-closures-slice-patterns-ok.rs
    [ui (polonius)] ui/borrowck/borrowck-closures-slice-patterns.rs
    [ui (polonius)] ui/borrowck/borrowck-field-sensitivity-rpass.rs
    [ui (polonius)] ui/borrowck/borrowck-field-sensitivity.rs
    [ui (polonius)] ui/borrowck/borrowck-init-in-fru.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array-match.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array-no-overlap-match.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array-no-overlap.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array-use-match.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array-use-no-overlap-match.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array-use-no-overlap.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array-use.rs
    [ui (polonius)] ui/borrowck/borrowck-move-out-from-array.rs
    [ui (polonius)] ui/borrowck/borrowck-multiple-captures.rs
    [ui (polonius)] ui/borrowck/borrowck-partial-reinit-1.rs
    [ui (polonius)] ui/borrowck/borrowck-partial-reinit-2.rs
    [ui (polonius)] ui/borrowck/borrowck-partial-reinit-3.rs
    [ui (polonius)] ui/borrowck/borrowck-partial-reinit-4.rs
    [ui (polonius)] ui/borrowck/borrowck-uninit-field-access.rs
    [ui (polonius)] ui/borrowck/borrowck-uninit-in-assignop.rs
    [ui (polonius)] ui/borrowck/borrowck-uninit-ref-chain.rs
    [ui (polonius)] ui/borrowck/borrowck-union-move-assign.rs
    [ui (polonius)] ui/borrowck/borrowck-union-move.rs
    [ui (polonius)] ui/borrowck/borrowck-use-in-index-lvalue.rs
    [ui (polonius)] ui/borrowck/borrowck-use-uninitialized-in-cast-trait.rs
    [ui (polonius)] ui/borrowck/borrowck-use-uninitialized-in-cast.rs
    [ui (polonius)] ui/closures/closure-expected-type/expect-region-supply-region-2.rs
    [ui (polonius)] ui/closures/closure-expected-type/expect-region-supply-region.rs
    [ui (polonius)] ui/drop/dynamic-drop-async.rs
    [ui (polonius)] ui/hrtb/hrtb-just-for-static.rs
    [ui (polonius)] ui/hrtb/hrtb-perfect-forwarding.rs
    [ui (polonius)] ui/ifmt.rs
    [ui (polonius)] ui/intrinsics/intrinsics-integer.rs
    [ui (polonius)] ui/issues/issue-17263.rs
    [ui (polonius)] ui/issues/issue-17385.rs
    [ui (polonius)] ui/issues/issue-19340-2.rs
    [ui (polonius)] ui/issues/issue-26217.rs
    [ui (polonius)] ui/issues/issue-2718.rs
    [ui (polonius)] ui/issues/issue-27282-move-match-input-into-guard.rs
    [ui (polonius)] ui/issues/issue-29166.rs
    [ui (polonius)] ui/issues/issue-3609.rs
    [ui (polonius)] ui/issues/issue-42679.rs
    [ui (polonius)] ui/issues/issue-74564-if-expr-stack-overflow.rs
    [ui (polonius)] ui/liveness/liveness-use-after-move.rs
    [ui (polonius)] ui/lub-glb/old-lub-glb-hr-noteq2.rs
    [ui (polonius)] ui/moves/moves-sru-moved-field.rs
    [ui (polonius)] ui/mpsc_stress.rs
    [ui (polonius)] ui/nll/issue-21232-partial-init-and-erroneous-use.rs
    [ui (polonius)] ui/nll/issue-48623-closure.rs
    [ui (polonius)] ui/nll/issue-48623-generator.rs
    [ui (polonius)] ui/nll/issue-51512.rs
    [ui (polonius)] ui/nll/user-annotations/patterns.rs
    [ui (polonius)] ui/numbers-arithmetic/saturating-float-casts.rs
    [ui (polonius)] ui/pattern/move-ref-patterns/borrowck-move-ref-pattern.rs
    [ui (polonius)] ui/pattern/move-ref-patterns/feature-gate-move_ref_pattern.rs
    [ui (polonius)] ui/pattern/move-ref-patterns/issue-53840.rs
    [ui (polonius)] ui/pattern/move-ref-patterns/move-ref-patterns-closure-captures-inside.rs
    [ui (polonius)] ui/pattern/move-ref-patterns/move-ref-patterns-closure-captures-pass.rs
    [ui (polonius)] ui/pattern/move-ref-patterns/move-ref-patterns-dynamic-semantics.rs
    [ui (polonius)] ui/regions/region-multiple-lifetime-bounds-on-fns-where-clause.rs
    [ui (polonius)] ui/resource-assign-is-not-copy.rs
    [ui (polonius)] ui/self/elision/alias-async.rs
    [ui (polonius)] ui/self/elision/assoc-async.rs
    [ui (polonius)] ui/self/elision/lt-alias-async.rs
    [ui (polonius)] ui/self/elision/lt-assoc-async.rs
    [ui (polonius)] ui/self/elision/lt-ref-self-async.rs
    [ui (polonius)] ui/self/elision/lt-self-async.rs
    [ui (polonius)] ui/self/elision/lt-struct-async.rs
    [ui (polonius)] ui/self/elision/multiple-ref-self-async.rs
    [ui (polonius)] ui/self/elision/ref-alias-async.rs
    [ui (polonius)] ui/self/elision/ref-assoc-async.rs
    [ui (polonius)] ui/self/elision/ref-mut-alias-async.rs
    [ui (polonius)] ui/self/elision/ref-mut-self-async.rs
    [ui (polonius)] ui/self/elision/ref-mut-struct-async.rs
    [ui (polonius)] ui/self/elision/ref-self-async.rs
    [ui (polonius)] ui/self/elision/ref-struct-async.rs
    [ui (polonius)] ui/self/elision/self-async.rs
    [ui (polonius)] ui/self/elision/struct-async.rs
    [ui (polonius)] ui/threads-sendsync/send_str_treemap.rs
    [ui (polonius)] ui/traits/trait-bounds-in-arc.rs
    [ui (polonius)] ui/type-alias-impl-trait/type-alias-impl-trait-tuple.rs
    [ui (polonius)] ui/unboxed-closures/unboxed-closures-move-some-upvars-in-by-ref-closure.rs
    [ui (polonius)] ui/union/union-borrow-move-parent-sibling.rs
    [ui (polonius)] ui/use/use-after-move-self-based-on-type.rs
    [ui (polonius)] ui/use/use-after-move-self.rs
    [ui (polonius)] ui/walk-struct-literal-with.rs
    [ui (polonius)] ui/wrapping-int-combinations.rs

test result: FAILED. 10380 passed; 90 failed; 72 ignored; 0 measured; 0 filtered out
Albin Stjerna (Aug 04 2020 at 15:42, on Zulip):

I'll try again with --bless and see what the differences are

lqd (Aug 04 2020 at 15:42, on Zulip):

ui/numbers-arithmetic/saturating-float-casts.rs and ui/wrapping-int-combinations.rs are the 2 known OOMs

lqd (Aug 04 2020 at 15:45, on Zulip):

the ones in ui/borrowck/ about moves and partial reinits should be interesting

Albin Stjerna (Aug 04 2020 at 15:47, on Zulip):

It seems we get some spurious errors I assume to be the same

lqd (Aug 04 2020 at 15:49, on Zulip):

how so ?

Albin Stjerna (Aug 04 2020 at 15:50, on Zulip):
error[E0382]: use of moved value: `f`
  --> /home/albin/rust/src/test/ui/self/elision/lt-alias-async.rs:13:30
   |
LL |     async fn take_self(self, f: &u32) -> &u32 {
   |                        ----  ^ value used here after move
   |                        |
   |                        value moved here
   |
   = note: move occurs because `self` has type `Struct<'_>`, which does not implement the `Copy` trait
Albin Stjerna (Aug 04 2020 at 15:51, on Zulip):

This...makes no sense, the code looks like this:

    async fn take_self(self, f: &u32) -> &u32 {
        f
    }
Albin Stjerna (Aug 04 2020 at 15:51, on Zulip):

self and f aren't even related in any way

lqd (Aug 04 2020 at 15:56, on Zulip):

I do wonder whether the way async fns are desugared can be special wrt to self ...

Albin Stjerna (Aug 04 2020 at 16:14, on Zulip):

I'll investigate more later. I'm still chalking this up as a partial success!

lqd (Aug 04 2020 at 16:15, on Zulip):

it definitely is a success !

lqd (Aug 04 2020 at 16:16, on Zulip):

great job already :)

Albin Stjerna (Aug 04 2020 at 16:18, on Zulip):

Thanks!

nikomatsakis (Aug 04 2020 at 16:26, on Zulip):

that's great -- those errors do look like nonsense but I imagine it has something to do with how we're faking the data

nikomatsakis (Aug 04 2020 at 16:27, on Zulip):

push the branch somewhere, @Albin Stjerna ?

Albin Stjerna (Aug 04 2020 at 18:01, on Zulip):

nikomatsakis said:

push the branch somewhere, Albin Stjerna ?

Sorry, I was making dinner. It's already at my fork, https://github.com/albins/rust

Albin Stjerna (Aug 04 2020 at 18:02, on Zulip):

Should I already open a WIP pull request against Rust?

lqd (Aug 04 2020 at 20:17, on Zulip):

niko had asked yesterday if we had a rundown of the move errors computation slowness, and I do now, on clap which takes 100-110s:

nikomatsakis (Aug 04 2020 at 21:39, on Zulip):

I think we can speedup that rule some more, too

lqd (Aug 04 2020 at 21:42, on Zulip):

seems likely

Albin Stjerna (Aug 05 2020 at 07:42, on Zulip):

I'll spend today working on diagnosing the errors in the ui tests I saw yesterday, trying to figure out what goes wrong

lqd (Aug 05 2020 at 07:45, on Zulip):

(maybe also do a quick location flipping of the access fact just in case ;)

lqd (Aug 05 2020 at 09:12, on Zulip):

beautiful v2.0 :D tracy_profile.png

nikomatsakis (Aug 05 2020 at 09:28, on Zulip):

@lqd wait, what is that?

nikomatsakis (Aug 05 2020 at 09:28, on Zulip):

looks slick

lqd (Aug 05 2020 at 09:29, on Zulip):

it's https://github.com/wolfpld/tracy :)

lqd (Aug 05 2020 at 09:30, on Zulip):

it's very slick indeed, better on windows rn as it mixes manual instrumentation and sampling

nikomatsakis (Aug 05 2020 at 09:31, on Zulip):

nice...

nikomatsakis (Aug 05 2020 at 09:31, on Zulip):

maybe for once I might have a reason to develop on windows

lqd (Aug 05 2020 at 09:32, on Zulip):

on linux it "just" does the manual instrumentation

lqd (Aug 05 2020 at 09:32, on Zulip):

so it's -Zself-profile like, with aggregation, querying, graphical views, comparisons between profile runs (!) etc etc

lqd (Aug 05 2020 at 09:33, on Zulip):

of course it works within rustc as well

lqd (Aug 05 2020 at 09:33, on Zulip):

(I've posted some more screenshots of rustc integration before here where I just mimicked the -Zself-profile events, and an example of the sampling on polonius in this other screenshot)

lqd (Aug 05 2020 at 09:39, on Zulip):

(there also was the recent https://github.com/bombomby/optick-rs but this one's GUI only works on windows (data collection is system-independent), while Tracy's works everywhere)

Albin Stjerna (Aug 05 2020 at 09:46, on Zulip):

lqd said:

(maybe also do a quick location flipping of the access fact just in case ;)

Remind me, was it that we moved the access event from start-point to mid-point?

lqd (Aug 05 2020 at 09:56, on Zulip):

it caused move errors false positives on the datasets inside the repo

lqd (Aug 05 2020 at 09:57, on Zulip):

some things were initialized or uninit and across blocks the event and the move error were at the same point

lqd (Aug 05 2020 at 09:57, on Zulip):

my investigation into that was https://rust-lang.zulipchat.com/#narrow/stream/186049-t-compiler.2Fwg-polonius/topic/move.20errors/near/191031867

lqd (Aug 05 2020 at 09:58, on Zulip):

it seemed to make sense, but at the same time likely caused https://github.com/rust-lang/rust/issues/70797 which we still don't exactly know for sure what's going on there

Albin Stjerna (Aug 05 2020 at 14:34, on Zulip):

lqd said:

(maybe also do a quick location flipping of the access fact just in case ;)

Doing this now :)

lqd (Aug 05 2020 at 14:35, on Zulip):

maybe read the linked thread, you didn't remember why it was the only fact at this kind of point, but of course there could ahve been a reason, maybe discussing with matthew or niko

lqd (Aug 05 2020 at 14:36, on Zulip):

but the case of initializing a local from a call was the issue

lqd (Aug 05 2020 at 14:36, on Zulip):

as both init and move error were checking the same point

lqd (Aug 05 2020 at 14:36, on Zulip):

it was an interesting case :)

lqd (Aug 05 2020 at 14:37, on Zulip):

at least the repo contains enough different datasets to test move errors a bit, see if there are false positives or not, and so on

Albin Stjerna (Aug 05 2020 at 14:38, on Zulip):

Yes, but I remember spending a lot of time thinking about that. It might have been a mistake though; I'll compare it to what I wrote in my thesis

lqd (Aug 05 2020 at 14:38, on Zulip):

but yeah, maybe we do need to flip it back where it used to be :) (the new issue with temporaries + drop is also interesting)

Albin Stjerna (Aug 05 2020 at 14:38, on Zulip):

I mean, just because I thought hard about it doesn't mean I didn't butter-finger that particular line in Rust

lqd (Aug 05 2020 at 14:39, on Zulip):

yeah it could be a simple typo

Albin Stjerna (Aug 05 2020 at 14:39, on Zulip):

Also, specifically open/closed intervals is something I frequently struggle with, so I might have just messed it up

Albin Stjerna (Aug 05 2020 at 14:40, on Zulip):

We'll know when the tests finish I guess

lqd (Aug 05 2020 at 14:40, on Zulip):

I myself don't remember if you had talked about this specific case of initializing with calls with @nikomatsakis when you were coming up with the rules

lqd (Aug 05 2020 at 14:41, on Zulip):

as it seemed move errors really did detect errors along an edge, while this seemed to be at a specific point ?

lqd (Aug 05 2020 at 14:43, on Zulip):

as this could have been fixed two ways:

Albin Stjerna (Aug 05 2020 at 14:45, on Zulip):

I have some related lab notes (so happy I kept those), but that seems to be mainly about emitting initialisation facts

Albin Stjerna (Aug 05 2020 at 14:46, on Zulip):

For function arguments, that is

nikomatsakis (Aug 05 2020 at 15:23, on Zulip):

are we discussing the "false positive" case?

lqd (Aug 05 2020 at 15:26, on Zulip):

its cause yeah

lqd (Aug 05 2020 at 15:26, on Zulip):

the false positive fix which ended up creating that other issue

lqd (Aug 05 2020 at 15:27, on Zulip):

in case it also impacts the move errors Albin is seeing now on rustc tests

Albin Stjerna (Aug 05 2020 at 15:31, on Zulip):

It seems to, because I don't think I see nearly as many failures when I switched back mid/start points for fact emission, but for whatever reason the test script has been stuck on if-expr-stack-overflow for a long time now

Albin Stjerna (Aug 05 2020 at 15:31, on Zulip):

I'm considering killing it

Albin Stjerna (Aug 05 2020 at 15:32, on Zulip):

Ok, that seems to have worked in the sense that the test terminated. I now have...different errors, so there is clearly an interaction going on

Albin Stjerna (Aug 05 2020 at 15:34, on Zulip):

Ok that didn't solve it anyway, so I'd have to investigate this further

nikomatsakis (Aug 05 2020 at 15:35, on Zulip):

if I wanted to examine the failure,

nikomatsakis (Aug 05 2020 at 15:35, on Zulip):

how would I do so :)

nikomatsakis (Aug 05 2020 at 15:35, on Zulip):

should I use @Albin Stjerna's branch?

nikomatsakis (Aug 05 2020 at 15:35, on Zulip):

(is it published somewhere..?)

nikomatsakis (Aug 05 2020 at 15:35, on Zulip):

also, remind me which test it is etc

Albin Stjerna (Aug 05 2020 at 15:35, on Zulip):

Yes, you can more or less check out my branch and run one of the failing tests

lqd (Aug 05 2020 at 15:36, on Zulip):

what test are you trying btw ?

Albin Stjerna (Aug 05 2020 at 15:36, on Zulip):

branch is https://github.com/albins/rust

Albin Stjerna (Aug 05 2020 at 15:37, on Zulip):

lqd said:

what test are you trying btw ?

All of them, I guess? I'm running ./x.py test -i --stage 1 --compare-mode polonius src/test/ui --bless

Albin Stjerna (Aug 05 2020 at 15:38, on Zulip):

One example of a failing test is async-await/multiple-lifetimes/hrtb.rs

Albin Stjerna (Aug 05 2020 at 15:39, on Zulip):

(that one specifically works fine if I reverse @lqd's changes and emit path_accessed_at_base at the start-point rather than mid-point)

Albin Stjerna (Aug 05 2020 at 15:42, on Zulip):

I haven't verified this, but I see very little overlap in which files error

Albin Stjerna (Aug 05 2020 at 15:42, on Zulip):

Which sort of gives me the feeling that the move errors are "almost correct"

lqd (Aug 06 2020 at 13:07, on Zulip):

ah actually I should mention this

lqd (Aug 06 2020 at 13:11, on Zulip):

the false positive fix didn't just remove the unexpected errors in the polonius repo on code that should work, it also made polonius match NLLs again on 2 of the rustc tests related to move/init analysis we talked about at the time: maybe-initialized-drop-implicit-fragment-drops
and maybe-initialized-drop-with-uninitialized-fragments

lqd (Aug 06 2020 at 13:14, on Zulip):

my understanding is that they started to compile because of the changes in fact generation done to support move errors; we ultimately want them to compile; and they can be close to the "spurious issue"

lqd (Aug 06 2020 at 18:48, on Zulip):

alright this rustc test will be interesting to try with the new move errors rules implementation, as the current one either doesn't complete or OOM I'm not sure (both var_maybe_partly_initialized_on_exit and move_error relations exhibit this issue)

Last update: Jun 20 2021 at 01:15UTC