<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Want to share some of my experience with non-empty DelayedTypos in ~Sema, hopefully it can be useful. Many times there are lingering delayed typos because of multiple typos in an expression. So when we try to fix one typo, it fails and we discard the entire expression which can contain more TypoExpr in it. Correct way seems to check discarded expressions and clean up corresponding TypoExpr. But with the current design there is no simple way to do that and it has limited value when assertions are disabled.<div class=""><br class=""></div><div class="">I think the change suggested in <a href="https://reviews.llvm.org/D64799" class="">https://reviews.llvm.org/D64799</a> is reasonable and should help with low-value assertion failures.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Volodymyr<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jul 16, 2019, at 10:46, Ilya Biryukov 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" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">Thanks for pointing this out, the codegen does run on every top level declaration.<div class="">However, it does nothing if any errors were reported. </div><div class=""><br class=""></div><div class="">That means we could prevent codegen by:</div><div class="">1. emitting the diagnostics for uncorrected typos on each top-level declaration, before the codegen kick in,</div><div class="">2. checking if there are any "pending" typos in addition to checking for errors before doing the codegen.</div><div class=""><br class=""></div><div class="">Either should be doable. (1) has the advantage of reporting the errors earlier, making them easier to fix/diagnose.</div><div class=""><br class=""></div><div class="">However, (1) might not be a little involved. At least I got the impression from talking to various people that some typos are only fixed at template instantiation time.</div><div class="">The code to figure out at which point the uncorrected typos should be emitted for template instantiations might be a little involved because of this.</div><div class=""><br class=""></div><div class="">I would be surprised if the proposed assertion(!HasErrors || Typos.empty()) ever fired in practice. It's rare to see only a single compiler error coming from clang, so I would expect almost any typo to induce at least another error right away. That's actually why I'd expect the "broken codegen" to be hardly possible in practice.</div><div class=""><br class=""></div><div class="">Out of the options we have, I'll probably add checks for (2) to codegen and emit the delayed typos at the end of TU. That seems to be the simplest option, at least.</div><div class="">Happy to go with (1) or the alternative assertion if people think the proposed approach would lead to too many diagnostics.</div></div><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div class="gmail_quote" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;"><div dir="ltr" class="gmail_attr">On Tue, Jul 16, 2019 at 7:29 PM Reid Kleckner <<a href="mailto:rnk@google.com" class="">rnk@google.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class=""><div dir="ltr" class="">On Tue, Jul 16, 2019 at 10:18 AM David Blaikie via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank" class="">cfe-dev@lists.llvm.org</a>> wrote:<br class=""></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class="">End of the TU sounds too late to me - IR generation is done incrementally (at the end of functions, for instance - though I'm not sure that's the only point), so leaving typos in until the end of the TU could lead to the "IR generation getting weird because of pending typo corrections" issue, no?</div></blockquote><div class=""><br class=""></div><div class="">Well, we currently assert in ~Sema, which is after the end of the TU, so if we hit today's assert, we've already done incremental codegen without crashing. Diagnosing at end of TU doesn't make the situation worse. The way I understand clang's incremental codegen strategy, we generate code after every top level decl. We sometimes skip incremental codegen if errors have been reported or if a Decl is invalid. I think it's a bug if an error hasn't been reported but a delayed typo expr gets sent to codegen, and we should add asserts to defend against that.</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class=""><div dir="ltr" class="">On Mon, Jul 15, 2019 at 8:19 PM Ilya Biryukov via clangd-dev <<a href="mailto:clangd-dev@lists.llvm.org" target="_blank" class="">clangd-dev@lists.llvm.org</a>> wrote:</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr" class=""><div class="">We would like to avoid assertion failures for those, which leads me to the following questions:</div><div class="">- Is there a way to quickly track down the place that miss the CorrectDelayedTypos* call?</div></div></blockquote><div class="">A common pattern is that an error causes an Expr subtree to be discarded, and the code that does so "forgets" to call CorrectDelayedTypos.</div><div class="">e.g. <a href="https://reviews.llvm.org/rL366200" target="_blank" class="">https://reviews.llvm.org/rL366200</a></div><div class="">There's usually a diagnostic emitted before the Expr is discarded, so in these cases poking around the diag emit location often sheds light. But my fear is there are tens or hundreds of these bugs, and it's hard to enumerate them.</div><div class=""><br class=""></div><div class="">At some level, this seems silly - if the Expr doesn't survive, its typos don't need to be corrected to protect CodeGen from them. The diagnostics are probably important though.</div><div class="">If we could ensure the diagnostics are emitted as Reid says, and reduce the requirement to be that Exprs that survive parsing get typo-corrected, then this might be tractable.</div></div></div></blockquote></div></blockquote></div></blockquote></div></blockquote><div class=""><br class=""></div><div class="">Another idea is to weaken the assert to validate that either errors have been reported, or there are no delayed typos. That would mean it's OK to forget to diagnose typos if parts of the AST are invalid, since we expect the user to fix their code and recompile, potentially discovering the typo on the next compile.</div></div></div></blockquote></div><br clear="all" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""></div><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">--<span class="Apple-converted-space"> </span></span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div dir="ltr" class="gmail_signature" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;"><div dir="ltr" class=""><div class=""><div dir="ltr" class=""><div class="">Regards,</div><div class="">Ilya Biryukov</div></div></div></div></div><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">_______________________________________________</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">cfe-dev mailing list</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><a href="mailto:cfe-dev@lists.llvm.org" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">cfe-dev@lists.llvm.org</a><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a></div></blockquote></div><br class=""></div></body></html>