Stream: t-compiler/wg-polonius

Topic: trait FactTypes


csmoe (Sep 25 2019 at 13:55, on Zulip):

@nikomatsakis https://github.com/rust-lang/polonius/pull/130
those ugly clone came from move/expect reference errors, I'm trying to resolve.
pasted image

Albin Stjerna (Sep 25 2019 at 14:02, on Zulip):

If you can't find a way to work around that, maybe this is a good time to do the refactoring thing where we split the facts into parts for the different stages of the computation

Albin Stjerna (Sep 25 2019 at 14:03, on Zulip):

As I recall all parts of the computation use their own inputs, except for the CFG, which is used by all of them I think

csmoe (Sep 25 2019 at 14:13, on Zulip):

@Albin Stjerna is this refactoring?

Albin Stjerna (Sep 25 2019 at 14:13, on Zulip):

Yes, exactly that one!

Albin Stjerna (Sep 25 2019 at 14:13, on Zulip):

I had intended to do it myself, but if you feel up for it go ahead!

Albin Stjerna (Sep 25 2019 at 14:13, on Zulip):

But maybe check with err someone who isn't an overwintered master's student first :)

csmoe (Oct 02 2019 at 13:52, on Zulip):

@nikomatsakis request for a review, "clones" cleared.

nikomatsakis (Oct 03 2019 at 08:41, on Zulip):

ok so I just merged this, but then I realized it conflicts something bad with @lqd's renaming PR polonius#127

nikomatsakis (Oct 03 2019 at 08:41, on Zulip):

which I was also about to go merge

nikomatsakis (Oct 03 2019 at 08:42, on Zulip):

@lqd do you think you could squash/rebase that PR? (I probably should've merged it first, since it is older :doh: )

lqd (Oct 03 2019 at 08:42, on Zulip):

:doh:

lqd (Oct 03 2019 at 08:43, on Zulip):

sure, I planned to squash after making sure you liked the result

lqd (Oct 03 2019 at 08:43, on Zulip):

esp wrt lower case naming in the rules comments

nikomatsakis (Oct 03 2019 at 08:50, on Zulip):

I think I'm :+1: on that

nikomatsakis (Oct 03 2019 at 08:50, on Zulip):

closer matching between comments and code seems good

lqd (Oct 03 2019 at 08:51, on Zulip):

alright I'll do it at lunch break

nikomatsakis (Oct 03 2019 at 09:26, on Zulip):

@lqd <3

lqd (Oct 03 2019 at 11:14, on Zulip):

omg the conflicts :tears:

lqd (Oct 03 2019 at 11:27, on Zulip):

oh wow

lqd (Oct 03 2019 at 12:08, on Zulip):

maybe, and I can't stress this enough, rebasing and then squasing was not the smartest of moves

lqd (Oct 03 2019 at 12:08, on Zulip):

done https://github.com/rust-lang/polonius/pull/127

lqd (Oct 12 2019 at 21:21, on Zulip):

After a week of working with it, I'm not so sure about this change anymore, it seems we've traded complexity in defining function/type signatures (with 1 generic type parameter instead of 5) in 12 different places, for T::noise in 200+ different places

Albin Stjerna (Oct 14 2019 at 12:18, on Zulip):

Is this not merged into rustc yet?

Albin Stjerna (Oct 14 2019 at 12:18, on Zulip):

I just rebased off of Polonius master and apparently it doesn't compile with current rustc master

lqd (Oct 14 2019 at 12:19, on Zulip):

the change is only on master here yes, rustc wasn't updated to this new API

Albin Stjerna (Oct 14 2019 at 12:19, on Zulip):

Ah, ok

lqd (Oct 14 2019 at 12:20, on Zulip):

I'm lukewarm on doing so but we can talk about it at the next meeting, maybe you'll have an opinion on it as well

Albin Stjerna (Oct 14 2019 at 12:20, on Zulip):

I don't think I do, but sure

lqd (Oct 14 2019 at 12:20, on Zulip):

I can send you a diff if you want to build it tho

Albin Stjerna (Oct 14 2019 at 12:20, on Zulip):

Yes please,

lqd (Oct 14 2019 at 12:20, on Zulip):

ok just a sec

Albin Stjerna (Oct 14 2019 at 12:21, on Zulip):

It would be faster than me figuring it out myself :)

lqd (Oct 14 2019 at 12:25, on Zulip):

I'm 4-5 days behind, so something like https://gist.github.com/lqd/03862f0f65a051fd60ef8074089b5c05

Albin Stjerna (Oct 14 2019 at 12:38, on Zulip):

Thank you!

Albin Stjerna (Oct 14 2019 at 12:39, on Zulip):

I think I have path-precise initialisation tracking now, but I'd like to test that it works :)

Albin Stjerna (Oct 14 2019 at 12:39, on Zulip):

Also, I think I may have a prototype for move errors literally in a few hours if that works

