Assertion error with if-else-if inside class #5444

Activity

1. Mohamed Hegazy (mhegazy)

   added

   BugA bug in TypeScriptA bug in TypeScript

   Help WantedYou can do thisYou can do this

   on Oct 29, 2015
2. mhegazy commented on Oct 29, 2015

   

   Mohamed Hegazy (mhegazy)

   on Oct 29, 2015

   Contributor

   More actions

   A PR would be appreciated.
3. Mohamed Hegazy (mhegazy)

   added this to the CommunityThis milestone has been deleted milestone on Oct 29, 2015
4. jeffreymorlan commented on Oct 31, 2015

   

   jeffreymorlan

   on Oct 31, 2015

   ContributorAuthor

   More actions

   I don't know what the right fix to this is, but here's my analysis:

   • The "if"s are getting parsed as MethodDeclarations. The first method node's .end is less than the second method node's .pos because of the intervening erroneous "else", which does not become a node.

   • checkFunctionOrConstructSymbol loops over declarations of a symbol. If declarations are not "consecutive", it calls reportImplementationExpectedError on the earlier one:

     else if (!isExportSymbolInsideModule && previousDeclaration && previousDeclaration.parent === node.parent && previousDeclaration.end !== node.pos) {
         reportImplementationExpectedError(previousDeclaration);
     }

   • reportImplementationExpectedError assumes that its node and its next sibling (found by ts.forEachChild) must be for different symbols (and thus differ in some way: kind, name, or static-ness). Because if they were for the same symbol, those would be "consecutive" declarations and not an error.

   The problem is the two different definitions of what makes nodes "consecutive". Is it node.pos === prev.end, or being next in the ts.forEachChild iteration? cFOCS uses the former definition, while rIEE uses the latter. And with the current parser, they are not the same.
5. mhegazy commented on Nov 7, 2015

   

   Mohamed Hegazy (mhegazy)

   on Nov 7, 2015

   Contributor

   More actions

   Vladimir Matveev (@vladima) any thoughts?
6. jeffreymorlan

   added 2 commits that reference this issue on Nov 8, 2015

   Fix issue microsoft#5444

   c72477b

   Fix issue microsoft#5444

   ...

   470d1d2
7. jeffreymorlan commented on Nov 8, 2015

   

   jeffreymorlan

   on Nov 8, 2015

   ContributorAuthor

   More actions

   I've realized it would be inefficient for checkFunctionOrConstructorSymbol to walk the tree since it has to check for consecutiveness on every function/method overload, whether error or not, and having to scan the list of children for each means checking a class becomes potentially O(n²). So it's reportImplementationExpectedError that needs to change. PR
8. Daniel Rosenwasser (DanielRosenwasser)

   closed this as completedin #5566on Nov 28, 2015
9. Daniel Rosenwasser (DanielRosenwasser)

   added a commit that references this issue on Nov 28, 2015

   Merge pull request #5566 from jeffreymorlan/fix5444

   ...

   2f282a7
10. Daniel Rosenwasser (DanielRosenwasser)

    added

    FixedA PR has been merged for this issueA PR has been merged for this issue

    on Nov 28, 2015
11. DanielRosenwasser commented on Nov 28, 2015

    

    Daniel Rosenwasser (DanielRosenwasser)

    on Nov 28, 2015

    Member

    More actions

    Thanks jeffreymorlan!
12. Mohamed Hegazy (mhegazy)

    modified the milestones: TypeScript 1.8, CommunityThis milestone has been deleted on Nov 30, 2015
13. microsoft

    locked and limited conversation to collaborators on Jun 19, 2018