RFC: Centralize naming convention

[CasperN]

Currently, the various code generators reinvent their own ways to escape keywords and recase identifiers to fit with local style.
DRY style concerns aside, the manual approach might not be working so well given #7137, #7138, #7139, #7140, and #7141. #7111 discusses overriding our casing conventions which would require touching almost everywhere, but would be easy given centralized naming.

In #7111 (comment) I outline a class that can be used to centralize naming conventions. I think something like:

class CaseManager {  // Feel free to bikeshed this name
 public:
  struct Config {
    Case types;
    Case constants;
    Case methods;
    Case functions;
    Case fields;
    Case variants;
    Case namespaces;
    Case file_nodes;
    // most languages escpae keywords by appending "_" while csharp prepends "@"
    std::string keyword_prefix;
    std::string keyword_suffix;
    std::string object_prefix;
    std::string object_suffix;
    std::string namespace_seperator;
  };
  CaseManager(Config config, std::set<std::string> keywords)
    : config_(config), keywords_(std::move(keywords)) {}

  // Formats `d.name` as a type, then escapes it if its a keyword.
  std::string Type(const Definition& d) const;
  // Formats `d.name` as a constant, then escapes it if its a keyword.
  std::string Constant(const Definition& d) const;
  // analogous const methods for Method, Function, Field, Variant, etc

  // Formats `d.name` as a type, then adds prefix and suffix, then escapes it if its a keyword.
  std::string ObjectType(const Definition& d) const;
 private:
  Config config_;
  std::set<std::string> keywords_;
};

I think of it similarly to data access objects (DAOs) except its for applying policies to our identifiers. This would be a pretty substantial change as it raises the abstraction level from "string manipulation code in every code generator" to "configuration per code generator", but I think it will help simplify our systems and make them more maintainable. What do we think?

@aardappel @dbaileychess @vglavnyy @krojew (is there an @all-maintainers?)

TODO:

Convert code generators

• idl_gen_cpp
• idl_gen_csharp
• idl_gen_dart
• idl_gen_go
• idl_gen_grpc
• idl_gen_java
• idl_gen_kotlin #7198
• idl_gen_lobster
• idl_gen_lua
• idl_gen_php
• idl_gen_python
• idl_gen_rust
• idl_gen_swift
• idl_gen_ts
• idl_gen_json_schema
• idl_gen_fbs
• idl_gen_text

other stuff

• Incorporate CommentConfig
• Deprecate BaseGenerator::Namespace

[CasperN]

Also, imo, case_fmt_.Type(x) tells me more about the semantics of the generated code than ToUpperCamelCase(x)

[dbaileychess]

Testing mention @flatbuffers/maintainers

[dbaileychess]

Thanks @CasperN, I think this would be a good utility class to have.

Class name might be NamingManager or something, since it does more than just case manipulations.

I would make overloads for each of the methods that take in a const std::string& as well.

How do we handle:

1. Namespacing? Since these are often part of the name but may have different styling and separators (::, ., etc..)
2. Paths? I saw some cases in the code where we are changing the case of a path (for generating an include file), which lead to some odd rules.
3. Global/language overrides? Would we switch out the CaseManager that we pass to the code generator or would we set an override flag in the CaseManager?

[aardappel]

Yes, something like this would refactor the existing code, and centralize "how to make something camelcase" etc in one place. Then any overriding flags are suddenly simple to implement.

Each language would instantiate a default.

Generally sounds good to me? Of course a fair bit of work to do this, and I am not that convinced that these overriding options are that desirable, but I don't feel strongly either way.

[CasperN]

I would make overloads for each of the methods that take in a const std::string& as well.

In the long run, it's not clear to me that we should be using strings everywhere. There's a semantic difference between a fully qualified type, ns_a.ns_b.ns_c.TableA; the type, TableA, and its namespace, ns_a.ns_b.ns_c; and maybe these should be different types. We use a ton of strings and lose some type system protections for it...