Albin Stjerna (Oct 14 2019 at 12:39, on Zulip):

But no way of testing it haha

lqd (Oct 14 2019 at 12:41, on Zulip):

Thank you!

you're welcome

lqd (Oct 14 2019 at 12:42, on Zulip):

yeah I'm unsure/don't remember the plan for those sorry, I can't help much

Albin Stjerna (Oct 14 2019 at 12:42, on Zulip):

No problem, I don't think there was even a plan :)

Tshepang Lekhonkhobe (Oct 14 2019 at 22:07, on Zulip):

I'm 4-5 days behind, so something like https://gist.github.com/lqd/03862f0f65a051fd60ef8074089b5c05

why not PR this

Tshepang Lekhonkhobe (Oct 15 2019 at 06:42, on Zulip):

oh, I realize that there is no polonius release with the changes

lqd (Oct 15 2019 at 08:47, on Zulip):

I'd like to talk about this change first, and even so we wouldn't need to do a release just for this change per se

nikomatsakis (Oct 29 2019 at 19:19, on Zulip):

OK, finally caught up here

nikomatsakis (Oct 29 2019 at 19:21, on Zulip):

it seems like the "pro" of this change is

the con is that you have to write T::Fact instead of Fact, primarily?

I'm ok either way I guess. I tend to ultimately move to the "bundle of associated types", but in this case it doesn't really offer that much in particular.

lqd (Oct 29 2019 at 19:24, on Zulip):

maybe we can live with it for a while and after we have enough anecdata after the focus week, we can take a look at this again

nikomatsakis (Oct 29 2019 at 19:25, on Zulip):

I'm looking at your PR polonius#134, @lqd

lqd (Oct 29 2019 at 19:26, on Zulip):

yay

lqd (Oct 29 2019 at 19:27, on Zulip):

it also relates to Albin's initialization PR, errors in general (move/init errors, illegal subsets) etc

lqd (Oct 29 2019 at 19:28, on Zulip):

(and I don't know how you envisioned those and their integration with rustc)

nikomatsakis (Oct 29 2019 at 19:29, on Zulip):

seems like we should land polonius#134?

nikomatsakis (Oct 29 2019 at 19:29, on Zulip):

although I don't know what @Albin Stjerna was referring to with mem::replace

lqd (Oct 29 2019 at 19:30, on Zulip):

an older rev where I mem::replaced some of AllFacts' clones Vecs instead of cloning them

Albin Stjerna (Oct 29 2019 at 19:30, on Zulip):

Nevermind that

nikomatsakis (Oct 29 2019 at 19:31, on Zulip):

okok

nikomatsakis (Oct 29 2019 at 19:31, on Zulip):

if we were going to get rid of the clones

lqd (Oct 29 2019 at 19:31, on Zulip):

we can land if you think it's interesting niko, esp if it makes sense wrt the things we don't know about what you had in mind

nikomatsakis (Oct 29 2019 at 19:32, on Zulip):

if we were going to get rid of the clones

I guess we'd have to change to &AllFacts

nikomatsakis (Oct 29 2019 at 19:32, on Zulip):

I don't quite know why we don't give ownership

lqd (Oct 29 2019 at 19:32, on Zulip):

me neither tbh

nikomatsakis (Oct 29 2019 at 19:32, on Zulip):

we can land if you think it's interesting niko, esp if it makes sense wrt the things we don't know about what you had in mind

well I definitely think factoring out the "common code" is good

nikomatsakis (Oct 29 2019 at 19:32, on Zulip):

I'm more and more hoping that we can get away frm "specify the algorithm"

nikomatsakis (Oct 29 2019 at 19:33, on Zulip):

I guess that coming back to things I feel a bit overwhelmed by "too many choices"

lqd (Oct 29 2019 at 19:33, on Zulip):

one question I had was would rustc just call polonius once ? or differently, say to get init/move errors at a point in time, borrowck errors at another

nikomatsakis (Oct 29 2019 at 19:34, on Zulip):

I think rustc would call polonius once

nikomatsakis (Oct 29 2019 at 19:34, on Zulip):

presently we do all those things at once

lqd (Oct 29 2019 at 19:35, on Zulip):

because I was wondering whether the current NLL code would eg use liveness computed by polonius

nikomatsakis (Oct 29 2019 at 19:35, on Zulip):

I don't quite know why we don't give ownership

maybe just so the cli can run graphviz w/o a clone?

nikomatsakis (Oct 29 2019 at 19:35, on Zulip):

because I was wondering whether the current NLL code would eg use liveness computed by polonius

I would hope so

nikomatsakis (Oct 29 2019 at 19:35, on Zulip):

I mean it doesn't now

nikomatsakis (Oct 29 2019 at 19:35, on Zulip):

still, what do you mean actually?

nikomatsakis (Oct 29 2019 at 19:35, on Zulip):

in the presentation of errors?

Albin Stjerna (Oct 29 2019 at 19:35, on Zulip):

I don't quite know why we don't give ownership

maybe just so the cli can run graphviz w/o a clone?

That doesn't sound like a great trade-off honestly?

nikomatsakis (Oct 29 2019 at 19:35, on Zulip):

(I think all other uses of liveness would hopefully be replaced by polonius)

nikomatsakis (Oct 29 2019 at 19:36, on Zulip):

That doesn't sound like a great trade-off honestly?

no, no it doesn't :)

nikomatsakis (Oct 29 2019 at 19:36, on Zulip):

given that we're cloning anyway

lqd (Oct 29 2019 at 19:36, on Zulip):

yeah if all uses of liveness are replaced by polonius then we're good

nikomatsakis (Oct 29 2019 at 19:36, on Zulip):

I'm not 100% sure how to the "nice error reporting" would work -- I imagine we might be able to phrase some of what it does as well as polonius predicates -- but I assumed that would be something we'd worry about later

Albin Stjerna (Oct 29 2019 at 19:36, on Zulip):

There goes the "only calculate liveness for interesting variables" idea

nikomatsakis (Oct 29 2019 at 19:36, on Zulip):

Why?

nikomatsakis (Oct 29 2019 at 19:37, on Zulip):

rustc already tries to avoid computing liveness

Albin Stjerna (Oct 29 2019 at 19:37, on Zulip):

Ah

lqd (Oct 29 2019 at 19:37, on Zulip):

for liveness it can work

nikomatsakis (Oct 29 2019 at 19:37, on Zulip):

I guess my point is: I dont' think we use liveness for very much :)

Albin Stjerna (Oct 29 2019 at 19:37, on Zulip):

Well, it's less restrictive than I had expected

lqd (Oct 29 2019 at 19:37, on Zulip):

it works now in my branch I mean

lqd (Oct 29 2019 at 19:37, on Zulip):

for initialization it's more shaky

nikomatsakis (Oct 29 2019 at 19:37, on Zulip):

what is "it" here?

Albin Stjerna (Oct 29 2019 at 19:37, on Zulip):

...a relevant question

lqd (Oct 29 2019 at 19:37, on Zulip):

it = "only calculating a limited subset of liveness"

nikomatsakis (Oct 29 2019 at 19:38, on Zulip):

one thing: rustc does recompute a lot of the initialization stuff for the "drop elaboration" code. I am not envisioning that we move that into polonius (at this time)

nikomatsakis (Oct 29 2019 at 19:38, on Zulip):

also, what is your branch, @lqd? :)

