Fix bug 381123: crash while parsing PHP code
ClosedPublic

Authored by mtijink on Jul 22 2017, 5:49 PM.

Details

Summary

In bug 381123 KDevelop crashed on certain PHP code. The underlying cause seems to that the classDec does not have an internalContext() set (no clue why, I couldn't figure that out).

This diff fixes the crash, but does not yield the desired behaviour: automatically deriving the type of the foreach variable (i.e. in the example of the test plan: the $row variable should be an int, but is currently calculated as mixed. Placing the foreach after the class definition works, though).

Test Plan

The following steps used to crash kdevelop, now they don't.

  1. Open the following file and wait until it's been parsed (reduced from the example of Alexander Zighalin as posted on bug 381123)
<?php
/*

/* */
foreach(new foo() as $row) {}

class foo implements Iterator {
    public function current(): int {
    }
}
  1. Close the comment on line 2 by hitting enter and closing it with */

Diff Detail

Repository
R52 KDevelop: PHP Support
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mtijink created this revision.Jul 22 2017, 5:49 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJul 22 2017, 5:49 PM
zhigalin requested changes to this revision.Jul 23 2017, 10:17 AM
zhigalin added subscribers: kfunk, apol, zhigalin.

Cool! Seems we were working on the same issue
As mentioned by @apol and @kfunk a test is needed.

P.S. Zhigalin, not Zighalin

This revision now requires changes to proceed.Jul 23 2017, 10:17 AM

I found another possible fix: removing line 505 (classDec = dynamic_cast<ClassDeclaration*>(type->declaration(nullptr));) also fixes the issue, so that seems to be the root cause.

How should I write this test? The problem only occurs after modification of the file, so I do not know how to test it.

mwolff added a subscriber: mwolff.Jul 25 2017, 10:14 PM

the duchain_multiplefiles.{cpp,h} code should allow reparsing, look e.g. at TestDUChainMultipleFiles::testTodoExtractorReparse

mtijink updated this revision to Diff 17212.Jul 26 2017, 9:24 AM
mtijink edited edge metadata.

I added the test, and verified it fails without the diff and succeeds with the diff.

zhigalin accepted this revision.Jul 26 2017, 4:37 PM
zhigalin added inline comments.
duchain/builders/typebuilder.cpp
506 ↗(On Diff #17212)

Actually this performs a rescan using CursorInRevision::invalid

This revision is now accepted and ready to land.Jul 26 2017, 4:37 PM
This revision was automatically updated to reflect the committed changes.