chore(topology): Change SinkContext and TransformContext to expose fields publicly

[MOZGIII]

A follow-up on #2557 (comment).

[LucioFranco]

This seems fine to me, though I still am not totally a fan of removing the fn for using the struct items. I'll approve and defer to others though.

[LucioFranco]

So this is the unfortunate side effect of using the struct items, we have to add extra clones where as before we did that for you within the fn. This removed a lot of need deal with the borrowck.

[MOZGIII]

In my view, explicit clones are better. It's that thing where if you want your code to do some work - you have to do some work typing too 😄 I was really confused why we did so many clones needlessly while refactoring this. I know this is cold code, but it still bothers me, irrationally...
Actually, the fns API confused me before - when I wanted to pass things by ref (cause that was a sane thing to do - so I really wanted to pass by ref) I ended up having to clone, and I didn't like that at all.

[LucioFranco]

So clone is complicated because it can mean heavy allocation or a simple ref count bump. In most of our clones they are simple ref count bumps. So the idea is to say if I give you a concrete type back that is likely reference counting and its again cheap to clone it around. It removes the need to think about it. Most users that use a resolver want a 'static version of it not a &'_ version. So doing that clone for them makes that easier.

With explicit clones you basically now push that thinking onto the user which imo is even worse. This is a pit fall of clone and becomes quite confusing. So by exposing the types directly behind a & reference forces the users to think of this.

[MOZGIII]

Most users that use a resolver want a 'static version of it not a &'_ version. So doing that clone for them makes that easier.

My case was with an executor.

So the idea is to say if I give you a concrete type back that is likely reference counting and its again cheap to clone it around. It removes the need to think about it.

I still think about it, but I don't have a known-to-work solution of passing a ref. I understand the goal, and I wish it really did allow me to not think about clone costs, but I know that when I'm getting a type by-value it has to come from somewhere. The only solution I'm certain of is taking a ref.
I like the idea of lifting the cognitive complexity - but the by-value API produced quite the opposite effect on me.

[LucioFranco]

I like the idea of lifting the cognitive complexity - but the by-value API produced quite the opposite effect on me.

This probably comes down to the effect of the api wasn't documented correctly and the idea behind it wasn't explained. Which is my fault! I think this is more a philosophy thing rather than practical difference.

[LucioFranco]

I do think in general if we can avoid lifetimes we should. They have a complexity that isn't always needed. In this case I am convinced its not needed even if it does work. For something so foundational simplicity is key in a system with already so much complexity.

[MOZGIII]

In my view the absence of clear lifetime specification is the complexity, and rust provides wonderful tools to tackle that. Compared to C++, where people were bound to either use unsafe pointers or pass by value every time - for centuries - tools that rust offers are just a blessing. It'd be a shame not to use them. 😄 Maybe I'm just overreacting here, but I really like rust.

[LucioFranco]

Lifetimes are nice and Rust does a great job at handling them as you said. But unfortunately not everyone is used to them and it creates a slight barrier to entry imo.

[bruceg]

I think I agree with @LucioFranco on this one. I don't think moving the clones to the call sites enhances readability. Nor, I think, do these destructuring assignments, but that's a smaller detail. It might make sense for things like a &'static str or other values that are cloned rarely, but that's not what's going on here.

[MOZGIII]

I can re-add the functions and rollback the changes to the code tree, only leaving field renaming and changing the visibility to pub.
@LucioFranco, @bruceg what do you think? Would this be better?

I'd rather use https://github.com/Hoverbear/getset than manually implement accessor fns.

[lukesteensen]

I'm really failing to see the value of this change. I understand that in certain cases you'll want to know when something is being cloned and control that yourself, but none of these locations seem to qualify. Unless I'm missing something, every one of these calls was happening exactly once at build time so it wouldn't matter even if the clones were expensive (and they're not).

There are also some real downsides here. We've increased the amount of code, introduced variability at the call sites, and increased our coupling to the exact current implementation of the context structs.

I also like Rust and I think defaulting struct members to private across module boundaries is a good decision 😄 Lifetimes are a great way to reason about necessary complexity, but I'd much rather just avoid the complexity altogether when it's providing no value.

It appears I'm the third reviewer that's against this change, so my vote would be to simply close this PR and move on with the current API. If someone really feels strongly we can reconsider.

[MOZGIII]

The value (and the whole purpose of the PR) is to match the form of #2557. So we probably should reach a consensus there first. That said, the additional value this PR provides is that it shows how the world looks like if we use pub structs. I think it pays off since we can reason about how things based on a concrete example.

My position here is mainly that we want all context types to share the same design. What exactly that design would be depends, so let's leave this PR for now.
We can close this once the #2557 merges, and close or change and merge this PR depending on the results. Generally, I agree that this change doesn't generate the value that would outweigh the disadvantages. But maybe we'll fix this in the process.

Thanks everyone for the input. What's been discussed here is, generally, a discussion of the style, that can serve as a generic precedent.
I've gathered some insight into the team perspectives on what's considered complex and not complex, and it didn't match my personal estimates - so that was beneficial.