<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Sam,<div class=""><br class=""></div><div class="">Thanks for working on this! Clang’s tooling will definitely benefit from this work.<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On May 10, 2019, at 1:01 AM, Sam McCall via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="">Thanks, Jan. I'm not familiar with all of the relevant parts of Parser/Sema, so these are useful pointers.</div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 9, 2019 at 9:32 PM Jan Korous <<a href="mailto:jkorous@apple.com" class="">jkorous@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class="">Hi Sam,<div class=""><br class=""></div><div class="">This seems really useful indeed.</div><div class=""><br class=""></div><div class="">I am just wondering about how this would play with speculative actions.</div><div class=""><br class=""></div><div class="">I am more familiar with speculative stuff in Parser (peculativeEvaluationRAII, TentativeParsingAction, Parser::TryParseLambdaIntroducer</div></div></blockquote><div class="">I'm not sure if we'd want to use this to represent errors at the parser level, I don't know enough yet about the parser to really have an opinion.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><div class=""> but it seems that sometimes we also use the same approach in Sema:</div><div class=""><div class="">- SFINAETrap (Which IIUC isn't used just for SFINAE - for example in RebuildForRangeWithDereference.)</div></div></div></blockquote><div class="">SFINAETrap is diagnostic-based, so we need to ensure when emitting RecoveryExpr:</div><div class=""> - we must also emit a diagnostic that causes substitution failure</div><div class=""> - we must not cause a diagnostic that is emitted even on substitution failure</div><div class="">Other than that, I expect this to interact OK: when substitution fails, the subtrees with RecoveryExprs will be discarded.</div><div class="">It may hurt the *performance* of substitution failure though, as we create more nodes rather than just cascading the failure immediately.</div><div class="">(RebuildForRangeWithDereference is indeed not SFINAE but should work the same way)</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><div class=""><div class="">- ContextRAII</div></div></div></blockquote><div class="">I'm not familiar with this class or how it's used for speculative actions - can you elaborate?</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><div class=""><div class="">- TypoExpr</div></div></div></blockquote><div class="">These are somewhat complementary in that TypoExpr is used when an identifier can't be resolved, and RecoveryExpr is useful to preserve subexpressions that *can* be resolved.</div><div class=""><br class=""></div><div class="">That said there are cases where these intersect like `invalid_function(valid_parameter)` - it would be nice to generate something sensible here: a RecoveryExpr(CallExpr) where invalid_function is dropped/represented by a TypoExpr/represented by a RecoveryExpr(DeclRefExpr)...</div><div class="">I'm not sure exactly how this should work mechanically, and also don't think it needs to work immediately.</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><div class=""><div class=""><br class=""></div><div class="">Any thoughts on that?</div><div class=""><br class=""></div><div class="">Cheers,</div><div class=""><br class=""></div><div class="">Jan</div><div class=""><br class=""><blockquote type="cite" class=""><div class="">On May 9, 2019, at 8:13 AM, Tom Honermann via clangd-dev <<a href="mailto:clangd-dev@lists.llvm.org" target="_blank" class="">clangd-dev@lists.llvm.org</a>> wrote:</div><br class="gmail-m_-7735652319510743880Apple-interchange-newline"><div class="">
<div bgcolor="#FFFFFF" class=""><p class="">This would be helpful. Within Coverity, we hacked together our own error recovery mechanism (mostly involving demoting errors to warnings and discarding function bodies and variable initializers with errors), but our mechanism sometimes leads to other down
stream errors (I investigated a spurious "enumerator 'x' does not exist in instantiation of 'y' just this week). The ability to explicitly indicate errors in the AST would be helpful.</p><p class="">Ideally, I'd like to have ErrorDecl, ErrorStmt, ErrorExpr, and ErrorType nodes. But there is no reason they would all have to be introduced at the same time.<br class="">
</p><p class="">Tom.<br class="">
</p>
<div class="gmail-m_-7735652319510743880moz-cite-prefix">On 5/9/2019 7:31 AM, Sam McCall via cfe-dev wrote:<br class="">
</div>
<blockquote type="cite" class="">
<div dir="ltr" class="">In the presence of broken code, the AST clang generates is often very limited.
<div class="">Often Sema diagnoses errors and doesn't create AST nodes. For example, if the arguments to a function call are wrong, we get no CallExpr, and all subexpressions are discarded.<br class="">
</div>
<div class="">IDE tools like clangd see a lot of broken code, and rely on the resulting AST, so I'd like to improve this situation.<br class="">
</div>
<div class=""><br class="">
</div>
<div class="">If the user writes the following erroneous code:</div>
<div class=""> a(b, c).d() // error: a only takes one parameter</div>
<div class="">then no AST is emitted for this expression, and:</div>
<div class="">1) clangd's "go to definition" doesn't work on a, even though there's no overloading</div>
<div class="">2) "go to definition" also doesn't work on b and c or their subexpressions, even though these expressions are valid</div>
<div class="">3) code completion doesn't work properly for d(), even though we know what type a() returns. (go to definition doesn't either).</div>
<div class="">There are lots of similar cases: (foo->bar when bar doesn't exist, etc).<br class="">
</div>
<div class=""><br class="">
</div>
<div class="">My proposal is to add a fairly generic RecoveryExpr that the AST can emit in these situations. It would capture:</div>
<div class=""> - the approximate StmtClass we were trying to create (e.g. CallExpr)</div>
<div class=""> - the subexpressions - (e.g. an overloadexpr for the callee, and the arguments)</div>
<div class=""> - the type, if we can make a reasonable guess (e.g. all overloads have the same type)</div>
<div class=""><br class="">
</div>
<div class="">I also considered other approaches, and think they don't work so well:</div>
<div class=""> - just generate the e.g. CallExpr etc anyway, and maybe add an "invalid" flag. This means weakening many invariants in many different node types. It's subtle and error-prone, and adds conceptual and implementation complexity to many nodes that now have
to be able to represent various error cases. Even within clang it's hard to find all the analyses that need to be updated. (<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_pipermail_cfe-2Ddev_2019-2DApril_062151.html&d=DwMFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=oOJsFtgkFCCcDlfLWKmfrVDtpXLihXJTBGJfSapXCV0&m=k3m3ocpqnJDQ1DBLtX6DNqMbG51U4RJ9VyH8qj2SfSc&s=jddDcS9V5Twt16Gxg9n1WTXzsxqOYx-KQIOIvXEovEc&e=" target="_blank" class="">http://lists.llvm.org/pipermail/cfe-dev/2019-April/062151.html</a>)</div>
<div class=""> - extract the extra information we need from Sema, rather than the AST. e.g. Sema should invoke a callback whenever an identifier is resolved to a Decl. This solves go-to-definition pretty well, but isn't a very flexible approach. The AST is rich, and
provides context, can be dumped etc, a Sema callback would be an ad-hoc solution that is less convenient to use and understand.</div>
<div class=""> - add a hierarchy of new nodes to represent errors (RecoveryCallExpr etc). It seems like a lot of work to model all the different ways code can be invalid, and I don't think the extra structure would be useful enough to justify it.</div>
<div class=""><br class="">
</div>
<div class="">I have a very rough prototype of this idea: <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D61722&d=DwMFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=oOJsFtgkFCCcDlfLWKmfrVDtpXLihXJTBGJfSapXCV0&m=k3m3ocpqnJDQ1DBLtX6DNqMbG51U4RJ9VyH8qj2SfSc&s=x4VyJNvrS0sFh72jDS_z6ox6SL4zxZLE4lIP3i7tnLc&e=" target="_blank" class="">https://reviews.llvm.org/D61722</a> (emits
RecoveryExpr for failed overload resolution).</div>
<div class="">It does create a form of recovery that affects diagnostics. But the changes exposed by tests are few and seem mostly good (especially compared to emitting CallExpr).</div>
<div class="">Some open questions:</div>
<div class=""> - what's the type of the expression when we can't guess? (IntTy in the patch)</div>
<div class=""> - will there be surprises if RecoveryExpr is emitted in more places and they start to interact?</div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><div class=""><div class=""><blockquote type="cite" class=""><div class=""><div bgcolor="#FFFFFF" class=""><blockquote type="cite" class=""><div dir="ltr" class="">
<div class=""> - should RecursiveASTVisitor, ASTMatchers etc skip these nodes by default, for compatibility with external code?</div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div></div></div></blockquote><div><br class=""></div><div>I think being conservative when it comes to the RecursiveASTVisitor is probably correct, but at the same time it would be nice if it was traversed by default. That way the existing tools can visit the children of the expression that the compiler thinks are correct so they will have access to a more complete AST. The typo expressions have a bit of a prior art there I suppose, since they actually don’t correspond to the real source, but to the source that the compiler thinks should’ve been written.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><div class=""><div class=""><blockquote type="cite" class=""><div class=""><div bgcolor="#FFFFFF" class=""><blockquote type="cite" class=""><div dir="ltr" class="">
<div class=""> - what are the important use cases that I haven't thought about?</div>
<div class=""><br class="">
</div>
</div>
<br class="">
<fieldset class="gmail-m_-7735652319510743880mimeAttachmentHeader"></fieldset>
<pre class="gmail-m_-7735652319510743880moz-quote-pre">_______________________________________________
cfe-dev mailing list
<a class="gmail-m_-7735652319510743880moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>
<a class="gmail-m_-7735652319510743880moz-txt-link-freetext" href="https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Ddev&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=oOJsFtgkFCCcDlfLWKmfrVDtpXLihXJTBGJfSapXCV0&m=k3m3ocpqnJDQ1DBLtX6DNqMbG51U4RJ9VyH8qj2SfSc&s=pBwBCrRoYbCV6AZvGdWTEDd5gKfDDajyTQ8VRoQ38LY&e=" target="_blank">https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Ddev&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=oOJsFtgkFCCcDlfLWKmfrVDtpXLihXJTBGJfSapXCV0&m=k3m3ocpqnJDQ1DBLtX6DNqMbG51U4RJ9VyH8qj2SfSc&s=pBwBCrRoYbCV6AZvGdWTEDd5gKfDDajyTQ8VRoQ38LY&e=</a>
</pre>
</blockquote>
</div>
_______________________________________________<br class="">clangd-dev mailing list<br class=""><a href="mailto:clangd-dev@lists.llvm.org" target="_blank" class="">clangd-dev@lists.llvm.org</a><br class=""><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/clangd-dev" target="_blank" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/clangd-dev</a><br class=""></div></blockquote></div><br class=""></div></div></blockquote></div></div>
_______________________________________________<br class="">cfe-dev mailing list<br class=""><a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a><br class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev<br class=""></div></blockquote></div><br class=""></div></body></html>