fix(jsii): two enum members cannot have the same value

[rix0rrr]

If two enum members have the same value, only the first one will
be retained.

This is a bit of an issue as we are renaming enum members: the named
version will never appear in the assembly, and so not work over jsii.

What's worse, if we deprecate-and-strip the original one, neither
of the enum members will appear.

Address this issue by working around TypeScript's natural tendencies
to collapse enum values. There was a bug in single-valued enum symbolIds
as well, which this PR also fixes.

Fixes #2782.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[nija-at]

Nice!

[njlynch]

Note that this as written is going to break the CDK build due to some existing enum names -- which were previously shadowed and ignored -- not adhering to jsii conventions. See #3208

[rix0rrr]

I actually don't think this is the best way to go anymore.

Quoth an internal exchange:

---

What does it even mean for 2 enums to have the same value? It doesn't mean anything. Enum values are supposed to be distinct.

What's going wrong here is that we are overloading the enum to be a mapping from 1 or more names we care about that we might want to change, to a single fixed value in CloudFormation that we cannot change. Well that shouldn't have been an num, that should have been a regular mapping.

So instead of

enum SubnetType {
  PRIVATE = 'private',

  PRIVATE_WITH_NAT = 'private'
}

We should have had:

enum SubnetType {
  PRIVATE,
  PRIVATE_WITH_NAT
}

const CfnSubnetType: Record<SubnetType, string> = {
  [SubnetType.PRIVATE]: 'private',
  [SubnetType.PRIVATE_WITH_NAT]: 'private',
}

And we're conflating the two.

(Disregarding the fact that in this particular case, the value of SubnetType actually doesn't go directly into CloudFormation so we had complete freedom and there was no need to pick conflicting values even)

---

So I think the better solution is for jsii to recognize and reject this situation, and fix the occurrences in the CDK.

Thoughts @nija-at, @otaviomacedo, @RomainMuller ?

[nija-at]

I agree with this argument. It's makes sense to me, in principle.

How do you think we'll implement this without making it (a) a jsii breaking change, and (b) apply it on the cdk without it being a cdk breaking change?

For the former, we could make this a diagnostic that is 'ignore' by default and enable it for all CDK packages.

For the latter, I suspect there will be some (perhaps small) set of users using the enum outside of the construct, for whom this will be a breaking change.

[rix0rrr]

without making it a [...] breaking change

I was planning to... not.

• We can fix CDK before rolling out the change.
• People using jsii and running afoul of this: well they should fix their code. The jsii upgrade treadmill isn't nearly as demanding as the CDK upgrade treadmill. You can choose not to upgrade your compiler, and everything will work fine (or you can fix it if you desperately want to).
• People using CDK enums outside their intended purpose: sigh. Of course they will. But they shouldn't.

I'm thinking we should discourage this going forward as well: assigning desirable string values to enums was the cause of this mess in the first place. How about we disallow assigning values to enums? (Error or warning) Then people also will not be tempted to use the enums out of context (not a lot of point in reusing SubnetType.PRIVATE if it's going to be an alias for you for the value 2).

But this is probably overly wishful thinking hey?

[nija-at]

Of course they will. But they shouldn't.

Why shouldn't they? It's an enum like any other. We haven't forbidden its use nor is it an obscure use case to write a CDK construct that relies on an enum provided by the CDK.

well they should fix their code. [...] You can choose not to upgrade your compiler, and everything will work fine

Agreed with this statement, as long as there is an upgrade path.
However, the upgrade path for the CDK is unclear (maybe only to me?) without incurring breaking changes. See above.

If the path for the CDK is unclear, then we have no guidance for the rest of the jsii ecosystem.

Here are the options I see -
(a) a diagnostic that warns by default but continues current behavior. New (and maybe all existing) CDK packages error in this case.
(b) a diagnostic that errors by default but can be disabled for specific enums (via annotation?)
(c) "drop the bomb" - make the breaking change but be prepared to revert if there is significant pushback.

(b) sounds like a reasonable approach with a decent upgrade path.

[rix0rrr]

What you are saying is we are locked in to existing enum values in the CDK libraries, right?

[rix0rrr]

We haven't forbidden its use nor is it an obscure use case to write a CDK construct that relies on an enum provided by the CDK.

Well it depends on what you do with it. If you pass the enum back to the CDK, you don't have a problem. If you compare to the symbolic value, you don't have a problem.

You only have a problem if you wrote your code to do "I know what value this enum evaluates to, let me write that directly to a JSON file"

[nija-at]

What you are saying is we are locked in to existing enum values in the CDK libraries, right?

Kind of, yes.

Well it depends on what you do with it

Good point.
Maybe it won't be as bad as I claim it to be. If the set of enum values stay the same, we may be mostly in the clear.

[rix0rrr]

@yuth is working on this