RFC 287: CLI deprecated warnings

[skinny85]

• Tracking issue: Deprecated API Warnings #287
• Rendered version

---

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

[eladb]

Might be worth doing a prototype

[eladb]

There is also the alternative of manual annotation, which I understand we dismissed because it's hard to enforce, but worth adding

[skinny85]

I don't think I know of this alternative. Do you care to expand on how would that work Elad?

[eladb]

Just Hand-Writing code for each deprecated case.

[eladb]

Can you elaborate on this?

[skinny85]

Added some more explanation. Let me know if it's clear now.

[eladb]

What's the best way to make this capability available to other CDKs?

[skinny85]

I feel like it'll be hard to share this solution between CDKs. Even if we did the changes in JSII, the part that gathers the metadata into the Stack, and the part that renders it in the CLI, is CDK-specific. It would be hard to move that to a common place like constructs (because it uses the knowledge of the output format of each CDK - in the case of the AWS CDK, the Cloud Assembly, for instance).

[rix0rrr]

How would it work for struct properties?

[skinny85]

I guess for struct properties, we would have to detect their usage statically in JSII 😕.

[skinny85]

Thanks for the comments @eladb @rix0rrr ! I've incorporated them in the newest revision.

Pull request has been modified.

[skinny85]

@eladb I talked about this with @rix0rrr offline, and I think I'd want to re-structure the RFC a little bit.

Since the RFC format has a "working backwards" structure, it heavily encourages proposing a concrete solution to a problem, with a given customer experience.

I think maybe we should step a little, and re-phrase this RFC in terms of the problem we're trying to solve. In this case, it is:

We want to help our customers migrate their applications from CDK V1 to V2

Given that goal, there are a few ways we can solve this issue.

The ideal experience I can imagine for this would be some tool, let's hypothetically call it cdk upgrade, that actually changes your code to be V2-compliant:

1. Changes the packages you depend on in package.json / pom.xml / requirements.txt/ etc.
2. Changes the imports your code uses, if needed.
3. Changes the usages of deprecated elements to their non-deprecated equivalents.

While this is the ideal experience, I think we can agree that building it would be fantastically expensive.

Given that, the second best thing we can do is warn the customer when they're using something that will not be available in V2, like deprecated elements (I can definitely see extending it to things like setting no longer supported feature flags, etc.). While I originally said in the RFC that it should happen for anything that does cdk synth, Rico convinced me that this is locking in to a particular solution too strongly.

What are the possible solution we can use to emit these warnings?

1. Parse the customer's code directly

Advantages:

1. Allows us to discover some tricky deprecated elements,
   like enum values, or deprecated types used only in type declarations,
   that will be difficult to do with only runtime reporting.
2. Would prevent reporting warnings for deprecated usages outside the customer's code
   (for example, in our own libraries).

Disadvantages:

1. We would have to write a separate parser and type checker for each language supported by the CDK.
2. This additional parsing could have an adverse impact on the performance of CDK commands. This downside can be mitigated by moving this to a separate, non-synth command (cdk doctor, perhaps?), although that has its own downsides (customers might not know about a separate command).
3. It's not obvious this analysis can even be performed at all in the case of dynamically-typed languages like JavaScript or Python, and even TypeScript has some edge-cases making it pretty difficult.

2. Use TypeScript decorators to add warnings at runtime

Advantages:

1. Changes only needed in CDK code.

Disadvantages:

1. It won't be possible to reliably get the module the deprecated element belongs to
   (only the name of the element) --
   the decorator is simply a JavaScript function defined in core,
   and doesn't have (easy) access to the information on what module a given object it's called on belongs to.
2. TypeScript does not allow decorators on interfaces
   (neither on the interface directly, nor on any of its properties).
   This means we will not be able to register warnings for structs, or struct properties.
3. We will not be able to distinguish between the customer's and our's usages of deprecated elements, which means the customer will be getting warnings they cannot fix.

3. Modify JSII to change the emitted JavaScript for deprecated elements

Advantages:

1. Doing it in JSII will make it easy to record the module the given element is in.
2. Doing it in JSII allows this functionality to be used by other JSII projects,
   like cdk8s, or cdktf.

Disadvantages:

1. Because the code adding the warning for the deprecated elements will be CDK-specific,
   it will be pretty awkward to correctly pass what exactly that code should be from CDK to JSII
   (and, additionally, that code will most likely be slightly different for each type --
   class, method, property, etc. --
   of deprecated element we support).
2. Requires coordination between the JSII and CDK projects
   (merging a JSII PR, waiting for a release, updating CDK to use the new version, etc.),
   which typically lengthens the total development time.
3. We will not be able to distinguish between the customer's and our's usages of deprecated elements, which means the customer will be getting warnings they cannot fix. We can mitigate that warning by creating the API for silencing warnings, and then using that new API in all sites of deprecated code in our libraries, but that will be tedious, manual work, which will be hard (but probably not impossible) to verify in an automated way that it was done.

