[cfe-dev] [PATCH] Fixit for incorrect includes

[David Blaikie]

On Tue, Jul 17, 2012 at 11:46 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Tue, Jul 17, 2012 at 1:39 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> On Tue, Jul 17, 2012 at 11:35 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>> On Tue, Jul 17, 2012 at 1:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>> On Tue, Jul 17, 2012 at 11:01 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>> On Tue, Jul 17, 2012 at 12:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>> On Tue, Jul 17, 2012 at 10:48 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>>>> On Tue, Jul 17, 2012 at 12:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>>> On Tue, Jul 17, 2012 at 9:14 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>>>>>>>>> This patch creates a fixit for include directives where the file could
>>>>>>>>> not be found when using angle brackets, but can be found when using
>>>>>>>>> quotes.  The converse is not needed since quoted includes will search
>>>>>>>>> angle bracket locations by default.
>>>>>>>>>
>>>>>>>>> Eg)
>>>>>>>>> #include <header.h>  // can be found via #include "header.h" instead
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>
>>>>>>>> Seems like a neat idea to me - but I'm not an authoritative sign-off.
>>>>>>>>
>>>>>>>> You used NULL as null constants for 2 of the conditional operators,
>>>>>>>> then 0 for the third - that seems inconsistent. You might want to
>>>>>>>> check what the prevailing style is in this file & stick to that
>>>>>>>> (generally in LLVM, '0' seems to be winning as the authoritative null
>>>>>>>> pointer constant, I believe).
>>>>>>>
>>>>>>> Easy enough to rectify (I'll standardize on 0) -- this was a copy from
>>>>>>> above, so I'll fix up there as well.
>>>>>>>
>>>>>>>> Did you consider adding this case to the existing fixit testing files?
>>>>>>>> They're already a grab-bag of things that can be fixed (this helps
>>>>>>>> keep the test suite fast by not adding more separate test file
>>>>>>>> executions) & this seems like it'd be at home there.
>>>>>>>
>>>>>>> I thought about it, but it strikes me as different enough to warrant
>>>>>>> its own test case.  Specifically, the fixit behavior is kind of
>>>>>>> tricky.  Because the fixit writes out a separate file, but in a
>>>>>>> different directory, I have to be a bit sneaky otherwise the header
>>>>>>> file isn't available.  This is because the temp file goes to the
>>>>>>> Output directory, but the header file remains in the FixIt directory.
>>>>>>> To work around this, I copy the testcase to a temp file, modify the
>>>>>>> testcase file directly with the fixit,
>>>>>>
>>>>>> Why can't you modify the temp file directly instead? (& you'd have to
>>>>>> copy the header over too, I suppose - you can identify the temp
>>>>>> directory with %T which means you can copy to fixed name files if
>>>>>> that's important (as it would be for the header name): %T/foo.cpp or
>>>>>> whatever)
>>>>>
>>>>> You learn something new every day!  I didn't realize you could use %T
>>>>> for the temp directory.  Now the testcase looks like:
>>>>>
>>>>> // RUN: %clang_cc1 -fsyntax-only -verify %s
>>>>> // RUN: cp %s %t
>>>>> // RUN: cp %S/fixit-include.h %T
>>>>> // RUN: not %clang_cc1 -fsyntax-only -fixit %t
>>>>> // RUN: %clang_cc1 -Wall -pedantic %t
>>>>>
>>>>> Which is cleaner (to me).  Thanks!
>>>>>
>>>>>> & yeah, arguable whether the existing fixit cases should be burdened
>>>>>> with this mucking about - I don't mind too strongly either way.
>>>>>
>>>>> Since include errors are fatal,
>>>>
>>>> This actually raises a good question/point: when Clang encounters
>>>> fatal errors it basically stops, right? (it doesn't produce more
>>>> diagnostics) That's incorrect if we've provided a fixit and recovered
>>>> from the error - when fixits are provided we should recover as if the
>>>> code were written that way. With your change as it stands we
>>>> "successfully" apply fixits for this code:
>>>>
>>>> #include <non_existent.h>
>>>> int main(float) {
>>>> }
>>>>
>>>> because we didn't provide the error about main. What we should get is
>>>> the error about main and no fixit application because we encountered
>>>> an unfixable error.
>>>
>>> I'm not actually recovering with my patch -- I'm still early returning
>>> out of HandleIncludeDirective.
>>
>> Yep, that's probably incorrect. If Clang issues a fixit, it
>> should/must recover as if the user wrote the code the way the fixit
>> suggests.
>>
>>> So if this is possible to "fixit",
>>> does that mean this really shouldn't be a fatal error in this case?
>>
>> I believe so, yes.
>
> That seems like a larger change than I was hoping for.  What are the
> consequences of this?

In this particular case? I'm not entirely sure - but assuming the
lookup happens correctly, etc - I don't see any reason it'd be
fundamentally wrong/impossible.

> Do we want to have a fatal and non-fatal
> variant of the error (fatal for when no fixit is available)?

Yes, I believe that'd be necessary. They can have different diagnostic
names but the same text and just one of them is non-fatal.