[Fix #9004] Improve error message for extending interface#9180
Conversation
|
Hi @HerringtonDarkholme, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
|
|
||
| function checkAndReportErrorForExtendingInterface(errorLocation: Node, name: string): boolean { | ||
| if (!errorLocation || errorLocation.kind !== SyntaxKind.Identifier || | ||
| !errorLocation.parent || !errorLocation.parent.parent || |
There was a problem hiding this comment.
it could be a PropeortyAccess, so use getAncestor instead.
There was a problem hiding this comment.
also add a test for the dotted name case.
There was a problem hiding this comment.
The intention here is that if one wrongly extends interface, then the errorLocation must be exactly at HeritageClause -> ExpressionWithTypeArguments -> Identifier.
There was a problem hiding this comment.
consider this:
namespace N {
export interface I { }
}
class C extends N.I {
}you still want to show the error on I. the node you get is HeritageClause -> ExpressionWithTypeArguments -> PropertyAccessExpression -> Identifier
There was a problem hiding this comment.
Current approach will not report extends interface error for clauses like class Child extends Module.Interface
I know this. So should I support propertyAccess in this pull request too?
There was a problem hiding this comment.
For such cases
namespace N {
export interface I {}
}
class C extends N.I {}resolveEntityName does not work here because it requires a ExpressionWithTypeArguments node, while N.I is binded as a expression.
Do you have a suggestion?
There was a problem hiding this comment.
Oh, I didn't understand your comments until now!
I know how to do it.
| } | ||
| if (errorLocation.kind === SyntaxKind.Identifier) { | ||
| const name = (<Identifier>errorLocation).text; | ||
| const interfaceOrModule = resolveName( |
There was a problem hiding this comment.
Sorry wrong recommendation earlier. you probably should use resolveEntityName it handles both identifiers and propeortyAccess.
|
|
||
| function checkAndReportErrorForExtendingInterface(errorLocation: Node): boolean { | ||
| let parentClassExpression = errorLocation; | ||
| while (parentClassExpression) { |
There was a problem hiding this comment.
getAncestor cannot be used here because it will confuse with class A extends generateClass(Interface) {}.
See the test case classExtendsInterfaceInExpression
|
thanks! |
|
Thanks for your helpful advice! |
Fixes #9004
I found it had more to do with checker than diagnostics.json. We have to first check whether it is a valid base class before reporting
cannot extending an interface. So I put the error reporting inresolveNameandcheckPropertyAccessExpressionOrQualifiedName, likecheckAndReportErrorForMissingPrefix.New baseline is accepted, and no new tests are added because current testcases are enough for this change.