<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 5, 2014 at 3:00 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class="im">
<div>On Feb 5, 2014, at 2:29 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 5, 2014 at 12:43 PM, Ben Langmuir<span> </span><span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span><span> </span>wrote:<br>
<blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote"><div><br>On Feb 5, 2014, at 11:18 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
<br>> I'm not especially fond of treating annot_module_begin and annot_module_include differently. That'll mean we get completely different diagnostics for a '#include' in a bad place depending on whether it's including something in the same top-level module or something in a different top-level module.<br>
><br><br></div>I approached this from the other side and didn't like that annot_module_include is handled so differently from @import.</blockquote><div><br></div><div>I think they're fundamentally different, at least currently:</div>
<div><br></div><div>#include sometimes triggers a submodule build (in the same preprocessor + parser + sema) so must only appear at the top level in such a case. We also want the #include to behave (effectively) the same whether it textually includes a file or imports a module, so that people don't accidentally write code that means different things with compilers that support modules and with compilers that do not (or depending on which module a particular header is part of, or the order in which submodules of a module are built).</div>
<div><br></div><div>Conversely, @import merely makes declaration and macro names visible. It never triggers an in-situ submodule build (sometimes a different module will be built in a separate thread and context, but that's not a problem). It can't (reliably) be used to import a submodule of the same module. So, there's no *technical* reason not to allow @import anywhere, as far as I can tell. That said, some of these properties are probably bugs in @import, and maybe it should act more like #include.</div>
</div></div></div></div></blockquote><div><br></div></div><div>Sounds like you know much more about this than I do :) </div><div><br></div><div>When you say “sometimes a different module will be build in a separate thread and context” does that include building a submodule of the current module, or can that never happen?’</div>
</div></div></blockquote><div><br></div><div>That doesn't happen today. If you have a.h and b.h in module X, and we happen to decide to build a.h before b.h, and a.h contains:</div><div><br></div><div>@import X.b;</div>
<div><br></div><div>... then that @import is a no-op. If it had been</div><div><br></div><div>#include "b.h"</div><div><br></div><div>we would have built the submodule X.b immediately. See test/Modules/Inputs/submodules/import-self-b.h for a complete example.</div>
<div><br></div><div>I suspect this is a bug =)</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div>I agree that there is no technical reason @import can’t live anywhere, and as you say I would like to constrain it to be more like #include in this respect.</div><div class="im">
<div><br></div><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote">
<div>> I'm also not sure that the diagnostic change here is an improvement. Previously:<br>><br>> #include "not_module.h"<br>> #include "module.h"<br>><br>> ... where not_module.h contains:<br>
><br>> class X {<br>>   // ...<br>> // whoops, forgot the '};'<br>><br>> would give a "missing '}'" diagnostic, pointing at the '{' that is left un-closed. It now gives a mysterious 'must be at the top level' diagnostic, not pointing at the '}' any more, followed by more errors (because 'module.h' didn't get imported and because the code after the #include is still within the class). The error here was almost certainly that a '}' was missing, so this seems like a regression.<br>
<br></div>Ick, I’ll take a look at this case.<br><div><br>><br>> In what cases does this change allow us to recover from errors better?<br><br><br></div>Whenever an implicit import is not at top level, we currently give errors like ‘expected expression’, or ‘expected }’, pointing at the location on #include/#import, and give no hint as to how a preprocessor directive (#include) could be the source of the error.</blockquote>
<div><br></div><div>We can, and almost certainly should, customize the ExpectAndConsume diagnostic for the case where the current token is one of the annot_module_* tokens. I think that's probably the best way to attack this issue.</div>
</div></div></div></div></blockquote><div><br></div></div><div>I’ll look into this.</div><div class="im"><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote">
To make matters worse, once you break out of whatever parsing context you were in due to the module_include, any following code is likely to become errors.<br><br>int foo() {<br>  int x = 5;<br>  #import “Bar.h” // expected }<br>
  if (x == 6) {// expected identifier or (<br>    ++x; // expected identifier or (<br>  }<br>  return 1; // expected identifier of (<br>} // extraneous closing brace</blockquote><div><br></div><div>Do you think this is actually a likely error case? I find it implausible that someone would deliberately include a modular header into the middle of some scope. I think it's vastly more likely that they missed a close brace before the include.</div>
</div></div></div></div></blockquote><div><br></div></div><div>This was prompted by code like the following being spotted "in the wild" (where the actual header is in a system module):</div><div><br></div><div>@implementation SomeClass</div>
<div>#import <system/header.h></div><div>// … more stuff here</div><div>@end</div><div><br></div><div>I have no data to support it being a common case, but I’d like to improve the error if possible.</div></div></div>
</blockquote><div><br></div><div>OK, I can certainly believe that people do this somewhat deliberately inside an @implementation (but not so much in a class or function). We occasionally see people try to put #includes inside extern "C" and namespaces in C++ (mistakenly believing they're including a C header, usually), and that's another case where assuming that we should be at the top level might not work out very well.</div>
<div><br></div><div>Conversely, I have data to support the position that missing-}-at-end-of-file is a big problem in C++, and our diagnostics for it are currently terrible. For a missing } in a modular header, we have a great story, and we *know* the 'expected }' diagnostic is correct, but for a non-modular header, the best answer we have today is to hope that the *next* include will be of a modular header. So I think recovering by skipping the include and staying in the nested scope is also not going to work out well.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="im">
<blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>If this really is a likely scenario, then I think we should make the diagnostic for an unexpected annot_module_* a FatalError, because there's simply no way we can reasonably recover (we have two recovery options, and either of them is disastrous if we're in the other situation).</div>
</div></div></div></div></blockquote></div></div><br><div>Wouldn’t this make your example even worse? Instead of getting a bogus error (an #import not being at top level) followed by the actual error (missing }) you would only see the bogus one?</div>
</div></blockquote><div><br></div><div>I was imagining that we'd only produce the one error (the error that is currently "expected '}'", but with a change to ExpectAndConsume would presumably become something more like "fatal error: module import not at top level; expected '}'" with a note pointing to the '{' as normal).<br>
</div></div></div></div>