refactor(core,accountsdb,bincode,net): Fixes, improvements, renames + discuss

[InKryption]

This PR is mainly comprised of refactors, but also contains some slight deviations from a specific naming convention we follow implicitly: blanket screaming snake case constants.

Note: by implicitly, I mean that it isn't recommended by the zig style guide, nor do we outline it in our style guide.

In this PR I propose that we move to replace superfluous init methods with "init constants", which we could then also take advantage of when upgrading to zig 0.14 with the advent of Decl Literals. This was also in part prompted by a discussion we had on the logger PR. And of course, I am also proposing that we make these init constants normal snake case, instead of screaming snake case.

Also, after we decide on this, we should outline this in our style guide - I can also add that to this change set.

Of course review of any of the other changes in this PR is welcome.

[dnut]

This PR is mainly comprised of refactors, but also contains some slight deviations from a specific naming convention we follow implicitly: blanket screaming snake case constants.

Note: by implicitly, I mean that it isn't recommended by the zig style guide, nor do we outline it in our style guide.

I think this was just an oversight. My understanding is that it was a deliberate style choice. To conform to the convention, I've been trying to use uppercase for file-scoped consts, unless they are imports. But I think I typically use lowercase for struct-scoped consts. I understand that files are structs but you know what I mean.

I don't have a very strong opinion about it personally. If I had to choose, my vote would probably be to use lowercase for all constants.

In this PR I propose that we move to replace superfluous init methods with "init constants"

If all the function does is return a const, then I agree with removing the function. But I think we should distinguish between two situations:

1. If there is clearly one most sensible and obvious default approach to initialize the struct, then we probably don't even need a decl. Can't we just set defaults for the fields? Especially if it's already called "init" and all it does is return a constant. I'm curious to hear other opinions about this specifically.
2. If there are a handful of reasonable alternative initialization states for a struct, and they are needed in multiple places, then I agree with using const decls to represent them.

we could then also take advantage of when upgrading to zig 0.14 with the advent of Decl Literals.

This also works with init functions right?

[Rexicon226]

but also contains some slight deviations from a specific naming convention we follow implicitly: blanket screaming snake case constants.

I don't have a very strong opinion about it personally. If I had to choose, my vote would probably be to use lowercase for all constants.

I don't have a strong opinion either, but I do like the current "screaming snake case", it's pretty easy to visually see which identifiers are constants. That's just my preference, I do not mind other solutions.

If there is clearly one most sensible and obvious default approach to initialize the struct, then we probably don't even need a decl. Can't we just set defaults for the fields? Especially if it's already called "init" and all it does is return a constant. I'm curious to hear other opinions about this specifically.

I agree that could be moved to using default field values. For example, an init function that takes no parameters and doesn't do any side-effect like computations inside of itself (incrementing some external counter or something), also doesn't do any extensive pure computation to initialize values (compute some key?), is superfluous.

If there are a handful of reasonable alternative initialization states for a struct, and they are needed in multiple places, then I agree with using const decls to represent them.

In agreement!

This also works with init functions right?

Yep,

const S = struct {
    ... some fields
    fn init(x: u32) S {
       ... returns S
    }
};

const x: S = .init(10);

is a valid usage of decl literals. To add to that, init could be a create style function that returns a *S and that would work as well.

[0xNineteen]

tbh would prefer screaming-case for all constants (agreed const decls are better than an empty method call) - i think it shows the differentiation very clearly and is easy to read

If there is clearly one most sensible and obvious default approach to initialize the struct, then we probably don't even need a decl. Can't we just set defaults for the fields?

i feel like this is more dangerous, because it might allow you to initialize some fields, forget some others and everything compile fine -- with the decls, youre explicit that you want all fields as the default which is less error prone and more clear what you are doing

const x: S = .init(10);

idk about you guys, but thats so hard to read relative to x = S.init(10)

[InKryption]

@0xNineteen

const x: S = .init(10);

idk about you guys, but thats so hard to read relative to x = S.init(10)

I would say that's more a matter of familiarity, but the former provides strictly more information to the reader, in the least verbose way:

