-
Notifications
You must be signed in to change notification settings - Fork 12.9k
--noImplicitOverride #39669
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
--noImplicitOverride #39669
Changes from 1 commit
8b90b8d
ea5608a
9371d87
6cb7f5e
061cd27
37ada55
6401e1a
be00363
fbb6e28
6014a3a
a65df91
43d9f70
e73c941
c0edf6f
dca93ac
2fd560a
94187c7
2b57fbd
3a2286b
d34afb9
b4614d1
1ef034d
aebfb28
7753983
5c04ccc
add16c9
5316e2f
f36fede
811bd1e
d0f5095
dd51815
22bb09d
eee3b77
f94e93c
74f14c3
e35ee17
d8fda5f
18434fd
5ab5a7f
f52822a
48c23a5
a3f0443
cdd0739
8e53ad3
827bc95
618f883
bc4d758
47296d6
5d19a90
68e1440
82109e9
2552495
cf5dc7e
b65d244
84cc69d
6d89a7a
062f149
feb9093
0c09a2e
3cf3c28
d874ae8
e361ddc
40da344
f9384bc
eea8259
ff3cbc1
a3766d2
e2c245e
8f3dcb9
1db64fb
71c00d5
e3c0993
89517a0
6c8d00d
bbfb7eb
4575b5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
tests/cases/conformance/override/override2.ts(12,14): error TS4113: Method must have override modifier because it's override the base class 'AB'. | ||
tests/cases/conformance/override/override2.ts(17,14): error TS4113: Method must have override modifier because it's override the base class 'AB'. | ||
tests/cases/conformance/override/override2.ts(12,14): error TS4113: Class member must have override modifier because it's override the base class 'AB'. | ||
tests/cases/conformance/override/override2.ts(17,14): error TS4113: Class member must have override modifier because it's override the base class 'AB'. | ||
|
||
|
||
==== tests/cases/conformance/override/override2.ts (2 errors) ==== | ||
|
@@ -16,14 +16,14 @@ tests/cases/conformance/override/override2.ts(17,14): error TS4113: Method must | |
abstract class AD2 extends AB { | ||
abstract foo(v: ''): void // need override? | ||
~~~ | ||
!!! error TS4113: Method must have override modifier because it's override the base class 'AB'. | ||
!!! error TS4113: Class member must have override modifier because it's override the base class 'AB'. | ||
} | ||
|
||
abstract class AD3 extends AB { | ||
override foo(v: ''): void { } // need override? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, sorry if I missed earlier discussion on this, but I don’t think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we raise an error if use override with implement abstract method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what would make sense to me, but I don’t feel too strongly about it—@DanielRosenwasser what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess if you rename in the base class, you won't run into issues. Still seems surprising. One thing to be sure of is that if you are re-declaring an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is what I’d expect: abstract class Base {
abstract foo(): unknown;
abstract bar(): void;
}
// No errors:
class Sub extends Base {
override abstract foo(): number;
bar() {
return;
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @andrewbranch I switched your comment to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that’s less sketchy, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mprobst I think that the current message there is
is there something I'm missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If users were able to put
I'm arguing that it's useful to get the second error message, to more quickly determine what went wrong. There's also the scenario where you refactor such that an interface you implement no longer needs you to implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mprobst seems like something somewhat similar to weak type checks - but what would that specific check be? When do you know that some method needs to be overridden? When everything is protected or private? |
||
abstract bar(): void; | ||
~~~ | ||
!!! error TS4113: Method must have override modifier because it's override the base class 'AB'. | ||
!!! error TS4113: Class member must have override modifier because it's override the base class 'AB'. | ||
} | ||
|
||
class D4 extends AB { | ||
|
Uh oh!
There was an error while loading. Please reload this page.