lqd (Oct 29 2019 at 19:38, on Zulip):

lol

Albin Stjerna (Oct 29 2019 at 19:38, on Zulip):

I had an idea (in a different topic) of trying a more top-down approach that would only compute liveness for variables that we know are interesting because we know they have origins we are interested in, etc

nikomatsakis (Oct 29 2019 at 19:38, on Zulip):

also, what is your branch, lqd? :)

to be clear, I just feel like you have a lot of branches :)

lqd (Oct 29 2019 at 19:39, on Zulip):

very true :)

nikomatsakis (Oct 29 2019 at 19:39, on Zulip):

also, maybe we should move this over to some other topic, or something

lqd (Oct 29 2019 at 19:39, on Zulip):

https://github.com/lqd/borrow-check/tree/prefiltering contains both placeholder loans and some of the "filtering" we're talking about here

lqd (Oct 29 2019 at 19:39, on Zulip):

it filters too much as it also does on initialization, only checking it for "interesting variables" which comes out of the liveness and invalidated/erroneous loans

lqd (Oct 29 2019 at 19:40, on Zulip):

and that would prevent getting all init/move errors ofc

nikomatsakis (Oct 29 2019 at 19:40, on Zulip):

ok

lqd (Oct 29 2019 at 19:41, on Zulip):

once we can also solidify the plans wrt placeholder loans (eg the "cfg root" question, or how we'd emit those loans in rustc, say)

lqd (Oct 29 2019 at 19:41, on Zulip):

maybe land the "context" PR, etc then I can make PRs out of those branches

Albin Stjerna (Oct 29 2019 at 19:42, on Zulip):

Once the context lands I can (more easily) start testing my move errors

lqd (Oct 29 2019 at 19:43, on Zulip):

it's a bit tough to work on both perf and completeness at the same time as they step on each other's toes

nikomatsakis (Oct 29 2019 at 19:56, on Zulip):

it's a bit tough to work on both perf and completeness at the same time as they step on each other's toes

yep this is why I think we should focus on completeness first

nikomatsakis (Oct 29 2019 at 19:56, on Zulip):

once we can also solidify the plans wrt placeholder loans (eg the "cfg root" question, or how we'd emit those loans in rustc, say)

yep! need to do that

Last update: Nov 15 2019 at 20:30UTC