fix(46246): Extending Class/Interface With a Primitive Throws a More Concise Error Report #49208

maross3 · 2022-05-22T19:32:32Z

Example

Class:

class foo extends number {}

Error:
A class cannot extend a primitive type like 'number'. A class can only extend named object types.

Interface:

interface bar extends number { }

Error:
An interface cannot extend a primitive type like 'number'. An interface can only extend named object types.

Notes:

gulp runtests passed on current branch.
I updated a few unit tests to test for the error. I did not feel new test cases were necessary, as the errors were well covered.
Function responsible for the ambiguous error: checkAndReportErrorForUsingTypeAsValue
Function to check heritage clause: checkAndReportErrorForExtendingPrimitiveType

Description

The approach was to check for extension of primitives directly before the ambiguous error message was thrown. This lead to making a new function.

I didn't add the code to the function checkAndReportErrorForUsingTypeAsValue because the intended error checking functionality was separate from evaluating the value for error checking.

The new function checkAndReportErrorForExtendingPrimitiveType now handles checking heritage clause related errors in regards to classes extending primitives.

In the code, we check if the current errorLocation's expected heritage clause was, in fact, a SyntaxKind.HeritageClause. A bitmask was used to check against the expected heritage clause: errorLocation.parent.parent.kind. If it's a primitive name and a heritage clause, we throw a more descriptive error.

This was my first contribution to any project of any size, so I would appreciate any feedback!

ghost · 2022-05-22T19:32:44Z

All CLA requirements met.

typescript-bot · 2022-05-22T20:45:35Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

MartinJohns · 2022-05-23T12:14:47Z

An interface can only extend named types and classes.

Unless that named type is a union type or a mapped type.

Or as error 2312 frames it:

An interface can only extend an object type or intersection of object types with statically known members.

maross3 · 2022-05-23T17:16:59Z

An interface can only extend named types and classes.

Hey, thanks for the info.

I accidentally made a bonus descriptive error for classes extending primitives:

class foo extends number {} // An interface (SHOULD BE CLASS) cannot extend a primitive type like 'number'; An interface can only extend named types and classes.

Couple quick questions:

Should we keep this error message and use {1} to state whether class or interface?
Should we call error with error 2312 as the second parameter instead?
Should we make a new error or remove the error for classes?

tests/baselines/reference/classExtendingPrimitive.errors.txt

src/compiler/diagnosticMessages.json

MartinJohns · 2022-05-23T19:45:40Z

Couple quick questions:

No idea. I'm no member of the TypeScript team. I just saw the new error message you added and wanted to point that it's incorrect (or at least not always correct).

…ypeScript into Extend-Primitive-Error Merging json

maross3 · 2022-05-23T21:45:55Z

Thank you,

Added the logic to distinguish between the interface and class errors.
Added additional error to keep consistent with other functions/error messages.
Fixed the typos

Added <repo-root>/ to example usage

maross3 · 2022-05-25T15:33:56Z

I'm following up on the review request, thank you @DanielRosenwasser

sandersn · 2022-05-31T23:51:32Z

#46357 fixed #46246 earlier, although we dropped it until now. I merged that PR, but this PR also adds an error message on classes, which is pretty useful. If you want to continue on this PR, you could add the class error to the code added by #46357.

sandersn · 2022-05-31T23:52:06Z

README.md

