Skip to content

Use related spans for "implement abstract class" errors #48030

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 28, 2023
Merged

Use related spans for "implement abstract class" errors #48030

merged 4 commits into from
Mar 28, 2023

Conversation

FlyingPumba
Copy link
Contributor

@gabritto Here is the PR again that was reverted. I didn't change the message. I don't actually remember how to do it correctly (it's a PR from one year ago after all 😅).

Old PR: #42546
Fixes #32848

Example compiling the following code.

abstract class A {
	abstract b(): void;
	abstract c(): void;
}

class B extends A { }

Error message before:

hello.ts:6:7 - error TS2515: Non-abstract class 'B' does not implement inherited abstract member 'b' from class 'A'.

6 class B extends A { }
~

hello.ts:6:7 - error TS2515: Non-abstract class 'B' does not implement inherited abstract member 'c' from class 'A'.

6 class B extends A { }
~

Found 2 errors.

Error message after:

hello.ts:6:7 - error TS18036: Non-abstract class 'B' does not implement all abstract members of 'A'

6 class B extends A { }
~

hello.ts:6:7
6 class B extends A { }
~
Non-abstract class 'B' does not implement inherited abstract member 'b' from class 'A'.
hello.ts:6:7
6 class B extends A { }
~
Non-abstract class 'B' does not implement inherited abstract member 'c' from class 'A'.

Found 1 error.

cc: @DanielRosenwasser

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Feb 25, 2022
@FlyingPumba FlyingPumba mentioned this pull request Feb 25, 2022
Merged
@gabritto
Copy link
Member

thanks a lot, I'll try not to mess things up with git this time

@gabritto gabritto self-assigned this Feb 25, 2022
sandersn
sandersn requested changes Mar 27, 2023
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks reasonable. Just a couple of minor questions.

It's been a really long time since we last reviewed this, so if you're not interested in bringing this PR up-to-date, I understand.

src/compiler/diagnosticMessages.json Outdated
@@ -7285,5 +7285,9 @@
"A 'return' statement cannot be used inside a class static block.": {
"category": "Error",
"code": 18041
},
"Non-abstract class '{0}' does not implement all abstract members of '{1}'": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Non-abstract class '{0}' does not implement all abstract members of '{1}'": {
"Non-abstract class '{0}' does not implement all inherited abstract members of '{1}'": {

src/services/codefixes/fixClassDoesntImplementInheritedAbstractMember.ts Outdated
Comment on lines 5 to 6
Diagnostics.Non_abstract_class_0_does_not_implement_inherited_abstract_member_1_from_class_2.code,
Diagnostics.Non_abstract_class_expression_does_not_implement_inherited_abstract_member_0_from_class_1.code,
Copy link
Member

@sandersn sandersn Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these needed anymore now that they're related spans? Is it even possible to fix just one missing member in the first place?

Edit: It's not, although I don't know how related spans display, so it's worth finding out whether the codefix can be invoked from a related span.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, of course. The related spans are not really related spans, just elaborations. So these are definitely not needed.

sandersn added 2 commits March 28, 2023 06:25
@sandersn
Merge branch 'main' into related-spans-for-implement-abstract-class-e…
458963d 
…rror
@sandersn
Few small fixes
768c917 
1. Lazily create top-level error instead of saving related errors in a
list. addRelatedInfo already creates a list.
2. Remove unused errors in codefix trigger.
3. Related span points to base properties instead.
sandersn
sandersn reviewed Mar 28, 2023
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional things I noticed. I fixed them after I got this PR stuck in my head overnight.

src/services/codefixes/fixClassDoesntImplementInheritedAbstractMember.ts Outdated
Comment on lines 5 to 6
Diagnostics.Non_abstract_class_0_does_not_implement_inherited_abstract_member_1_from_class_2.code,
Diagnostics.Non_abstract_class_expression_does_not_implement_inherited_abstract_member_0_from_class_1.code,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, of course. The related spans are not really related spans, just elaborations. So these are definitely not needed.

src/compiler/checker.ts Outdated
@@ -39533,12 +39534,16 @@ namespace ts {
}

if (derivedClassDecl.kind === SyntaxKind.ClassExpression) {
error(derivedClassDecl, Diagnostics.Non_abstract_class_expression_does_not_implement_inherited_abstract_member_0_from_class_1,
const err = createDiagnosticForNode(derivedClassDecl,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use the base node for the related span

src/compiler/checker.ts Outdated
@@ -39494,6 +39494,8 @@ namespace ts {

// NOTE: assignability is checked in checkClassDeclaration
const baseProperties = getPropertiesOfType(baseType);
const derivedClassDecl = getClassLikeDeclarationOfSymbol(type.symbol)!;
const inheritedAbstractMemberNotImplementedErrors: Diagnostic[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of saving a list, create an error lazily and push onto that. It already has a list internally.

@sandersn sandersn merged commit 2b57a88 into microsoft:main Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn approved these changes

@gabritto gabritto Awaiting requested review from gabritto

@armanio123 armanio123 Awaiting requested review from armanio123

@jakebailey jakebailey Awaiting requested review from jakebailey

Assignees

@gabritto gabritto

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use related error spans for "implement abstract class" errors
4 participants
@FlyingPumba @gabritto @sandersn @typescript-bot