Prevent module import inside @implementation

[Ben Langmuir]

On Feb 5, 2014, at 2:29 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Wed, Feb 5, 2014 at 12:43 PM, Ben Langmuir <blangmuir at apple.com> wrote:
> 
> On Feb 5, 2014, at 11:18 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> 
> > 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.
> >
> 
> I approached this from the other side and didn't like that annot_module_include is handled so differently from @import.
> 
> I think they're fundamentally different, at least currently:
> 
> #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).
> 
> 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.

Sounds like you know much more about this than I do :) 

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?’

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.

> 
> > I'm also not sure that the diagnostic change here is an improvement. Previously:
> >
> > #include "not_module.h"
> > #include "module.h"
> >
> > ... where not_module.h contains:
> >
> > class X {
> >   // ...
> > // whoops, forgot the '};'
> >
> > 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.
> 
> Ick, I’ll take a look at this case.
> 
> >
> > In what cases does this change allow us to recover from errors better?
> 
> 
> 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.
> 
> 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.

I’ll look into this.

>  
> 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.
> 
> int foo() {
>   int x = 5;
>   #import “Bar.h” // expected }
>   if (x == 6) {// expected identifier or (
>     ++x; // expected identifier or (
>   }
>   return 1; // expected identifier of (
> } // extraneous closing brace
> 
> 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.

This was prompted by code like the following being spotted "in the wild" (where the actual header is in a system module):

@implementation SomeClass
#import <system/header.h>
// … more stuff here
@end

I have no data to support it being a common case, but I’d like to improve the error if possible.

> 
> 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).

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?

Ben
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140205/439bc406/attachment.html>