8000 --noImplicitOverride by Kingwl · Pull Request #39669 · microsoft/TypeScript · GitHub
[go: up one dir, main page]

Skip to content

--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

Merged
merged 76 commits into from
Mar 26, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
8b90b8d
wip: add types
Kingwl Jul 15, 2020
ea5608a
wip
Kingwl Jul 17, 2020
9371d87
Add cases
Kingwl Jul 17, 2020
6cb7f5e
Add some case
Kingwl Jul 17, 2020
061cd27
Add more check
Kingwl Jul 20, 2020
37ada55
accept baseline
Kingwl Jul 20, 2020
6401e1a
add abstract abd declare method
Kingwl Jul 20, 2020
be00363
add override in declaration
Kingwl Jul 20, 2020
fbb6e28
accept baseline
Kingwl Jul 20, 2020
6014a3a
add property override
8000 Kingwl Jul 21, 2020
a65df91
Fix decalre modifier
Kingwl Jul 21, 2020
43d9f70
update baseline
Kingwl Jul 21, 2020
e73c941
Add more cases
Kingwl Jul 21, 2020
c0edf6f
make lint happy
Kingwl Jul 21, 2020
dca93ac
make lint happy
Kingwl Jul 21, 2020
2fd560a
Update description
Kingwl Jul 21, 2020
94187c7
Add codefix
Kingwl Aug 19, 2020
2b57fbd
simplify code
Kingwl Aug 19, 2020
3a2286b
Merge branch 'master' into exprement-override
Kingwl Aug 19, 2020
d34afb9
accept baseline
Kingwl Aug 19, 2020
b4614d1
Update desc
Kingwl Aug 21, 2020
1ef034d
Accept baseline
Kingwl Aug 21, 2020
aebfb28
Add override completions
Kingwl Aug 24, 2020
7753983
filter out implements field in override context
Kingwl Aug 24, 2020
5c04ccc
fix tests
Kingwl Aug 24, 2020
add16c9
Add parameter property check
Kingwl Aug 24, 2020
5316e2f
Accept baseline
Kingwl Aug 24, 2020
f36fede
acept baseline
Kingwl Aug 24, 2020
811bd1e
Add parameter property to declaration code action
Kingwl Aug 25, 2020
d0f5095
Add quickfix for override parameter property
Kingwl Aug 25, 2020
dd51815
Merge branch 'master' into exprement-override
Kingwl Sep 1, 2020
22bb09d
fix code style
Kingwl Sep 3, 2020
eee3b77
Add override with interface tests
Kingwl Sep 3, 2020
f94e93c
Add more cases about modifier position
Kingwl Sep 3, 2020
74f14c3
Merge branch 'master' into exprement-override
Kingwl Sep 14, 2020
e35ee17
rename flag
Kingwl Sep 14, 2020
d8fda5f
rename flags
Kingwl Sep 14, 2020
18434fd
Added tests.
DanielRosenwasser Sep 14, 2020
5ab5a7f
Accepted baselines.
DanielRosenwasser Sep 15, 2020
f52822a
Always issue errors for unnecessary 'override' modifiers.
DanielRosenwasser Sep 15, 2020
48c23a5
Accepted baselines.
DanielRosenwasser Sep 15, 2020
a3f0443
Merge branch 'master' into exprement-override
Kingwl Sep 15, 2020
cdd0739
Override perf (#4)
Kingwl Sep 15, 2020
8e53ad3
Merge branch 'master' into exprement-override
Kingwl Sep 16, 2020
827bc95
Do not issue error if implement abstract
Kingwl Sep 16, 2020
618f883
Add abstract-spec check
Kingwl Sep 16, 2020
bc4d758
Avoid override dead lock
Kingwl Sep 16, 2020
47296d6
Add more case
Kingwl Sep 16, 2020
5d19a90
Add codefix for new error
Kingwl Sep 16, 2020
68e1440
Fix error message
Kingwl Sep 16, 2020
82109e9
Merge branch 'master' into exprement-override
Kingwl Oct 30, 2020
2552495
Add jsdoc override tag (#6)
Kingwl Nov 2, 2020
8000
cf5dc7e
Override jsdoc tag (#7)
Kingwl Nov 2, 2020
b65d244
Disallow codefix in js
Kingwl Nov 2, 2020
84cc69d
Merge branch 'master' into exprement-override
Kingwl Nov 5, 2020
6d89a7a
update baseline
Kingwl Nov 5, 2020
062f149
update baseline
Kingwl Nov 5, 2020
feb9093
Omit override in d.ts
Kingwl Nov 5, 2020
0c09a2e
Add more case in js
Kingwl Nov 5, 2020
3cf3c28
Accept baseline
Kingwl Nov 5, 2020
d874ae8
fix override js test
Kingwl Nov 6, 2020
e361ddc
Merge branch 'master' into exprement-override
Kingwl Dec 31, 2020
40da344
Merge branch 'master' into exprement-override
Kingwl Jan 7, 2021
f9384bc
fix crlf
Kingwl Jan 7, 2021
eea8259
Revert merge conflict changes
Kingwl Jan 7, 2021
ff3cbc1
Accept baseline
Kingwl Jan 7, 2021
a3766d2
Avoid space
Kingwl Jan 7, 2021
e2c245e
Merge branch 'master' into exprement-override
Kingwl Feb 4, 2021
8f3dcb9
Merge branch 'master' into exprement-override
Kingwl Mar 5, 2021
1db64fb
Fix CR issues
Kingwl Mar 5, 2021
71c00d5
Merge branch 'master' into exprement-override
Kingwl Mar 8, 2021
e3c0993
Accept baseline
Kingwl Mar 8, 2021
89517a0
Fix typo and add more check
Kingwl Mar 9, 2021
6c8d00d
Fix error name
Kingwl Mar 9, 2021
bbfb7eb
Merge branch 'master' into exprement-override
Kingwl Mar 10, 2021
4575b5a
Merge branch 'master' into exprement-override
Kingwl Mar 26, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update description
  • Loading branch information
Kingwl committed Jul 21, 2020
commit 2fd560ae922a171eb377e7905d41ea925fc144bd
6 changes: 3 additions & 3 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34428,16 +34428,16 @@ namespace ts {
const prop = getPropertyOfType(typeWithThis, declaredProp.escapedName);
const baseProp = getPropertyOfType(baseWithThis, declaredProp.escapedName);
if (prop && !baseProp && hasOverride) {
error(member, Diagnostics.Method_cannot_have_override_modifier_because_it_s_not_existed_in_the_base_class_0, baseClassName);
error(member, Diagnostics.Class_member_cannot_have_override_modifier_because_it_s_not_existed_in_the_base_class_0, baseClassName);
}
else if (prop && baseProp && !hasOverride) {
error(member, Diagnostics.Method_must_have_override_modifier_because_it_s_override_the_base_class_0, baseClassName);
error(member, Diagnostics.Class_member_must_have_override_modifier_because_it_s_override_the_base_class_0, baseClassName);
}
}
}
else if (hasOverride) {
const className = typeToString(type);
error(member, Diagnostics.Method_cannot_have_override_modifier_because_class_0_does_not_extended_another_class, className);
error(member, Diagnostics.Class_member_cannot_have_override_modifier_because_class_0_does_not_extended_another_class, className);
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3361,15 +3361,15 @@
"category": "Error",
"code": 4110
},
"Method cannot have override modifier because class '{0}' does not extended another class.": {
"Class member cannot have override modifier because class '{0}' does not extended another class.": {
"category": "Error",
"code": 4111
},
"Method cannot have override modifier because it's not existed in the base class '{0}'.": {
"Class member cannot have override modifier because it's not existed in the base class '{0}'.": {
"category": "Error",
"code": 4112
},
"Method must have override modifier because it's override the base class '{0}'.": {
"Class member must have override modifier because it's override the base class '{0}'.": {
"category": "Error",
"code": 4113
},
Expand Down
32 changes: 16 additions & 16 deletions tests/baselines/reference/override1.errors.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
tests/cases/conformance/override/override1.ts(9,5): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override1.ts(11,14): error TS4112: Method cannot have override modifier because it's not existed in the base class 'B'.
tests/cases/conformance/override/override1.ts(15,14): error TS4111: Method cannot have override modifier because class 'C' does not extended another class.
tests/cases/conformance/override/override1.ts(22,9): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override1.ts(24,18): error TS4112: Method cannot have override modifier because it's not existed in the base class 'B'.
tests/cases/conformance/override/override1.ts(33,5): error TS4113: Method must have override modifier because it's override the base class '(Anonymous class)'.
tests/cases/conformance/override/override1.ts(37,14): error TS4112: Method cannot have override modifier because it's not existed in the base class '(Anonymous class)'.
tests/cases/conformance/override/override1.ts(42,18): error TS4111: Method cannot have override modifier because class '(Anonymous class)' does not extended another class.
tests/cases/conformance/override/override1.ts(9,5): error TS4113: Class member must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override1.ts(11,14): error TS4112: Class member cannot have override modifier because it's not existed in the base class 'B'.
tests/cases/conformance/override/override1.ts(15,14): error TS4111: Class member cannot have override modifier because class 'C' does not extended another class.
tests/cases/conformance/override/override1.ts(22,9): error TS4113: Class member must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override1.ts(24,18): error TS4112: Class member cannot have override modifier because it's not existed in the base class 'B'.
tests/cases/conformance/override/override1.ts(33,5): error TS4113: Class member must have override modifier because it's override the base class '(Anonymous class)'.
tests/cases/conformance/override/override1.ts(37,14): error TS4112: Class member cannot have override modifier because it's not existed in the base class '(Anonymous class)'.
tests/cases/conformance/override/override1.ts(42,18): error TS4111: Class member cannot have override modifier because class '(Anonymous class)' does not extended another class.


==== tests/cases/conformance/override/override1.ts (8 errors) ====
Expand All @@ -19,17 +19,17 @@ tests/cases/conformance/override/override1.ts(42,18): error TS4111: Method canno

fooo (v: string) {}
~~~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.

override bar(v: string) {}
~~~
!!! error TS4112: Method cannot have override modifier because it's not existed in the base class 'B'.
!!! error TS4112: Class member cannot have override modifier because it's not existed in the base class 'B'.
}

class C {
override foo(v: string) {}
~~~
!!! error TS4111: Method cannot have override modifier because class 'C' does not extended another class.
!!! error TS4111: Class member cannot have override modifier because class 'C' does not extended another class.
}

function f () {
Expand All @@ -38,11 +38,11 @@ tests/cases/conformance/override/override1.ts(42,18): error TS4111: Method canno

fooo (v: string) {}
~~~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.

override bar(v: string) {}
~~~
!!! error TS4112: Method cannot have override modifier because it's not existed in the base class 'B'.
!!! error TS4112: Class member cannot have override modifier because it's not existed in the base class 'B'.
}
}

Expand All @@ -53,19 +53,19 @@ tests/cases/conformance/override/override1.ts(42,18): error TS4111: Method canno
override foo () { }
bar () { }
~~~
!!! error TS4113: Method must have override modifier because it's override the base class '(Anonymous class)'.
!!! error TS4113: Class member must have override modifier because it's override the base class '(Anonymous class)'.

baz() {}

override bazz () {}
~~~~
!!! error TS4112: Method cannot have override modifier because it's not existed in the base class '(Anonymous class)'.
!!! error TS4112: Class member cannot have override modifier because it's not existed in the base class '(Anonymous class)'.
}

function ff () {
return class {
override foo () {}
~~~
!!! error TS4111: Method cannot have override modifier because class '(Anonymous class)' does not extended another class.
!!! error TS4111: Class member cannot have override modifier because class '(Anonymous class)' does not extended another class.
}
}
8 changes: 4 additions & 4 deletions tests/baselines/reference/override2.errors.txt
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) ====
Expand All @@ -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?
Copy link
Member

Choose a reason for hiding this comment

The 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 override should be required to implement an abstract method. (It’s not really overriding anything.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we raise an error if use override with implement abstract method?

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member
@DanielRosenwasser DanielRosenwasser Sep 16, 2020

Choose a reason for hiding this comment

The 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 abstract member in a class (i.e. it's still marked as abstract), then you MUST use an override keyword.

Copy link
Member
@andrewbranch andrewbranch Sep 16, 2020

Choose a reason for hiding this comment

The 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;
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

@andrewbranch I switched your comment to use unknown just because void in base, number in derived seems suspicious

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@mprobst I think that the current message there is

Non-abstract class 'Child' does not implement inherited abstract member 'myMethod' from class 'Parent'.

is there something I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

If users were able to put override on method, you'd get:

Non-abstract class 'Child' does not implement inherited abstract member 'myMethod' from class 'Parent'.
Method 'method' of 'Child' must override a super type method (or some such)

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 method. With override on implementing methods, you'll get an error for code that's no longer useful.

Copy link
Member

Choose a reason for hiding this comment

The 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 {
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/override3.errors.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
tests/cases/conformance/override/override3.ts(22,5): error TS4113: Method must have override modifier because it's override the base class 'D'.
tests/cases/conformance/override/override3.ts(22,5): error TS4113: Class member must have override modifier because it's override the base class 'D'.


==== tests/cases/conformance/override/override3.ts (1 errors) ====
Expand All @@ -25,7 +25,7 @@ tests/cases/conformance/override/override3.ts(22,5): error TS4113: Method must h
class EB extends D {
foo(): void {}
~~~
!!! error TS4113: Method must have override modifier because it's override the base class 'D'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'D'.
override bar(): void {}
}

16 changes: 8 additions & 8 deletions tests/baselines/reference/override4.errors.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
tests/cases/conformance/override/override4.ts(11,5): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override4.ts(13,5): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override4.ts(17,5): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override4.ts(23,5): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override4.ts(11,5): error TS4113: Class member must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override4.ts(13,5): error TS4113: Class member must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override4.ts(17,5): error TS4113: Class member must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override4.ts(23,5): error TS4113: Class member must have override modifier because it's override the base class 'B'.


==== tests/cases/conformance/override/override4.ts (4 errors) ====
Expand All @@ -17,23 +17,23 @@ tests/cases/conformance/override/override4.ts(23,5): error TS4113: Method must h
class D extends B {
p1: number = 2;
~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.
override p2: number = 3;
p3: () => void;
~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.
override p4: () => void;
override foo (v: string) {}

fooo (v: string) {}
~~~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.

}

class DD extends B {
override foo: () => void
fooo: () => void;
~~~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.
}
4 changes: 2 additions & 2 deletions tests/baselines/reference/override5.errors.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
tests/cases/conformance/override/override5.ts(10,13): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override5.ts(10,13): error TS4113: Class member must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override5.ts(12,14): error TS1040: 'override' modifier cannot be used in an ambient context.
tests/cases/conformance/override/override5.ts(14,14): error TS1243: 'static' modifier cannot be used with 'override' modifier.
tests/cases/conformance/override/override5.ts(16,14): error TS1030: 'override' modifier already seen.
Expand All @@ -18,7 +18,7 @@ tests/cases/conformance/override/override5.ts(21,5): error TS1089: 'override' mo
class D extends B{
declare p1: number
~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.

override declare p2: number
~~~~~~~
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/override6.errors.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
tests/cases/conformance/override/override6.ts(9,12): error TS4113: Method must have override modifier because it's override the base class 'B'.
tests/cases/conformance/override/override6.ts(9,12): error TS4113: Class member must have override modifier because it's override the base class 'B'.


==== tests/cases/conformance/override/override6.ts (1 errors) ====
Expand All @@ -12,7 +12,7 @@ tests/cases/conformance/override/override6.ts(9,12): error TS4113: Method must h
class D extends B {
public bar: number = 1
~~~
!!! error TS4113: Method must have override modifier because it's override the base class 'B'.
!!! error TS4113: Class member must have override modifier because it's override the base class 'B'.
constructor(public foo: string, public baz: number) {
super(foo, 42)
}
Expand Down
0