Page MenuHomePhabricator

Warning for framework headers using double quote includes
ClosedPublic

Authored by bruno on May 21 2018, 1:21 PM.

Details

Summary

Introduce -Wquoted-include-in-framework-header, which should fire a warning
whenever a quote include appears in a framework header and suggest a fix-it.
For instance, for header A.h added in the tests, this is how the warning looks
like:

./A.framework/Headers/A.h:2:10: warning: double-quoted include "A0.h" in framework header, expected angle-bracketed instead [-Wquoted-include-in-framework-header]
#include "A0.h"
         ^~~~~~
         <A/A0.h>
./A.framework/Headers/A.h:3:10: warning: double-quoted include "B.h" in framework header, expected angle-bracketed instead [-Wquoted-include-in-framework-header]
#include "B.h"
         ^~~~~
         <B.h>

This helps users to prevent frameworks from using local headers when in fact
they should be targetting system level ones.

The warning is off by default.

rdar://problem/37077034

Diff Detail

Repository
rL LLVM

Event Timeline

bruno created this revision.May 21 2018, 1:21 PM

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

bruno added a comment.May 21 2018, 3:40 PM

See also PR22165.

Nice, seems related to this indeed. Are you aware of any development along those lines in clang-tidy? We would like this to be part of clang and be used as part of the normal compiler workflow, it can surely be as well part of any clang-tidy related include guidelines in the future.

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

Right. I believe this is going to be used in practice, the reason I'm adding it involves some user demand for such warning. Such quoted include use in frameworks happen often and we would like a smooth transition to happen here (e.g. do not initially affect -Werror users). If it proves worth it, we can migrate to on by default in the future. It wouldn't be a problem if we have it on by default on open source and disable by default downstream, but I rather be consistent.

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

Right. I believe this is going to be used in practice, the reason I'm adding it involves some user demand for such warning. Such quoted include use in frameworks happen often and we would like a smooth transition to happen here (e.g. do not initially affect -Werror users). If it proves worth it, we can migrate to on by default in the future. It wouldn't be a problem if we have it on by default on open source and disable by default downstream, but I rather be consistent.

You could just not include new warning into -Wall and -Wextra. Those who will want to check #include statements correctness could enable it explicitly.

bruno added a comment.May 23 2018, 4:45 PM

Hi Eugene,

You could just not include new warning into -Wall and -Wextra. Those who will want to check #include statements correctness could enable it explicitly.

This is exactly what's happening in the patch, the new warning isn't part of -Wall or -Wextra, and is marked DefaultIgnore, which means that will fire only when -Wquoted-include-in-framework-header is passed to the driver. Am I missing something from your explanation?

Thanks,

Hi Eugene,

You could just not include new warning into -Wall and -Wextra. Those who will want to check #include statements correctness could enable it explicitly.

This is exactly what's happening in the patch, the new warning isn't part of -Wall or -Wextra, and is marked DefaultIgnore, which means that will fire only when -Wquoted-include-in-framework-header is passed to the driver. Am I missing something from your explanation?

Thanks,

Thank you for clarification! Sorry, I didn't know TableGen syntax well enough to deduce this from source.

See also PR22165.

Nice, seems related to this indeed. Are you aware of any development along those lines in clang-tidy? We would like this to be part of clang and be used as part of the normal compiler workflow, it can surely be as well part of any clang-tidy related include guidelines in the future.

The warning is off by default.

We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?

Right. I believe this is going to be used in practice, the reason I'm adding it involves some user demand for such warning. Such quoted include use in frameworks happen often and we would like a smooth transition to happen here (e.g. do not initially affect -Werror users). If it proves worth it, we can migrate to on by default in the future. It wouldn't be a problem if we have it on by default on open source and disable by default downstream, but I rather be consistent.

Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?

include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

80-col limit?

Also, I'd probably drop "system style" and reword slightly to:

"double-quoted include \"%0\" in framework header, expected angle-bracketed include <%0> instead"