4. Rely on linters

We can modify JSII emit deprecated elements into the source code of the languages used, which is then picked up by a linter for that language.

Advantages:

1. A simpler solution than nr 3 in terms of the number of JSII changes required.
2. Allows us to distinguish between the customer's and our own usages of deprecated elements, reducing the number of false positives.

Disadvantages:

1. Will require the customer to run a separate command on their source code, language-specific.

5. Do it "manually"

For every deprecated property, in the class that uses it, check if it was provided, and register a warning. In the constructor of every deprecated class, register a warning. Etc.

Advantages:

1. Simplest technically.
2. Allows us to suppress the warnings for our usages of deprecated elements.

Disadvantages:

1. Tedious, manual work that will be easy to miss.

6. Etc.

I assume I haven't though of some other possible solution...

---

@eladb @rix0rrr - what are your thoughts on re-writing the RFC to be more in that format?

[eladb]

@skinny85, @rix0rrr, I agree that we need to devise a holistic story for migration from V1 to V2. Incidentally, @nija-at and I discussed this last week, and said we will likely need a dedicated RFC for this that will work backwards from the migration process. I think that for dev-preview we just need a doc that tells users what they need to do (we even have some snippets of it in the monocdk RFC and the constructs RFC).

Having said that, as I see it, warnings about usage of deprecated APIs has value outside of the migration point. We don't want users to use deprecated APIs in general, and we also want them to approach new major versions with the minimum amount of required changes. This implies that we want to inform them about usage of deprecated APIs as soon as possible. The shorter the list of changes they need to make (and the more mechanical it is), its more likely they will migrate. Additionally, some of the alternative solutions described above will not work after migration, only before, because the 2.0 codebase does not include any of the deprecated APIs (at least per our current plan).

As for solutions, I still think we should deeply look into the straightforward alternative of manually annotating usage of deprecated APIs. I have a feeling we ditched it too soon and I think from a cost/benefit perspective it's actually the most appealing one. I understand it might not be fool-proof and/or automatic as the other options, but I think it can pragmatically address the problem we are trying to solve (i.e. "warn users of 1.0 about usage of deprecated APIs").

I will articulate the solution so it's clear what I mean: we will manually go, search for "@deprecated" and add Annotations.addDeprecation() calls everywhere in our codebase when deprecated APIs are used (method calls, classes, props, etc). Simply write code. This is a one-time, mechanical effort across our codebase, which I don't think is going to be very expensive. As soon as we do that, we've already addressed the major customer need: v1.0 users will get warnings during synth/deploy if they use deprecated APIs (yeah they will see usage of deprecated APIs in dependencies but we can attach stack traces to construct warnings). Even if this is where we stop and maybe forget a few instances, I would argue that's probably "good enough" (esp. given IDEs also show usage of deprecated APIs).

We can also add a PR linter rule (@iliapolo can help) to identify introduction of new @deprecated APIs and require the PR diff will also include an addDeprecation() call. Again, this is not fool-proof and may require overrides in some occasions, but I'd argue it's completely sufficient for the need.

The main benefit of this approach is that we know it will work for all use cases and deprecation types (I still don't understand how some of the proposed solutions will identify usage of deprecated _props in constructs). It will also not require us to go into an adventure of building sophisticated tools and introduce more uncertainty into the 2.0 plan.

Given our current state with 2.0 timelines (and generally), it's important that we will be critical of how much risk/uncertainty a certain approach adds (that's missing in the analysis of the alternatives). In my mind all the solutions that involve per-language code analysis introduce too much uncertainty for this stage in the project, and any non-trivial solution requires a prototype to prove that it can address all the types of deprecations.

[rix0rrr]

building sophisticated tools

This does not need to be the case. We can just integrate with existing language tooling, for even less effort than writing our own dynamic deprecation checking (which, as shown, cannot check all usages of deprecations).

• TypeScript: eslint plugin https://github.com/gund/eslint-plugin-deprecation
• Java: we're already covered because we already emit the @Deprecated annotation (compiler already produces warnings)
• .NET: we're already covered because we already emit the [System.Obsolete()] annotation (compiler already produces warnings)
• Python -- the only one that's not covered right now.

Is adding a @jsii.deprecated attribute to the Python generator and writing a documentation page on how to set up eslint for TypeScript projects is not less effort than manually annotating all code everywhere?

Not to mention that relying on existing static analysis tools produces better results (and for Python the results will be equivalent, so at least not worse).

Also, don't forget that we're going to have to remove all that Annotations.addDeprecation() code again in the v2 branch, where it's going to lead to forward merge conflicts which take time to clean up... so the TCO of this code is more than just a Ctrl-F and copy/pasting some lines in (which is probably already going to be quite sizeable, honestly). And we'd have to do the same again for deprecations added later in v2.

