Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codefix for adding type annotations to method parameters (and return type?) from base class/interface declarations #46297

Open
5 tasks done
justingrant opened this issue Oct 10, 2021 · 4 comments
Labels
Domain: Quick Fixes Editor-provided fixes, often called code actions. In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@justingrant
Copy link
Contributor

justingrant commented Oct 10, 2021

Suggestion

When a class implements an interface or extends a base class (esp. a class type defined in the package's .d.ts), then there should be an easy way for IDE users to add type annotations to parameter types and return types for class methods that exist on the base type(s). Note that this is different (although related) to #23911 which is about inference. I'm talking here about enabling opt-in IDE actions, not automatic inference. An opt-in solution may avoid some of the edge cases and technical pitfalls that have prevented #23911 from being successful so far.

🔍 Search Terms

infer from usage
parameter types
return types
implements
base class

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Currently, "infer from usage" for function parameters (does it infer return types too?) seems to ignore the type of that method in interfaces that are implemented by that class, or in base classes. It shouldn't! The base class or interface seems like it should be the most reliable indicator of "usage" and should either override or augment whatever type info TS can gather from usage.

Both parameter types and return types should be fetched from the base interface or class.

I know that there's work on inference (#23911 and elsewhere), but I'm asking for something simpler: an opt-in way for IDE users to add type annotations to their source code using the base class or interface type(s).

What makes #23911 hard are all the weird cases, like multiple inheritance, overloads, derived classes that intentionally vary signatures from its parent, etc.

But using an opt-in, IDE-centric solution could sidestep many of those annoying corner cases by simply bailing out when those edge cases happen, or providing a "best effort" output that the user can reject with Ctrl-Z/CMD+Z if they don't like it.

I'm not sure whether this feature should be a refactoring, part of "infer from usage", or both.

It'd be ideal if there were a way to do it for one method and also to do it for all methods in a type and/or in a file, because often when you want to fill in one method's parameter/return types, then you also need to fill in all of them!

Note that any support should include both base classes and base interfaces.

📃 Motivating Example

We are in the middle of porting the Stage 3 Temporal proposal polyfill to TS. It was painful to add types to hundreds of class methods that implemented abstract class types defined in our .d.ts. This seemed like completely unnecessary manual gruntwork that could and should have been automated.

I also committed bugs in the process. For example, I accidentally omitted a = undefined default value on some parameters due to a mistake in the search-n-replace regex I was using to add parameter types.

I suspect that these are common problems any time a JS codebase is migrated to TS.

If there were an automated way to overlay the base class types on an existing implementation, it would have saved our team ~20 hours.

💻 Use Cases

The main use case I'm familiar with is porting a JS codebase to TS. Nowadays most JS packages have an existing .d.ts that defines the public API, so it'd be great to be able to use that to jumpstart the migration of the implementation to TS.

The best workaround I've found so far is to create some generic types (e.g. PlainDateParams<T extends keyof PlainDate> and PlainDateReturn<T extends keyof PlainDate) that define parameters and return types of class methods (including statics and constructors), and then use regex search-and-replace to look for method parameters that lack types, and use $1, $2, etc. to add XxxParams and XxxReturn annotations to them. This is better than completely manual work, but it's still time-consuming and error prone (e.g. when there are inline comments, default values, line breaks, etc.)

@andrewbranch
Copy link
Member

I think this should be mostly solved by #45670. Thoughts?

@andrewbranch
Copy link
Member

I would also be open to a codefix for this. I don’t think it belongs in “infer from usage” as the source isn’t “usage” per se, but the source is actually better than usage, and it would probably be an easy codefix to write.

@andrewbranch andrewbranch changed the title "Infer from usage" (or a refactoring?) for class methods should use implements and extends type info Codefix for adding type annotations to method parameters (and return type?) from base class/interface declarations Oct 12, 2021
@andrewbranch andrewbranch added In Discussion Not yet reached consensus Suggestion An idea for TypeScript Domain: Quick Fixes Editor-provided fixes, often called code actions. labels Oct 12, 2021
@justingrant
Copy link
Contributor Author

justingrant commented Oct 13, 2021

I think this should be mostly solved by #45670. Thoughts?

I see there being 4 use cases:

  1. Migrating existing JS projects that have a .d.ts defined in @types or in the project) to TS
  2. Implementing a new class that implements an existing interface (not sure if this also applies to extending a declare class declaration)
  3. Implementing a new derived class that extends a base class implementation (not declaration). The difference from (2) is that with (2) you can copy/paste the declaration into your code and you'll have all the types you need, while with (3) you'd need to generate a .d.ts from the existing implementation before you can copy/paste. That's harder.
  4. Adapting to changes in a base class or interface, e.g. base interface adds a new method or changes a parameter type

In my experience, (2) is really easy: just copy the code from .d.ts and replace the semicolons with implementations.

(3) requires generating a .d.ts which is sometimes easy, sometimes hard depending on your build config and your familiarity with TS tools. But (3) also seems relatively rare, at least in my naive view where deep inheritance hierarchies aren't popular in TS (as opposed to Java, C#, etc.) This may be a wrong assumption!

(4) is usually easy because typically there's only a few changes, not a wholesale refactor. So manually adding types until red squgglies go away is usually fine.

(1), however, is a big pain because you need to interleave the existing JS (param names and default values) with TS types. This is easy to screw this up and is very time-consuming on classes with many members.

Another challenge with (1) is a chicken-and-egg problem. If you enable noImplicitAny then your project can't compile and you can't run tests to validate your changes to one file before adding types to the next file. But if you leave noImplicitAny disabled (as you probably must when you're just starting to port a new JS project to TS) then you won't get any red squigglies telling you about parameters that need types. So you end up having to enable noImplicitAny, fix one file or part of one file, then turn it off and run tests, then rinse and repeat. This is annoying. (Although maybe I'm doing it wrong!) It'd be great if there were an easier way to add parameter and return types without needing to enable noImplicitAny first.

Anyway, if I were to prioritize investment, I'd probably want to focus on (1) because it's a very common use case that's also very difficult today. Making the others easier too is great, but lower priority IMHO.

If I'm understanding #45670 correctly, it won't help with (1), correct? Because it relies on adding a new method implementation as opposed to fixing an existing one?

Assuming I am understanding it correctly, then yes I agree that a codefix would be great. Seems like the only good solution for (1).

I would also be open to a codefix for this. I don’t think it belongs in “infer from usage” as the source isn’t “usage” per se, but the source is actually better than usage, and it would probably be an easy codefix to write.

Sounds great! (Assuming I'm correct above.) I'm not able to PR this but would be very happy to see this happen!

Would there be a "fix all methods in this class" option? A "fix all types in this file" option? Or would it be just one method at a time?

How would it behave if there were overloaded methods in the base type(s)? Are there other "gotcha" cases where the codefix may not work or may produce an unexpected result?

Would it work for property types as well as methods? What about getters/setters? Constructors too? Statics?

@andrewbranch
Copy link
Member

Usually a fix-all applies to the whole file. Note that a codefix would also require noImplicitAny, so I’m not sure if there’s a great solution to the problem of having to fix one file at a time.

Overloads are tricky. I’m not sure what the right behavior would be there.

mnvr added a commit to mnvr/mrmr.io that referenced this issue Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Quick Fixes Editor-provided fixes, often called code actions. In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

2 participants