@@ -95,7 +95,7 @@ gulp help              # List the above commands.
 ## Usage

 ```bash
-node built/local/tsc.js hello.ts
+node <repo-root>/built/local/tsc.js hello.ts


can you revert the changes here and in package.json? They're not related to the fix.

Sorry, will do.

sandersn · 2022-05-31T23:54:00Z

src/compiler/diagnosticMessages.json

+        "category": "Error",
+        "code": 18042
+    },
+    "A class cannot extend a primitive type like '{0}'. A class can only extend named object types.": {


While you're adding class-specific errors, a separate error for when classes implement primitive types would be nice.

sandersn · 2022-05-31T23:54:22Z

src/compiler/checker.ts

+                    else if(meaning & SymbolFlags.Class) {
+                        error(errorLocation, Diagnostics.A_class_cannot_extend_a_primitive_type_like_0_A_class_can_only_extend_named_object_types, rawName);
+                    }
+                     return true;


I don't think you should return true unless one of the above errors was issued.

Line 2503 if (isPrimitiveTypeName(name) && !(errorLocation.parent.parent.kind & ~SyntaxKind.HeritageClause))

This is our error check. If we are in this scope, there is an error, so we only need one return statement. The if/else checks if it's a class or interface and throws a respective error. After the error is called, we return true. Would you like to see the return within the inner scopes above, or is this okay? (It does need a default error, though)

sandersn · 2022-05-31T23:55:27Z

src/compiler/checker.ts

+                    if(meaning & SymbolFlags.Interface) {
+                        error(errorLocation, Diagnostics.An_interface_cannot_extend_a_primitive_type_like_0_An_interface_can_only_extend_named_object_types, rawName);
+                    }
+                    else if(meaning & SymbolFlags.Class) {


A syntactic check (of errorLocation.parent.parent.parent I think) is better. meaning is the kind of symbol requested by name resolution.

Also you should probably check that all those parent nodes are defined.

maross3 · 2022-06-03T13:33:26Z

I'll be picking this back up within the week. I would like to continue working on this and add the additional class error. Thank you for the feedback!

sandersn · 2022-08-01T23:29:08Z

@maross3 Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs.

maross3 added 2 commits May 22, 2022 07:24

Check and report extending primitive type, new diagnostic message

db2269c

added test cases, checked if heritage clause

ba102bc

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 22, 2022

maross3 mentioned this pull request May 22, 2022

Unhelpful error message when interface ... extends number #46246

Closed

maross3 added 4 commits May 22, 2022 15:57

Accidental push

d8a7755

Accidentally pushed v2

ccc6aa1

Create package-lock.json

2f7204e

Create package.json

9a7584e

maross3 marked this pull request as ready for review May 22, 2022 20:45

typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 22, 2022

maross3 changed the title ~~fix(46246): Extending a Primitive Throws a More Concise Error Report~~ fix(46246): Extending Class With a Primitive Throws a More Concise Error Report May 23, 2022

maross3 changed the title ~~fix(46246): Extending Class With a Primitive Throws a More Concise Error Report~~ fix(46246): Extending Class/Interface With a Primitive Throws a More Concise Error Report May 23, 2022

DanielRosenwasser requested changes May 23, 2022

View reviewed changes

tests/baselines/reference/classExtendingPrimitive.errors.txt Outdated Show resolved Hide resolved

src/compiler/diagnosticMessages.json Outdated Show resolved Hide resolved

maross3 added 2 commits May 23, 2022 16:46

Change Request Resolution

301cf68

Merge branch 'Extend-Primitive-Error' of https://github.com/maross3/T…

46c864f

…ypeScript into Extend-Primitive-Error Merging json

maross3 requested a review from DanielRosenwasser May 23, 2022 21:41

Fix typo

b27a7bf

Added <repo-root>/ to example usage

sandersn self-requested a review May 31, 2022 23:51

sandersn assigned DanielRosenwasser May 31, 2022

sandersn requested changes May 31, 2022

View reviewed changes

maross3 closed this Oct 2, 2022

This was referenced Sep 8, 2023

add conditions to check if the class would have extend or implement p… qwefgh90/TypeScript#2

Closed

Add conditions to check for class extension or implementation of prim… #55712

Merged

fix(46246): Extending Class/Interface With a Primitive Throws a More Concise Error Report #49208

fix(46246): Extending Class/Interface With a Primitive Throws a More Concise Error Report #49208

Uh oh!

Conversation

maross3 commented May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Notes:

Description

This was my first contribution to any project of any size, so I would appreciate any feedback!

Uh oh!

ghost commented May 22, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented May 22, 2022

Uh oh!

MartinJohns commented May 23, 2022

Uh oh!

maross3 commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinJohns commented May 23, 2022

Uh oh!

maross3 commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maross3 commented May 25, 2022

Uh oh!

sandersn commented May 31, 2022

Uh oh!

sandersn May 31, 2022

Choose a reason for hiding this comment

Uh oh!

maross3 Jun 3, 2022

Choose a reason for hiding this comment

Uh oh!

sandersn May 31, 2022

Choose a reason for hiding this comment

Uh oh!

sandersn May 31, 2022

Choose a reason for hiding this comment

Uh oh!

maross3 Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn May 31, 2022

Choose a reason for hiding this comment

Uh oh!

maross3 commented Jun 3, 2022

Uh oh!

sandersn commented Aug 1, 2022

Uh oh!

Uh oh!

maross3 commented May 22, 2022 •

edited

Loading

ghost commented May 22, 2022 •

edited by ghost

Loading

maross3 commented May 23, 2022 •

edited

Loading

maross3 commented May 23, 2022 •

edited

Loading

maross3 Jun 3, 2022 •

edited

Loading