The ROI on manually writing a bunch of checking code seems to be strictly lower than taking a couple minutes to look around and reuse what's already there... and even doing the latter doesn't seem to be all that expensive when compared to the costs of the naive solutions.

I really don't get why you are insisting on it so much?

[eladb]

It's an interesting analysis. The fact the TypeScript, Java and .NET are technically already covered is interesting (and I'd argue that now that VSCode strikethroughs deprecated APIs we don't even need the eslint rule). If we have a solution for python then we basically don't need to do anything to address the main concern.

I like this direction: focus on finding a way to warn Python users and basically that's it. No need to do any code analysis or write new tools. If we know users get warnings (either compile time or IDE) about usage of deprecated APIs, I think we've covered the need this RFC attempts to address.

---

don't forget that we're going to have to remove all that Annotations.addDeprecation() code again in the v2 branch

I was under the impression that we are just going to hide deprecated APIs, not delete the code. Did I miss anything?

[rix0rrr]

I'd argue that now that VSCode strikethroughs deprecated APIs we don't even need the eslint rule

Minor disagreement on that. What most people are going to use this for, especially in the context of migration, is going to be to answer the question "am I using any deprecated APIs in this project? yes/no" (as a subquestion of the ACTUAL question "can I safely upgrade this project?")

Having to open the project in an IDE and scrolling through every file to see if any identifiers are struck through is not a good way to answer that question. It will do nicely if you keep on top of it and diligently replace the deprecated elements as soon as you see them, but we can't assume all users have been doing that.

