<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/117458/">https://git.reviewboard.kde.org/r/117458/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 10th, 2014, 3:12 p.m. UTC, <b>Sven Brauch</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117458/diff/3/?file=263994#file263994line162" style="color: black; font-weight: bold; text-decoration: underline;">duchain/declarationbuilder.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ReferencedTopDUContext DeclarationBuilder::build(const IndexedString& url,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">156</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">i18n</span><span class="p">(</span><span class="s">"Different types returned from the same function: %1 and %2"</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Did you try this in some real-world JS code? Judging from what it is like in Python, something like this might have so many false positives that it's likely to do more harm than good ...
Instead, I would merge the old and new return type into an unsure type.</pre>
</blockquote>
<p>On April 11th, 2014, 2:38 p.m. UTC, <b>Denis Steckelmacher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Can I do that in a future patch? As you said, this one is already fairly big (I'm sorry for that but I could not help but add features until I was sure the infrastructure was solid and future-proof). I have two patches that I would like post for review after this one: the one add support for UnsureTypes in return statements, and one that already works and that allows the type of variable to be determined after its declaration (like in "var a; a = 3;", where a becomes an int). I would also like to be able to infer that the parameter a in "function foo(a) {}; foo(3.14);" is of type int. I have ideas but I still need to experiment.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yes, sure. You could also just not submit this part until it is sorted out. But it doesn't matter.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 10th, 2014, 3:12 p.m. UTC, <b>Sven Brauch</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117458/diff/3/?file=263997#file263997line50" style="color: black; font-weight: bold; text-decoration: underline;">duchain/helper.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">using namespace KDevelop;</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">50</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KDEVQMLJSDUCHAIN_EXPORT</span> <span class="kt">bool</span> <span class="nf">isWeakType</span><span class="p">(</span><span class="k">const</span> <span class="n">AbstractType</span><span class="o">::</span><span class="n">Ptr</span><span class="o">&</span> <span class="n">type</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Do you really need to treat void as useless? void is a valid type, it might be the correct guess. And the information that e.g. a function returns void is useful (while "it returns mixed" is not).</pre>
</blockquote>
<p>On April 11th, 2014, 2:38 p.m. UTC, <b>Denis Steckelmacher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">isWeakTypes does not mean that a type is useless, but that the information it carries is less important that the one of other types. For instance, a function is by default declared as returning void. If a return statement is found, and because void is weak, the return statement's type will replace "void" as the return type of the function. If the function already has a non-weak return type (int, string, struct, etc), then a return statement having a weak type (for instance "mixed", when the type could not be inferred) will be ignored, the current return type of the function taking precedence.
If the return statement is of type "mixed" and the function currently returns "void", then its return type is set to "mixed" (because, actually, the function returns something).
If the function's return type and the type of the return statement are both strong and different, then I raise a warning (that will later be changed to an Unsure type).</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think setting it to void by default is the mistake which causes all this trouble. If you would not set a type at all in the beginning, and only set it to void in the end if no type is set yet, then I think this would work without considering void as weak.
The background of this thought is that when you have a function which returns void in one place and something different in another place it would be nice to show this as unsure(void, int) -- which you can't do if you set it to void by default. And it's useful information for the user that the function might return nothing.
Regargind warnings, I would only generate warnings if you're 100% sure something is wrong. Everything else will litter the user's working code with warnings which don't indicate an actual problem, which is super annoying and makes the few real warnings useless. And the places where you can do real warnings in dynamic languages is very very small -- basically, undeclared local variables and not much more. I had some kinds of warnings in Python but removed them all again because I hate false positives (it litters perfectly good code with random yellow underlines). When KDevelop tells you it's wrong then you should be able to more or less rely on it being wrong in my opinion.</pre>
<br />
<p>- Sven</p>
<br />
<p>On April 11th, 2014, 2:38 p.m. UTC, Denis Steckelmacher wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Denis Steckelmacher.</div>
<p style="color: grey;"><i>Updated April 11, 2014, 2:38 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-qmljs
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch adds support for function expressions to the QML/JS plugin. This support required the following changes:
* ExpressionVisitor stops visiting when it encounters a type (no need to traverse the AST down to its bottom if the top-most expression is enough to determine a type). This gives it a O(1) time complexity.
* When DeclarationBuilder wants to know the type of an expression, it first visits this expression (to build any needed declaration), then only starts an ExpressionVisitor. This way, ExpressionVisitor knows all the declarations that may occur in the expression and is able to return its complete type. Moreover, if DeclarationBuilder has called getType (and has visited an expression), then will not enter the node it is visiting (as it already did so). This way, DeclarationBuilder keeps an O(n) time complexity.
* These two changes are illustrated by the support of "var f = function() { return true; }" statements.
* This patch also adds support for binary operators ("a == b" is always a boolean, "a << 2" is always an int, etc). This fixes a bug where the type of "(a == 5)" was wrongly guessed as "int"
The support for function expressions is split between DeclarationBuilder and ExpressionVisitor. DeclarationBuilder, when it encounters a function expression, creates a declaration for it, giving it a name built from the hexadecimal representation of its FunctionExpression* node pointer. When DeclarationBuilder sees a return statement, it sets the type of the enclosing function (whether it is a function declaration or a function expression has no importance here). When ExpressionVisitor is used to find the type of an expression, and this expression is a function expression, then ExpressionVisitor will build the hexadecimal name of the function and use getDeclaration to find its type.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">All the unit tests of the QML/JS plugin pass with this patch. Three tests have been added:
* The test for type inference is not expected to fail anymore, as the plugin is able to infer simple types
* New test for "(a == b)" expressions
* New test for "function() {}" expressions
Manual testing shows that syntax highlighting is correct, and also the "outline" view. Building the anonymous declarations by passing them an empty range (RangeInRevision()) removes the anonymous function from the outline view and disables any highlighting related to this declaration in the source view. The function can still have a name which can be freely chosen by the plugin.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>duchain/CMakeLists.txt <span style="color: grey">(9fab69c)</span></li>
<li>duchain/contextbuilder.h <span style="color: grey">(d53b2fe)</span></li>
<li>duchain/contextbuilder.cpp <span style="color: grey">(d38da2d)</span></li>
<li>duchain/declarationbuilder.h <span style="color: grey">(001c3b2)</span></li>
<li>duchain/declarationbuilder.cpp <span style="color: grey">(98da341)</span></li>
<li>duchain/expressionvisitor.h <span style="color: grey">(b4f0851)</span></li>
<li>duchain/expressionvisitor.cpp <span style="color: grey">(766435d)</span></li>
<li>duchain/helper.h <span style="color: grey">(23a6d0e)</span></li>
<li>duchain/helper.cpp <span style="color: grey">(3ffdd56)</span></li>
<li>duchain/parsesession.h <span style="color: grey">(0eb2762)</span></li>
<li>duchain/parsesession.cpp <span style="color: grey">(dbc4d90)</span></li>
<li>duchain/tests/testdeclarations.cpp <span style="color: grey">(668efaa)</span></li>
<li>tests/files/helloworld.js <span style="color: grey">(ff40f1c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117458/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>