Recent changes in `interface-definitions` break the rust linter.

[beauremus]

Recent changes in interface-definitions break the linter. The generated code has the problem so we can't fix it unless we force Rust's opinions on the .proto files.

Originally posted by @rneswold in fermi-ad/extapi-acsys#65 (comment)

Should we consider adopting this approach to naming?

[rneswold]

I think the protocols should describe their purpose. We shouldn't have to come up with (possibly awkward) synonyms to avoid language constraints. Rust, fortunately, has ways of allowing illegal identifiers and suppressing lint warnings. Maybe developers using other languages can chime in whether they can tolerate an unconstrained naming convention.

[jacob-curley-fnal]

The concrete issue here is that the enum values have wildly different sizes. Rust is correct to flag that: enums should be different "flavors" of the same general idea. If two enum values are completely different in structure, then we're probably not talking about flavors of the same thing anymore - we're likely just talking about two different ideas that should be split out into their own concepts. It adds clarity. And while this philosophical point is getting caught by Rust, it really applies to all languages that support the concept of an enum.

In this case, it's the result value of devdb's InfoEntry. One enum value is a big object of data, the other is a string with error text. I don't think those are communicating the same idea. I think the error path should be wholly distinct from the happy path. Maybe we have two fields, and the client has to check both to see if it got an error or a result? It already has to disambiguate the one result field, so this isn't putting any extra burden on the client (it actually makes it easier, as it knows the result will always be DeviceInfo and the error text will always be a string; right now, the result could be either DeviceInfo or a string). In a perfect world, gRPC would have better support for error vs ok responses, like REST does. If we really need that chunk of error text getting to the caller, then this seems to me like the best path forward. And if we don't necessarily need that error text making it to the caller, then we should let the server throw an exception and use the in-built error handling of gRPC instead!

[rneswold]

gRPC would have better support for error vs ok responses, like REST does.

I haven't done a great job at reporting errors. Switching between GraphQL (via HTTP 1.1 and WebSockets) and gRPC (via HTTP 2) means there are different ways to propagate errors to the client. This definitely needs some attention and I welcome any help on understanding and improving the situation.

FWIW, the linter error I disabled was complaining that each enum value had the same prefix/suffix in its name. The linter doesn't like this:

enum Status {
    StatOkay,
    StatError,
    StatRetry
}

Because you have to use them as Status::StatOkay.

[jacob-curley-fnal]

Ah, right, forgot that was also an issue. And I still agree with Rust's assessment for enums globally. 😆
In DAQ.proto, we have

message Alarm {
    enum Status {
        STAT_NONE = 0;      //  No alarm
        STAT_HIHI = 1;      //  Higher than high-high limit
        STAT_HIGH = 2;      //  Higher than high limit (ACNET does not use)
        STAT_LOW  = 3;      //  Lower than low limit (ACNET does not use)
        STAT_LOLO = 4;      //  Lower than low-low limit
        STAT_BOOL = 5;      //  Boolean alarm (ACNET Digital / EPICS STATE)
    }
    enum Severity {
        SEV_NONE  = 0;      //  No alarm
        SEV_MINOR = 1;      //  Minor alarm (ACNET event)
        SEV_MAJOR = 2;      //  Major alarm (ACNET exception)
    }

But I'd argue it's less cluttered to say

message Alarm {
    enum Status {
        NONE = 0;      //  No alarm
        HIHI = 1;      //  Higher than high-high limit
        HIGH = 2;      //  Higher than high limit (ACNET does not use)
        LOW  = 3;      //  Lower than low limit (ACNET does not use)
        LOLO = 4;      //  Lower than low-low limit
        BOOL = 5;      //  Boolean alarm (ACNET Digital / EPICS STATE)
    }
    enum Severity {
        NONE  = 0;      //  No alarm
        MINOR = 1;      //  Minor alarm (ACNET event)
        MAJOR = 2;      //  Major alarm (ACNET exception)
    }

STAT_ and SEV_ don't add anything, IMO

[rneswold]

The problem is C++ (and maybe other languages) in that NONE is defined in two enums and there isn't a namespace for them so the compiler probably complains. In Rust, you have to specify Status::None and Severity::None (or add a use Status if you want a default namespace.)

More recent versions of C++ added enum class EnumName { } so you'd add EnumName:: to the values.

[jacob-curley-fnal]

I've consulted the Protobuf language guide on enums, and it looks like you're right. The C++ compiler is using a version older than C++11, so it doesn't support duplicate names inside different enums. It also sounds like the first-party code generators from Google all strip off the prefix for you when they make the code in the language you want. So this is a shortcoming of the Rust code generator. I've opened an issue in tokio-rs/prost's GitHub repo, so hopefully they can take care of it for us.

For now, I'll just raise the question for discussion: is the duplicate name needed? Why do both Severity and Status have a NONE option? If Status is NONE, should there be an alarm at all? Seems like we should simply not return a value. Same with Severity. What is the use of an alarm with no severity? If we're alarming on something, I would think the alarm is at least of minor interest to the operators?

The guide requires that there be a default, 0, entry for each enum. And it also states the default should be either <type name>_UNKNOWN or <type name>_UNSPECIFIED. At least in our case here, we've got two colliding enums, and two options for the default value. Would it clear the way forward to use one for Status and the other for Severity?

[rneswold]

NONE for Status doesn't make sense. The status is about are we in a high-high/high/low/low-low alarm state?" So NONE should be something like NoAlarm. For Severity, maybe NONE should be Normal?

[jacob-curley-fnal]

Made a discovery - prost will correctly generate enums if you use the overall enum name as the prefix, exactly as the enum name is declared. For example, the following will both generate Rust code that gets flagged by the linter:

enum Status {
    STAT_NONE = 0;
    STAT_LOW = 1;
    ...
}
// Produces Status::STAT_NONE, Status::STAT_LOW, etc.

enum Status {
    STATUS_NONE = 0;
    STATUS_LOW = 1;
    ....
}
// Produces Status::STATUS_NONE, Status::STATUS_LOW, etc.

Instead, we'd need to write:

enum Status {
    Status_NONE = 0;
    Status_LOW = 1;
    ...
}
// Produces Status::NONE, Status::LOW, etc.

This violates the Protobuf style guide for enums still, but passes the Rust linter. No idea if it passes the Protobuf linter. And no idea if this generates good code in any other language. I've updated the ticket I submitted to the prost maintainers with this info, though still not holding my breath that they'll try to fix it

[jacob-curley-fnal]

Update from some experimentation I did today: this seems to work as it's supposed to now. That is, I made a local update to the .proto files such that we had

enum Status {
  STATUS_NONE = 0;
...
}

And I got code to generate that met the Rust guidelines for not repeating prefixes. So we should be good to adopt the Protobuf style definitions in our .proto files while also being conformant to the Rust style guide with our generated .rs files. 🚀

Not sure what I was seeing before, but possible I had a stale generated file in my IDE or something