You would want something that runs in bulk across all files and will identify the locations you need to change. eslint could do that (and a quick Google doesn't turn up any other tool that would do that, which surprises me but there we go).

[rix0rrr]

I was under the impression that we are just going to hide deprecated APIs, not delete the code. Did I miss anything?

Hmm. You are probably right. Disregard the point about merge conflicts then (but the point that we'll have to remove everything again when we go to actually strip the code remains).

[rix0rrr]

Oh my god of course Python's solution has been overengineered again: https://docs.python.org/3/library/warnings.html

This looks promising, we can probably borrow something from this: https://deprecation.readthedocs.io/en/latest/

[eladb]

If we take the approach of not adding a CDK/jsii-level mechanism to warn about deprecations, we should evaluate the potential developer experience in each language.

I think we want deprecation warnings to be surfaced without requiring that users explicitly run some tool or change the configuration of their project. This implies that either this has built-in support in the language toolchain (Java/.NET/TypeScript*) or that we utilize the CDK CLI somehow to surface those warnings via language-specific tools or through construct warnings (the original approach). *We should decide if IDE-only (TypeScript) is good enough or not. If it eventually turns out that doing more than IDE warnings is very expensive, we might be able to compromise here.

What about JavaScript? Do we want to address it? It is an officially supported CDK language, but we don't know how many people use JavaScript directly.

[NetaNir]

+1 for leveraging existing deprecation mechanism in every target language. It seems like the only language we don't have covered is TypeScript. I don't think we can rely on eslint, we don't even add it as a dependency in the CDK init template.

We should decide if IDE-only (TypeScript) is good enough or not. If it eventually turns out that doing more than IDE warnings is very expensive, we might be able to compromise here.

IDE visual warnings are only helpful for parts of the code that are actively been worked on. The majority of code in a code base is not reviewed in the IDE as a part of a usual development flow (side note: are TS strike-through available in all IDEs? Webstorm/Intellij?).

Since the support in other languages for deprecated API warnings seems to be good enough, I would argue that we should decide if we want to invest in a TypeScript only solution. To clarify, I mean: "If we do build something, it should be TS specific".

A note about a solution that prints API deprecation warnings on every CDK CLI command. If we do add end up adding something like that, we must add a --silence flag. We hear often from users that adding warnings to CDK commands output breaks automation/testings.

[eladb]

I am still not certain if the trivial manual construct annotations might be the most cost/effective approach given our multi-language story.

I am not sure what's the full solution for Python and JavaScript (and TypeScript*), so we need to evaluate this the overall effort and experience in all languages before we determine that simple warnings are "harder".

[rix0rrr]

I would argue that for TypeScript, the combination of:

• IDE hints while working; and
• Running check-typescript-deprecations on-demand for a bulk notification:

$ npx check-typescript-deprecations

/Users/me/Project/bin/app.ts
  9:9  warning  'add' is deprecated. use `Tags.of(scope).add()`  deprecation/deprecation

✖ 1 problem (0 errors, 1 warning)

...are good enough user affordances, no?

---

So the only work left to do seems:

• Double-check the deprecation experience in a couple of popular IDEs (IntelliJ Java, IntelliJ TypeScript, VSCode C#, Visual Studio C# seems good enough coverage?)
• Add a runtime support story for Python

?

[eladb]

Who is going to run the typescript utility? Our CLI?

[rix0rrr]

Who is going to run the typescript utility? Our CLI?

My thought was: the user, if they definitely need to know the answer to their question "am I using deprecated APIs?" definitively and right now, in preparation of an upgrade (or just because they're curious).

To nudge users in the right direction in the course of normal development, I would say that the strikethrough is probably motivation enough.

[eladb]

I tend to agree that IDE strikethroughs are probably good enough as an implicit signal. What about Python?

[rix0rrr]

What about Python?

The only mechanism Python has is to print warnings when the program is run. There is no static analysis so no IDE support/complete list on demand (barring MyPy but it does not have deprecation support).

Experience would be ~ the same as if we added CDK warnings, so this seems like lead for old iron to me.

[nija-at]

Getting language IDEs and tooling to notify of deprecated API usage sounds like a good plan and the right overall direction.

For v2, however, we need something more proactive to nudge users the right way. It needs to be mildly annoying. They should have the option to opt-out but it should opt-in by default (especially for existing CDK apps).
Asking users to set up a new tool is the opposite experience of that - an opt-in experience. This could result in far less users actually moving to the non-deprecated APIs, causing a lot more effort in migrating to v2 and finally resulting in reduced adoption of CDKv2.

If we want to stop supporting v1 and the two branch mode, we need to get more users onto v2.

[skinny85]

Thanks for the comments @eladb @rix0rrr @nija-at @NetaNir!

I've submitted a new revision, where the preferred solution has been switched to changing JSII. I've also greatly expanded the "Alternative solutions" section to synthesize (hopefully) the discussion we've been having.

Please comment, and let me know what you think!

[rix0rrr]

What does that mean?

The default behavior of the application will be ignore, and the CLI will in turn flip that to warn by default unless ignored?

Motivation: unit tests etc I suppose?

[skinny85]

It means the code adding the warnings will not be added to the emitted JavaScript by default at all.

[rix0rrr]

Not actually true I don't think.

We can turn:

class Foo {
  /** @deprecated */
  public static readonly CONSTANT = 5;
}
// into
class Foo {
  public static get CONSTANT(): number {
    console.error('[WARNING] oh no');
    return 5;
  }
}

And something similar for enums.

Honestly, I feel like something like:

// Would work fine and would work for enums too
Foo.prototype.CONSTANT = deprecatedAccessor(Foo.prototype.CONSTANT, 'Use Foo.Bloop instead');

[skinny85]

Fair enough. That's a huge intrusion to the generated code though. I've re-phrased the downside to reflect that.

[rix0rrr]

I don't find this convincing, and I'm not convinced that printing to stderr is less likely to be missed.

We're probably talking about different contingents of programmers here, and printing to stderr is optimizing more for developers "like us".

[skinny85]

I mean, we're showing the warnings in the IDE anyway - we're not taking this away. So if you don't agree they will be missed, then I guess you're in favor of a different option than the JSII one, no?

[rix0rrr]

3. Rewrite all code to stop using deprecated APIs (estimated effort: 2 months)

[skinny85]

I don't think it will take that long... see aws/aws-cdk#13500.

[rix0rrr]

Hah! :D

[nija-at]

Not sure I follow this sentence. Can you re-state?

[skinny85]

Re-worded. Let me know if this is clear now.

[nija-at]

add an API for turning that option off for those tests

Not following this. Add an API where? can you expand?

[nija-at]

We should discuss this f2f.

I'm working on a similar area and have encountered unforeseen issues, and has generally been harder than I thought it will be. It's because of the different ways we deprecated elements.

Maybe you'll have the same problems, maybe not. Worth discussing.

[skinny85]

I meant add the API to our testing libraries. Similarly like you added an API for tests using feature flags.

Sure, I'd love to discuss this F2F.

[skinny85]

@rix0rrr @nija-at included all of your comments, thanks! Would appreciate if you could take another pass.

[eladb]

Can we describe when this will not work? For example, what does it mean to "use" an interface in this context?

[skinny85]

Since that depends on the exact solution chosen, I've omitted it from this part.

Do we need to get into such details in the user-facing documentation?

[eladb]

This document should propose a solution and the "working backwards" section should be written as if this the proposed solution is the one we chose. If we end up changing the solution for whatever reason, we will need to evaluate the implications on the user experience and update this section (which is a good thing :-)).

[skinny85]

OK. Added. Let me know if this works.

[skinny85]

@eladb thanks for the review! Included all of your comments, would appreciate another pass.

[eladb]

Great write up

Pull request has been modified.

[eladb]

🥇

[eladb]

Added do-not-merge for final comments period.