lib/Lex/HeaderSearch.cpp
753–754 ↗(On Diff #147851)

This seems like a good place for a fix-it to switch the include style. Is there a reason to not do that work for the user?

873–874 ↗(On Diff #147851)

Similar here.

bruno added a comment.May 29 2018, 3:26 PM

Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?

I believe this could be too disruptive to enable now, since it's still very common to find quoted includes in framework headers. This is very important for Modules to properly work with frameworks (quoted headers are usually considered non-modular when modules builds kick in) and is actually very compelling for us to turn it on by default on Darwin as soon as we can, but we need to educate users first.

include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

Unfortunately this won't work because for framework style includes we use the angled-bracketed with the framework name. For example, if one wants to include Foo.h from Foo.framework, one should use #include <Foo/Foo.h>, although on disk you actually have Foo.framework/Headers/Foo.h. Framework header lookup does this magic and other similar ones.

Since we don't know which framework the quoted header could be part of, it was not included in the warning (doing so would require extra header searches - which could be expensive for this specific warning). However it seems that I can do better to indicate that the framework name is desired here, perhaps:

"double-quoted include \"%0\" in framework header, expected angle-bracketed include <FrameworkName/%0> instead"

How does that sound to you? Other suggestions?

lib/Lex/HeaderSearch.cpp
753–754 ↗(On Diff #147851)

Like I explained above, we don't know which framework the header could be part of, so a fix-it could be misleading.

dexonsmith added inline comments.May 29 2018, 3:38 PM
lib/Lex/HeaderSearch.cpp
753–754 ↗(On Diff #147851)

Clang supports editor placeholders, which we use in some refactoring-style fix-its. I think this would be spelled <#framework-name#>, or #include <<#framework-name#>/Foo.h>

Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?

I believe this could be too disruptive to enable now, since it's still very common to find quoted includes in framework headers. This is very important for Modules to properly work with frameworks (quoted headers are usually considered non-modular when modules builds kick in) and is actually very compelling for us to turn it on by default on Darwin as soon as we can, but we need to educate users first.

That sounds like good justification for it being off by default then, thank you.

include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

Thank you for the explanation!

I think your suggested text sounds good, though I do wonder how expensive is "expensive" in finding the intended header? Not only would it provide a better diagnostic, it would also let you use a fixit that doesn't use editor placeholders.

bruno added a subscriber: arphaman.May 30 2018, 3:03 PM
bruno added inline comments.
lib/Lex/HeaderSearch.cpp
753–754 ↗(On Diff #147851)

My current understanding (after chatting with @arphaman) is that editor placeholders isn't a great fit here:

  • For non IDE uses of this, it will just be weird to output something like #include <<#framework-name#>/Foo.h>. Even if we wanted to emit this only for IDE use, clang currently has no way to make that distinction (editor placeholder related compiler flags only make sense when actually making the special token sequence lexable, not when generating it)
  • Fixits are (with some known exceptions) meant to be applied to code and subsequently allow compilation to succeed, this wouldn't be the case here.
dexonsmith added inline comments.May 30 2018, 3:12 PM
include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

I'm also interested in just how expensive it would be, because I think users will be frustrated that the compiler knows it's a framework include "so it obviously knows which framework".

I'd be fine if the fix-it came in a follow-up commit though (not sure how Aaron feels).

lib/Lex/HeaderSearch.cpp
753–754 ↗(On Diff #147851)

Okay, sounds good.

bruno added inline comments.May 30 2018, 3:18 PM
include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

I haven't measured, but for each quoted include we would have to:

  • Start a fresh header search.
  • Look for Foo.h in all possible frameworks in the path (just on the Darwin macOS SDK path that would be around 140 frameworks).
  • If it's only found in once place, we are mostly safe to say we found a matching framework, otherwise we can't emit a reliable fixit.
  • Header maps and VFS might add extra level of searches (this is very common in Xcode based clang invocations).
dexonsmith added inline comments.May 30 2018, 3:23 PM
include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

Can we just check if it's a header in the *same* framework?

bruno added inline comments.May 30 2018, 3:46 PM
include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

For some pretty obvious cases we can probably assume that this is what the user wants, but even so it might be misleading. For example, if you're in ABC/H1.h, you include H2.h and the framework ABC has an ABC/H2.h. It could be that #include "H2.h" is mapped via header maps to $SOURCE/H2.h instead of using the installed headers in the framework style build products.

This is likely a mistake, but what if it's intentional? I would prefer if the user rethink it instead of just buying potential misleading clues. OTOH, I share the concern that we don't need to be perfect here and only emit the fixit for really obvious cases, and not for the others. Will update the patch to include a fixit to the very straightforward scenario: H2.h was found in the same framework style path as the includer.

aaron.ballman added inline comments.May 31 2018, 4:31 AM
include/clang/Basic/DiagnosticLexKinds.td
713 ↗(On Diff #147851)

Will update the patch to include a fixit to the very straightforward scenario: H2.h was found in the same framework style path as the includer.

Thanks, I think users will appreciate the compiler helping them out like that.

lib/Lex/HeaderSearch.cpp
753–754 ↗(On Diff #147851)

That makes sense to me.

bruno updated this revision to Diff 149313.May 31 2018, 10:45 AM

Updated the patch after Duncan and Aaron reviews. I actually went a bit more aggressive with the fixits, since I realized the conditions for the warning are already strict enough and we should take the chance to be more clear. For the attached testcase, the output now is:

./A.framework/Headers/A.h:2:10: warning: double-quoted include "A0.h" in framework header, expected angle-bracketed include <A/A0.h> instead
#include "A0.h"
         ^~~~~~
         <A/A0.h>
./A.framework/Headers/A.h:3:10: warning: double-quoted include "B.h" in framework header, expected angle-bracketed include <B.h> instead
#include "B.h"
         ^~~~~
         <B.h>
dexonsmith added inline comments.May 31 2018, 11:32 AM
test/Modules/double-quotes.m
24–25 ↗(On Diff #149313)

Would using an explicit module build make this any easier?

27–29 ↗(On Diff #149313)

When there's a fixit, you don't need to list it in the warning text (the fix-it itself is sufficient). I also feel like "quoted include" is as clear as "double-quoted include" (but more succinct). So I think these would be better as:

warning: quoted include "..." in framework header, expected angle-bracketed include instead

aaron.ballman added inline comments.May 31 2018, 11:38 AM
test/Modules/double-quotes.m
27–29 ↗(On Diff #149313)

Some other lexer diagnostics use "double-quoted" when they want to distinguish with "angle-bracketed" (see warn_pragma_include_alias_mismatch_angle and warn_pragma_include_alias_mismatch_quote as examples). I don't have a strong opinion on what form we use, but I'd prefer for it to be consistent exposition.

dexonsmith added inline comments.May 31 2018, 12:40 PM
test/Modules/double-quotes.m
27–29 ↗(On Diff #149313)

I agree we should be consistent. No reason to change it here then.

This revision is now accepted and ready to land.May 31 2018, 12:46 PM
bruno updated this revision to Diff 149359.May 31 2018, 2:40 PM
bruno edited the summary of this revision. (Show Details)

Update after Duncan's review: remove header name from the warning message (since it's already in the fixit)

test/Modules/double-quotes.m
24–25 ↗(On Diff #149313)

Not really =T

This revision was automatically updated to reflect the committed changes.