Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a chk package for checked types #2743

Closed
wants to merge 2 commits into from
Closed

Add a chk package for checked types #2743

wants to merge 2 commits into from

Conversation

eljobe
Copy link
Member

@eljobe eljobe commented Oct 17, 2024

The package name is intentionally very short because it will be included in function and method signatures and the code may become unwieldy if the package name is something like constraints.

The motivation for attempting to add a package like this was born out of wanting to simplify some code in the bold repo where we were checking that virtual > 0 in way too many places. But, I think this could be generally useful and would like to have it in the nitro repository so we can start adopting it in the codebase. Alternatively, if we think it would be a generally useful library, we could pull it out into a small repository and let it stand on its own.

The main purpose of the pull request is to see if we have a collective desire to use this kind of library, and, if so, where we would like it to live.

The package name is intentionally very short because it will be
included in function and method signatures and the code may become
unwieldy if the package name is something like constraints.
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Oct 17, 2024
@PlasmaPower
Copy link
Collaborator

I'm not sold on the UX tradeoff here, especially given that Go lacks operator overloading. My experience in Rust with the NonZero type hasn't been great in terms of the effort to create it vs safety, and the Go type seems a bit worse there in that it's hard to add two Pos64s for example. I'd like to see an example of how this would be used for a real function, and compare it there, as I'd worry it'd be better to just have a check in the function accepting a chk.Pos64 to error if the input uint64 is zero instead.

Side note -- I need to brush up on my Go inheritance. Can I do a > b for Pos64s? That's only if it's declared as type Pos64 = uint64 right? How about x := chk.Pos64 {0} ? Hopefully it's treated as a private value and that's not allowed.

@eljobe
Copy link
Member Author

eljobe commented Oct 18, 2024

The point about lack of operator overloading is definitely a good one. It is the biggest tradeoff I see in taking this approach.

As to your specific questions about what's allowed:
No, you can't a > b because > is not implemented on struct.
No, you can't x := chk.Pos64{0} because it's a private value. But, you can x := chk.Pos64{} because all public types can have zero values. That's why I made this type's "zero" value 1.

I'm happy to abandon this approach. Ideally, we'd also capture an Architecture Decision Record explaining why we don't want to consider this in our codebase in the future. It's great to be able to reference decisions like this in the future (in case someone is tempted to do this later.) What do you think about the idea of trying to get in the habit of creating those kinds of records?

@PlasmaPower
Copy link
Collaborator

Yeah, that sounds good to me. I'm definitely up for revisiting this any time we encounter a package we think we could use this type btw. It might be that it doesn't make sense to use this type in most places, but there's a couple places in the codebase where it's helpful.

@eljobe
Copy link
Member Author

eljobe commented Nov 29, 2024

Closing in favor of #2812

@eljobe eljobe closed this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants