Stream: project-error-handling

Topic: Frames Iterator PR


view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 17:19):

If anyone has additional bandwidth, I could use a second pair of eyes on the frames iterator PR I'm working through: https://github.com/rust-lang/rust/pull/78299

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 17:19):

I'm currently trying to implement Iterator for the Frames type, but I'm running into an issue with how I'm specifying the Iterator's type.

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 17:22):

Here's the actual error I'm getting: Screen-Shot-2020-12-18-at-11.21.36-AM.png

view this post on Zulip Joshua Nelson (Dec 18 2020 at 17:27):

I'm confused why the compiler thinks this is specialized

view this post on Zulip Joshua Nelson (Dec 18 2020 at 17:28):

is there more to the error?

view this post on Zulip Joshua Nelson (Dec 18 2020 at 17:28):

oh you specified Item twice

view this post on Zulip Joshua Nelson (Dec 18 2020 at 17:28):

you only need to specify it for Iterator, not IntoITerator, I think

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 19:50):

Hm, I still get a similar error, it's just now pointing at the type IntoIter declaration: Screen-Shot-2020-12-18-at-1.48.36-PM.png

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 19:50):

Even if I remove that line, it still complains about the same thing.

view this post on Zulip nagisa (Dec 18 2020 at 20:01):

anything that is an iterator is also an IntoIterator: https://doc.rust-lang.org/stable/core/iter/trait.IntoIterator.html#impl-IntoIterator-8

view this post on Zulip nagisa (Dec 18 2020 at 20:01):

So implementing both Iterator and IntoIterator would be specializing the linked impl.

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 20:02):

Ah, so just implementing Iterator is enough?

view this post on Zulip nagisa (Dec 18 2020 at 20:03):

Depends on what you're looking to achieve. I've no context here.

view this post on Zulip nagisa (Dec 18 2020 at 20:03):

It is plausible that you might want IntoIterator and not Iterator directly (much like vector or slice don't implement Iterator directly)

view this post on Zulip Jane Lusby (Dec 18 2020 at 20:04):

my guess is that this is the type returned by the frames fn, which is supposed to be an Iterator already

view this post on Zulip Jane Lusby (Dec 18 2020 at 20:04):

let me double check that vec::Iter is an iterator and not intoiter

view this post on Zulip Jane Lusby (Dec 18 2020 at 20:05):

ye

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 20:05):

The Frames type wraps a Frames::inner field, which is what is holding the Vec<BacktraceFrame>s

view this post on Zulip Jane Lusby (Dec 18 2020 at 20:06):

hmmm

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 20:06):

So I thought I would need to implement Iterator for the Frames type to expose the Vec<BacktraceFrame>s as an iterator.

view this post on Zulip Jane Lusby (Dec 18 2020 at 20:06):

aah you implemented Iterator as just pop

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 20:06):

Yea, I'm also now sure popping is what we want here to yield the frames in the desired order.

view this post on Zulip Jane Lusby (Dec 18 2020 at 20:06):

I'm assuming that is fine

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 20:07):

I feel like a VecDeque and popping from the front would make more sense

view this post on Zulip Sean Chen (he/him) (Dec 18 2020 at 20:07):

But I'm not sure

view this post on Zulip Sean Chen (he/him) (Jan 05 2021 at 22:08):

Is anyone familiar with std::lazy::SyncLazy? @Ashley Mannix suggested that it be used to replace the Mutex around captured backtraces:

// in backtrace.rs
enum Inner {
  Unsupported,
  Disabled,
  Captured(Mutex<Capture>),
}

The Captured(Mutex<Capture>) would be changed to Captured(SyncLazy<Capture>).

Is SyncLazy basically a lazy Mutex? I'm also not really sure how to use its API to replace the calls to capture.lock() where we acquire the Capture inside the Mutex. Anyone have insight into this?

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:08):

@Sean Chen im guessing its like a mutex that doesnt have an &mut T api

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:08):

so it can only either be initialized or accessed

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:09):

i'm not positive but I think that's the new once_cell built into std

view this post on Zulip Sean Chen (he/him) (Jan 05 2021 at 22:10):

Oh, so we'd replace any captured.lock() calls with calls to the SyncLazy::force method to access the type inside?

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:10):

not sure

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:10):

i'll have to look at the API, in a meeting atm

view this post on Zulip Joshua Nelson (Jan 05 2021 at 22:11):

@Sean Chen that, or just dereference it; it Derefs to Capture

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:11):

looks like yea

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:11):

oh

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:11):

^

view this post on Zulip Joshua Nelson (Jan 05 2021 at 22:17):

you can sort of imagine it as a RwLock that's specialized to only ever have one write

view this post on Zulip Ashley Mannix (Jan 05 2021 at 22:35):

Ah yeh, that suggestion was based on an assumption that we only lock the capture to do some kind of deferred initialization work. With SyncLazy we can still initialize lazily, but also deref as normal and return borrows to those frames without needing any locks. It would avoid having to do any cloning

view this post on Zulip Ashley Mannix (Jan 05 2021 at 22:35):

But I should look at the backtrace impl properly to see if that’s actually the case 🙂

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:36):

@Ashley Mannix i moved your message to a diff topic

view this post on Zulip Jane Lusby (Jan 05 2021 at 22:36):

assuming it was meant for this one

view this post on Zulip Ashley Mannix (Jan 05 2021 at 22:38):

Yep thanks :pray::big_smile: I’m on mobile and thought I was in the right one

view this post on Zulip Ashley Mannix (Jan 05 2021 at 22:46):

