Skip to content

Extending a class method with prototype assignment confuses the compiler about membership type #22895

Description

@mohsen1

This is an issue I encountered when working on webpack/webpack#6862

Bug Repository

See the bug repo for complete code
https://github.com/mohsen1/ts-bug-instance-member

Run npm test to run the test

Expected behavior:
No Error
Actual behavior:

index.js:7:19 - error TS2424: Class 'Ex' defines instance member function 'foo', but extended class 'MyClass' defines it as instance member property.

7 MyClass.prototype.foo = function() {
                    ~~~

Activity

  1. sandersn commented on Mar 27, 2018

    @sandersn
    Member

    Fix is up at #22935

  2. mohsen1 commented on Apr 1, 2018

    @mohsen1
    ContributorAuthor

    Nathan Shively-Sanders (@sandersn) using the nightly I'm still getting the error

    mohsen1/ts-bug-instance-member#1

  3. sandersn commented on Apr 2, 2018

    @sandersn
    Member

    I see, it looks like it breaks going the other direction, when the base uses prototype assignment. I’ll take anothe look.

  4. sandersn commented on Apr 2, 2018

    @sandersn
    Member

    Well, actually it turns out that it's the arrow function being used as a method. This is ... weird, but technically correct as long as the author remembers that this isn't an instance of Ex. I'll have a fix up shortly; I see a couple of ways to do it.

  5. sandersn commented on Apr 2, 2018

    @sandersn
    Member

    Note that --noImplicitThis works with "checkJs": true, but it marks every usage of this outside a class, which makes it hard to use transitionally. So recognising arrow functions as methods is good for understanding existing code, but bad in that it doesn't prevent js authors from possibly buggy new code.

    I think the right thing is to prefer understanding existing code, since we hope people with write new code in Typescript. They will, at least, have years of experience lowering their expectations of the correctness of any new Javascript that they write.

  6. sandersn commented on Apr 2, 2018

    @sandersn
    Member

    OK, the fix is merged. Mohsen Azimi (@mohsen1) can you try typescript@next tomorrow?

  7. mohsen1 commented on Apr 2, 2018

    @mohsen1
    ContributorAuthor

    Thank you for prompt response Nathan Shively-Sanders (@sandersn)!

  8. mohsen1 commented on Apr 4, 2018

    @mohsen1
    ContributorAuthor

    Still seeing this error with latest TS nightly.

    Run yarn tsc in this branch of Webpack to see errors:

    https://github.com/mohsen1/webpack/tree/bd1c827c884d943f68bbbf00cd91003875ae0bb2

    sample:

    
    lib/NormalModule.js:455:2 - error TS2425: Class 'Module' defines instance member property 'size', but extended class 'NormalModule' defines it as instance member function.
    
    455  size() {
         ~~~~
    
  9. sandersn commented on Apr 4, 2018

    @sandersn
    Member

    Yikes! OK, the problem this time is that the initialisers on the base aren't methods at all. They are null, which is definitely not method-like. I think the intention is to document that they are "abstract", but without creating a notImplementedFunction that always throws.

    Upon re-reading the override checking code, it seems that the intent of the error is to prevent instance vs property confusion like this:

    class Super {
      constructor() {
        this.p = () => 1
      }
    }
    class Sub extends Super {
      p() { return 2 }
    }
    console.log(new Sub().p()) // 1 ???!!!?
    
    class Base {
      get x() { return 1 }
    }
    class Derived extends Base { }
    Derived.prototype.x = 2
    console.log(new Derived().x) // 1 ??/?? it keeps happening

    So "isMethodLike" should really be "isPrototypeProperty" but the compiler is not very good at tracking this distinction. Fortunately, for Javascript special assignments, we can just look at the left-hand-side of the assignment. I'll go make that change.

  10. sandersn commented on Apr 4, 2018

    @sandersn
    Member

    OK, #23137 improves the isMethodLike check.

  11. locked and limited conversation to collaborators on Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

BugA bug in TypeScriptFixedA PR has been merged for this issue

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions