(ts) Raise syntax error for an abstract method that has body #12687
Conversation
sosukesuzuki
commented
Jan 24, 2021
•
sosukesuzuki
commented
| Q | A |
|---|---|
| Fixed Issues? | Fixes #12682 |
| Patch: Bug Fix? | Y |
| Tests Added + Pass? | Yes |
| License | MIT |
sosukesuzuki
added
area: errors
area: typescript
PR: Bug Fix 🐛
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/43259/ |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f6067d8:
|
| node: T, | ||
| isGenerator: boolean, | ||
| isAsync: boolean, | ||
| isConstructor: boolean, | ||
| allowDirectSuper: boolean, | ||
| type: string, | ||
| inClassScope: boolean = false, |
There was a problem hiding this comment.
nit: We can just use ...args if none of them are used in the post processing logic. So we would not forget to sync signatures when we add even more arguments to parseMethod.
There was a problem hiding this comment.
Use of ...args is one of the slowest alternatives. Just mention it in case perf is relevant here
There was a problem hiding this comment.
There are already some ...args in our codebase. I don't know if this is a performance bottleneck, but at least I don't think we need to worry in this PR.
There was a problem hiding this comment.
In general. Everything that is a perf regression should be improved, but this is just a TS plugin so maybe no point because too much perf issues in this plugin.
| // For estree plugin | ||
| method.type === "MethodDefinition" |
There was a problem hiding this comment.
We can do this.hasPlugin("estree") in this case, so that it's self-explanatory.
|
Use of 7 properties like that consumes lots of memory. Each property takes 3 bytes each. So 21 bytes total. Plus the size of the object will give e perf regression in V8 etc. '...' operator is slower than simply iterate through and push to an array (search on google). Direct string comparison is also slow. Just mention in case perf and memory is a concern. |
| const method = super.parseMethod(...args); | ||
| if ((method: any).abstract) { | ||
| const hasBody = this.hasPlugin("estree") | ||
| ? !!method.value.body |
| this.raise( | ||
| method.start, | ||
| TSErrors.AbstractMethodHasImplementation, | ||
| (method: any).key.name, |
There was a problem hiding this comment.
The method key is not always Identifier, and that's why Flow complains here.
abstract class C {
abstract [M]();
}I think we can just throw
An abstract method cannot have an implementation.
There was a problem hiding this comment.
Is this correct error message to be used here? For example TS never throws on
abstract class C { abstract a(): string; }, but throws on abstract class C { abstract a(); } because it's lack return type.
Regarding the example above. TS throws
A computed property name in a method overload must refer to an expression whose type is a literal type or a 'unique symbol' type.ts(1168
'[M]', which lacks return-type annotation, implicitly has an 'any' return type.ts(7010)
There was a problem hiding this comment.
If you disable the (opt-in, but enabled by default in the playground) noImplicitAny option, that code doesn't generate any error but just a lint suggestion: https://www.typescriptlang.org/play?noImplicitAny=false#code/MYewdgzgLgBAsjAvDARAawKYE8UwIYQyiRQDcAsAFBV4BG0ATnsLMADYGEDCMA3lTHz0oTFjADacALoAKAJQVKAXypA
There was a problem hiding this comment.
Sorry for the confusion here, I was meant to say that an abstract method can be computed, so that example is valid. And the TS error can be suppressed by specifying M. To be clear
abstract class C {
abstract [M]() {}
}will throw.
I don't think we have to extract computed key here, since it can be very verbose.
There was a problem hiding this comment.
I don't think we have to extract computed key here, since it can be very verbose.
I ended up doing it since I noticed it's exactly what TS does.
nicolo-ribaudo
force-pushed
the
fix-12682
branch
from
c448307
to
f6067d8
Compare
github-actions
bot
added
the
outdated
