icerpc/slicec

Rename member

Opened this issue · 5 comments

slicec makes extensive use of the term 'member' that has no meaning in Slice. We need to rename all these members.

The correct terms are fields and parameters.

For context, slicec indeed has a Field struct, and a Parameter struct, that see much use.
But, these structs are very similar, since fields and parameters are very similar in Slice.

Very often, we'd write two functions, one for each struct, and the implementations would be identical,
so to avoid code duplication, we decided it'd be better to introduce an umbrella type.

So we added the Member trait, which is implemented by Field and Parameter.
It's sole purpose to reduce code duplication in cases where these types have identical behavior/implementations.
Member was simply the closest umbrella term for these two things.


You're correct Member isn't a Slice term, it's out own internal compiler jargon and we should be careful to never leak it.
To my knowledge, we don't. If you find somewhere where we do, please fix it or open an issue.

And maybe there are some places where we take a Member, and we actually only need it for one of either Field or Parameter.
These places should be updated to use the concrete types, for sure.

But for implementations that are shared between Field and Parameter, we can't just replace Member with them, without duplicating the function.

We should consider merging Field and Parameter into a single struct (Field). Then no need for this umbrella trait with an odd name.

Just as an aside, having lots of types is actually very standard in Rust. Even those that are nearly identical to one another.
Rust is all about specificity and safety. It's also about offloading as much checking as possible to the type-system.
Having a struct where sometimes this field shouldn't be touched, and other times it must be used is a Rust smell.

Also, in my opinion, just comparing the structs isn't a good indicator of actual similarity.
Class and Exception are almost identical, even more so than Field and Parameter.
Result and Dictionary are literally identical structs (they both just hold two types).


Looking through the code, I think this would be a net-zero change. Some things would get simpler, others more complicated.
I'd actually considered this in the past, but while they are very similar, there are some important differences:

  • parameters can be streamed, fields cannot. Seeing is_streamed on fields would looks odd (since streamed fields aren't a thing), and indeed, this field would be dead code for certain cases.
  • fields support doc comments, parameters don't. So the comment field would be dead code for certain cases.
    The logic for generating doc comments for them is very different too (if you look at slicec-cs).
  • Fields always need to have a name, parameters do not (single return types).
  • Fields and parameters support a different set of attributes (readonly, deprecated, etc.), and the attribute checker just matches on type (a very simple check). We'd have to look at the parent type, since type alone isn't sufficient, and looking at the parent type is also harder because:
  • they have different parent types. Calling parameter.parent() gives an &Operation. Merging Parameter and Field would mean this could no longer return a concrete type, which makes the API less convenient for consumers (they'd get a trait type: &dyn Container<Field>).
  • Everywhere where we match the possible &dyn Container<Field>s would need to be updated with an extra Operation case. This case would almost always be dead code. Since Operations are very different than structs, classes, exceptions.
  • fields can cause cycles, parameters cannot. The cycle checker uses this fact to know when to stop checking a branch.
    We'd need to re-evaluate the logic it's using to make sure it doesn't hit false positives. At the very least, it would be needlessly checking more things than necessary.
  • The way you generate links to them is very different in C#, and probably other languages too. Right now we use a simple match parameter -> paramref and field -> see cref. This is no longer sufficient, we'd need to check the parent, which again, is more difficult now.

I prefer to leave these types separated. It's more semantically cleaner.

At the Slice2 encoding level, parameters are just like the fields of an anonymous struct that gets encoded into a request / response payload.

The differences between struct fields and parameters are very small:

  • not having a name at all is unique to return parameter (when there is only one) - not a big difference
  • "stream" is unique to parameters

and that's it.

Class and Exception are almost identical, even more so than Field and Parameter.
Result and Dictionary are literally identical structs (they both just hold two types).

This is a bogus argument. The syntax is similar but the semantics is completely different. That's totally different for fields and parameters.