However, that would be a different mass refactoring, so I agree that we should support const std::string& overloads in the mean time.

Namespacing? Since these are often part of the name but may have different styling and separators (::, ., etc..)

It seems reasonable to me. I can add Case namespaces and std::string namespace_seperator to Config.
Note that there's some namespace manipulation built into the CodeGenerator base class, which we'd also have to rip out.

Paths? I saw some cases in the code where we are changing the case of a path (for generating an include file), which lead to some odd rules.

sgtm, I can add Case filenodes; (until filenames and directories aren't the same). We're using a subset of characters that's portable between unix-likes and windows and I think our lower layer file APIs handle / vs \ so we can act as if we're all on linux.

Global/language overrides? Would we switch out the CaseManager that we pass to the code generator or would we set an override flag in the CaseManager?

I was thinking we'd have a factory fn per language. flatc.cpp would

1. invoke the factory e.g. RustDefaultNameManager
2. do some manipulation based on flags, e.g. call SetObjectPrefix
3. then pass it to the code generator

Of course a fair bit of work to do this, and I am not that convinced that these overriding options are that desirable, but I don't feel strongly either way.

Fair enough, I am 25% motivated by the case overriding FR, and 75% motivated by simplification/consistency

[jkuszmaul]

I'm not sure where the best place for this is, but there also tends to be a bit of statefulness that can be needed to account for some things. E.g., I encountered a situation where for typescript if I created a .fbs with:

enum foobar {
 EnumA,
}
table TableName {
  foobar:int;
  enum_member:foobar;
}

The generated code ends up with a constructor for TableNameT that looks like:

export class TableNameT {
constructor(
  public foobar: number = 0,
  public enum_member: foobar = foobar.EnumA,
){}

which results in a name conflict because within the context of the constructor, foobar refers to the variable, not the enum type. I imagine that there are similar types of pitfalls that must be tracked in other situations. The typescript codegen currently also has an AddImport call which imports types by there name, and if it notices that you are attempting to import multiple types with the same name, it will prepend the name of one of them with the namespace. I assume that these sorts of stateful, scope-based conflicts can also be encountered in other languages. Although whether this should be resolved by this proposal, by some other proposal, or just by better code design (e.g., the typescript codegen could always import with the namespace prepended, since it's just an internal thing; I bet there's some similar workaround for the object-API constructor issue), I do not know.

[CasperN]

That's a good point, and there's a similar shadowing issue in #6845.

Although whether this should be resolved by this proposal, by some other proposal, or just by better code design (e.g., the typescript codegen could always import with the namespace prepended, since it's just an internal thing; I bet there's some similar workaround for the object-API constructor issue), I do not know.

I think always using fully qualified types might be the way to go... but does that actually solve your problem if foobar and TableName are both in the root namespace?

[jkuszmaul]

Fully qualified names don't resolve the issue I was noticing--but it could simplify the logic around the already-existing code to handle this with imports. To resolve what I was noticing, maybe there's some fancy typescript syntax to help (despite mostly talking about typescript here I don't actually develop in typescript much...); alternatively, you could enforce a naming convention for the generated members of the object API that guarantees that they will be different from any actual type names, but I don't know if that is feasible or desirable, and keeping track of what conventions everything is generated to follow to guarantee that they don't overlap sounds cumbersome.

[CasperN]

#7143 is another casing related bug: Two fields are different in the schema but, due to case-style normalization, generate the same symbol, which is invalid.

[CasperN]

Looks like @tira-misu found

• [TS] Fix generation of struct members in object api #7148 : Another example requiring context aware escaping, due to field-name/type-name collision, like @jkuszmaul's example above
• [C#] Fix collision of field name and type name #7149 : a reason to not escape keyword identifiers in type script

[CasperN]

@dbaileychess would you like to claim some subset of code generators to refactor? I don't mind doing it myself, but help is appreciated.