Ok, so we lock the capture to call the resolve method. It might be slightly tricky to make the current SyncLazy API do that, but it should be possible with a regular old sync::Once. @Sean Chen I can put together a quick patch now to show you what I mean if you like :smile:

view this post on Zulip Sean Chen (he/him) (Jan 05 2021 at 22:48):

That would be helpful :smile:

view this post on Zulip Ashley Mannix (Jan 06 2021 at 00:20):

@Sean Chen I ended up opening https://github.com/rust-lang/rust/pull/80736 :smile:

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 20:07):

So I made the classic mistake of not implementing the changes in a feature branch separate from my local master branch. I git pulled into my local master (where I've been making all my changes as part of this PR) to merge in @Ashley Mannix's changes and fixed a merge conflict.

Am I good to push these changes to my fork? Or do I need to rebase against upstream master?

view this post on Zulip Joshua Nelson (Jan 13 2021 at 20:32):

@Sean Chen merging and rebasing are normally second, I would be hesistant to do both

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 20:36):

Hm, so what should I be doing instead? I want to incorporate Ashley's merged PR into mine, but I also am working from my fork's master branch instead of in a separate feature branch.

view this post on Zulip Joshua Nelson (Jan 13 2021 at 20:37):

I am confused what your git state looks like

view this post on Zulip Joshua Nelson (Jan 13 2021 at 20:37):

does git push work?

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 20:40):

Yes, but I need to merge in all of the upstream changes that have taken place since my fork (which has been a while). I guess I'm under the impression that I shouldn't be merging in all of those changes into my PR, but perhaps that's a faulty assumption.

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:43):

@Sean Chen if they're already in the master branch on the rust-lang/rust repo then it shouldn't matter

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 20:45):

So.. this is fine? https://github.com/rust-lang/rust/pull/78299/commits/43b9783292af62b5b60e323f5939b9c9a7e9801c
:grimacing:

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:47):

o.o

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:47):

maybe not

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:47):

I'd not have expected the diff to change once you updated your branch from master

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 20:47):

Yea, I figured :sweat_smile:

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:48):

yea i have no idea what that diff means

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:49):

oh its submodules

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:49):

confused

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 20:49):

Frankly, I think the cleanest way to resolve this is to just close this PR and re-open a new one. There's no real need to keep any of the history pre-merging in Ashley's PR.

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:50):

you shouldn't need to close the PR to do that

view this post on Zulip Jane Lusby (Jan 13 2021 at 20:50):

you can always just squash and cleanup the history then force push your branch

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 20:54):

So combine every commit I've made as part of this PR into a single one? What do you mean by "cleanup the history"?

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 21:36):

Or by squash do you mean get rid of every commit I've made as part of this PR?

view this post on Zulip Jane Lusby (Jan 13 2021 at 21:37):

oh, that's what I thought you meant by "not keep any history pre-merging ashley's PR"

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 21:37):

Sure, that works

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 22:00):

When doing the clean up, should I only be removing commits I made from my local commit history, or just get rid of all of them since my first commit?

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:05):

not sure what you mean

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:05):

commits I made from my local commit history

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:05):

the only other commit is the merge commit right?

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 22:06):

Right, all the other commits that were merged in

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:06):

what I'd do is back out the changes that you didnt make yourself including the merge PR

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:06):

so its just your commits

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:06):

then squash that down to 1 commit

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:07):

then rebase that on master

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:08):

has ashley's change landed on master

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:09):

it has

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:09):

yay

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:09):

okay so yea

view this post on Zulip Ashley Mannix (Jan 13 2021 at 22:52):

Another option if it gets too tangled could be to create a new temp branch from master, and cherry pick your commits onto it then force push it as your upstream branch. Submodules can be really nasty to work with... I always land in a mess with them :sweat_smile:

view this post on Zulip Jane Lusby (Jan 13 2021 at 22:52):

same

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 23:31):

All right, Jane and I were just trying to work through my screw up, but to no avail. I'm gonna go with the nuclear option and close this PR and re-open a new one. I don't care to keep any of the history pre-Ashley's merged PR.

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:31):

I don't think it was your screwup fwiw

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:32):

its just git

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:32):

combination of long lived branch, multiple merge commits, + submodules being updated between those merge commits

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 23:32):

Haha, thanks for the sentiment, but I'm sure I had a part to play :sweat_smile:

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:32):

i mean, i have no idea what you should have done differently

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:32):

maybe the rustc dev guide has suggestions

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:32):

ill check

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 23:33):

I think I should have rebased on top of Ashley's PR?

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:33):

https://rustc-dev-guide.rust-lang.org/git.html#quick-note-about-submodules

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:34):

the "if you run into an error like this you did nothing wrong" bit at the end has me thinking you did nothing wrong

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:34):

lolol

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:34):

apparently there's a work around here

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:34):

https://github.com/rust-lang/rust/issues/77620#issuecomment-705228229

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:34):

i have no idea what that does

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:34):

spooky

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 23:35):

So I needed to run this script: git diff --name-only | while read dir; do cd $dir && git checkout -f HEAD && git clean -d -f && cd -; done

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:35):

not sure

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:35):

but lets try

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 23:35):

Haha, sure, why not. I'll try it and report back

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:35):

try doing

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:35):

git submodule update first

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:40):

bah

view this post on Zulip Sean Chen (he/him) (Jan 13 2021 at 23:40):

It doesn't seem to be doing anything

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:40):

still not working when i try it

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:41):

yea

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:41):

just go ahead and remake the PR

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:41):

it looks like they encourage always rebasing

view this post on Zulip Jane Lusby (Jan 13 2021 at 23:41):

so i guess in the future just avoid merge commits


Last updated: Jan 29 2022 at 09:28 UTC