[PATCH] Fixit for incorrect includes

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?

~Aaron

include_fixit.patch (2 KB)

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

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.

[& this makes me wonder: how scary (would it even be
possible/practical?) would it be to do typo correction on #includes?
Though we should check if it's worthwhile first - I wonder how often
people make typos there]

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, then copy the temp back
(restoring the original contents).

[& this makes me wonder: how scary (would it even be
possible/practical?) would it be to do typo correction on #includes?
Though we should check if it's worthwhile first - I wonder how often
people make typos there]

I was wondering the same thing. For instance, one (perhaps) simple
extension would be to try a case insensitive search for the file and
suggest the proper casing if the file cannot be found. This is
especially interesting to x-platform developers where OS X and *nix
are generally case sensitive files, and Windows is generally case
insensitive.

~Aaron

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)

& yeah, arguable whether the existing fixit cases should be burdened
with this mucking about - I don't mind too strongly either way.

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, I figured this was a clean separation.
I'm not strongly tied to it though.

New patch attached.

~Aaron

include_fixit.patch (2.01 KB)

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. So if this is possible to "fixit",
does that mean this really shouldn't be a fatal error in this case?

~Aaron

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.

- David

That seems like a larger change than I was hoping for. What are the
consequences of this? Do we want to have a fatal and non-fatal
variant of the error (fatal for when no fixit is available)?

~Aaron

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.

The non-fixit and fixit diagnostics would probably have different text anyway. “Header not found” and “Header not found as written, but found if looked up as if it was written with quotes” are pretty different.

Sebastian

Would it be fine to use "file not found as written"? I ask because I
can see a second fixit coming pertaining to spelling or case
sensitivity.

~Aaron

This patch includes suggestions from David and Sebastian in terms of
functionality and wording.

~Aaron

include_fixit.patch (2.92 KB)

I love this Fix-It, but I'd like the diagnostic to be more specific:

Index: include/clang/Basic/DiagnosticLexKinds.td

Thanks (to everyone) for the reviews -- I've committed in r160406 with
Doug's suggestions.

~Aaron