• const x = S.init(y); doesn't guarantee that S.init actually returns S, that's merely convention, and conventions sometimes are broken, for better or worse.
• const x: S = S.init(y); guarantees us that either it will compile because S.init returns S, or it will fail because it doesn't (ie perhaps it returns *S, or something else entirely), but can become very verbose (ie : std.ArrayList(T) = std.ArrayList(T).init(allocator)).
• const x: S = .init(y); guarantees us that x is of type S, whilst utilizing its namespaced initialization method. It is also trivially applicable to more complex initializations:
  • var list: std.ArrayList(T) = .init(allocator);
  • const ptr: *PinnedType = try .create(allocator);

It also comes with the benefit that it unifies all initialization (much nicer in the case of std.ArrayListUnmanaged - I don't know about you, but std.ArrayListUnmanaged(T){} looks horrid).

There's also the point that, const x: T = init in general is the style zig is moving towards:

• remove the destination type parameter from all cast builtins ziglang/zig#5909 - this essentially makes const foo: T = @cast(x); the first class citizen, in-expression casts are only given second consideration.
• Proposal: Decl Literals ziglang/zig#9938 - plus all of the PRs to replace default field values with descriptive decl literal inits (ie for std.ArrayListUnmanaged: .{} -> .empty).
• remove T{} syntax in favor of type coercion ziglang/zig#5038 - not accepted, but based on discussions it at the very least seems extremely plausible.

With all that considered, I think it would be along the path of least resistance to embrace this style.

[0xNineteen]

huh never considered the safety guarantees - thats kinda nice - maybe it is just a case of familiarity, i guess time will see

[Sobeston]

If there is clearly one most sensible and obvious default approach to initialize the struct, then we probably don't even need a decl. Can't we just set defaults for the fields?

i feel like this is more dangerous, because it might allow you to initialize some fields, forget some others and everything compile fine -- with the decls, youre explicit that you want all fields as the default which is less error prone and more clear what you are doing

100%. In many cases setting defaults for fields is subtly a really bad idea, because people will feel safe partially-initialising structs in invalid ways which will cause big problems later.

[0xNineteen]

few spots missed

[dnut]

If there is clearly one most sensible and obvious default approach to initialize the struct, then we probably don't even need a decl. Can't we just set defaults for the fields?

i feel like this is more dangerous, because it might allow you to initialize some fields, forget some others and everything compile fine -- with the decls, youre explicit that you want all fields as the default which is less error prone and more clear what you are doing

100%. In many cases setting defaults for fields is subtly a really bad idea, because people will feel safe partially-initialising structs in invalid ways which will cause big problems later.

So would you advocate completely against using defaults for fields in any situation?

[InKryption]

If there is clearly one most sensible and obvious default approach to initialize the struct, then we probably don't even need a decl. Can't we just set defaults for the fields?

i feel like this is more dangerous, because it might allow you to initialize some fields, forget some others and everything compile fine -- with the decls, youre explicit that you want all fields as the default which is less error prone and more clear what you are doing

100%. In many cases setting defaults for fields is subtly a really bad idea, because people will feel safe partially-initialising structs in invalid ways which will cause big problems later.

So would you advocate completely against using defaults for fields in any situation?

For my part, I would heed the recommendation of the zig documentation itself:
https://ziglang.org/documentation/master/#toc-Faulty-Default-Field-Values

Default field values are only appropriate when the data invariants of a struct cannot be violated by omitting that field from an initialization.

Though besides that, there's also just the point of what it means for a value to be a good "default". I like using specifically named initialization values/procedures for types which are purely data, whose data the programmer will have to directly interface with in some way (as is the case of Pubkey, Hash, Signature, and a myriad of other types).

[0xNineteen]

[Rexicon226]

LGTM other than these could of nits.

[Sobeston]

There's a couple things I'd prefer, style-wise:

• avoiding SCREAMING_CASE
• .isZeroed() feels a bit better than .isAllZeroes()

But these are really just nitpicks and this PR improves things overall so I'm happy with it.

all comments were addressed