Prevent module import inside @implementation

[Richard Smith]

On Wed, Feb 5, 2014 at 3:00 PM, Ben Langmuir <blangmuir at apple.com> wrote:

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

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:

@import X.b;

... then that @import is a no-op. If it had been

#include "b.h"

we would have built the submodule X.b immediately.
See test/Modules/Inputs/submodules/import-self-b.h for a complete example.

I suspect this is a bug =)

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

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.

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.

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

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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140205/3fc85ebd/attachment.html>