leancodepl / contractsgenerator-dart

Dart contracts client generator for a CQRS API
3 stars 1 forks source link

Generated class names can clash #79

Open shilangyu opened 2 years ago

shilangyu commented 2 years ago

Currently, to pick a class name for a contract statement the following is done:

  1. Take the statement name
  2. If it collides with any other statement name (regardless of their namespace), then go one namespace up and prefix your current name with the namespace name
  3. Repeat step 2 until there are no more conflicts

For instance consider two statements One.Two.Three and Four.Two.Three where . denotes a namespace accessor.

Then to find the class name for One.Two.Three:

  1. Start with Three
  2. It collides with Four.Two.Three, go one namespace up: set current name to TwoThree
  3. Still collides with Four.Two.Three, go one namespace up: set current name to OneTwoThree
  4. No collisions. Final class name is OneTwoThree

This algorithm was chosen because Dart does not have namespaces and this algorithm produces possibly shortest names without collisions... Or so I thought.

Now consider the following statements: One.One, OneOne, Two.One. If you follow the algorithm, the resulting class names will be respectively OneOne, OneOne, and TwoOne. That's a name clash.

This brings us to the question: how do we want to generate names for such cases? Or, what would be a different (better) way of picking class names?

bartekpacia commented 2 years ago

Off the top of my head: replace . with _? Then for your second example, we'd get:

shilangyu commented 2 years ago

_ is a valid identifier character, so it could be used in a statement name and lead to clashes

bartekpacia commented 2 years ago

Is $ a valid identifier in C#?

shilangyu commented 2 years ago

Yes, I am pretty sure dart and c# shares the same character set for identifiers there goes my uneducated assumption

jakubfijalkowski commented 2 years ago

Nope, $ is invalid char in an identifier in C#. We might also limit valid identifier character on contracts level if it will be necessary.

Also, I think the "fix" (I still don't like the general algo - it does not generate stable names) for this would be slightly simpler. It relates to point (2):

If it collides with any other statement name (regardless of their namespace),

You should not check if it collides with any other statement, but with already used identifiers. So basically keep a hashset of already used identifiers and check with it. There will also be a case where this fails: a) NS2.Class b) NS2.NamedClass c) NS2.MyNamedClass d) NS2.NSMyNamedClass e) NS.My.Named.Class

For (e), it will try:

  1. Class collides with (a),
  2. NamedClass collides with (b)
  3. MyNamedClass collides with (d)
  4. NSMyNamedClass collides with (e).

So it also needs some other suffix mechanism (e.g. appending numbers).

bartekpacia commented 2 years ago

What if we duplicated the C# namespace hierarchy in generated Dart contracts? Then it'd be the contract user's responsibility to import the correct namespace. Kinda ugly though, and too complex, and too big of a change.

So for your second example, we'd get:

$ tree
.
├── One
│   └── One.dart
├── One.dart
└── Two
    └── One.dart
jakubfijalkowski commented 2 years ago

That, IMO, is the ideal option, but I think I am the only one who does think that.

shilangyu commented 2 years ago

You should not check if it collides with any other statement, but with already used identifiers

Is this stable though? A new statement comes along and affects others (as long as it is ordered before them). Also, I kinda don't like its stateful nature and dependence on order, it feels weird that some statement will receive a different naming just because it is ordered earlier/later. Plus generic nested classes such as Values will never "stabilize" to a more concrete name.

I feel like the only "stable" solution is when the generated name is fully independent of other statements. Current stable solutions:

  1. filesystem simulating namespaces (readable, solves collision problem, DX usability is not very good)
  2. names are always the full namespace (ok-ish readability, does not solve collision problem, DX usability ok)
  3. simulating namespaces with static classes (bad readability, solves collision problem, DX usability good)
jakubfijalkowski commented 2 years ago

Is this stable though?

Stable in the same way that the current solution is, meaning - not stable at all.

  1. names are always the full namespace (ok-ish readability, does not solve collision problem, DX usability ok)

Why not?


Also, I'm all for option (1) - with Cmd+. shortcut you basically get good enough DX so that the multitude of directories is not a problem (but I would still truncate some of the "static" directories).

shilangyu commented 2 years ago
  1. names are always the full namespace (ok-ish readability, does not solve collision problem, DX usability ok)

Why not?

Same case as in before, example: One.One and OneOne

jakubfijalkowski commented 2 years ago

Ah, got it - I assumed that the "dot" (or any other separator) stays.

mishioo commented 1 year ago

I don't have much to add to the discussion, just wanted to let you guys know that we encountered this issue in one of the projects.