r240158 - Check for consistent use of nullability type specifiers in a header.

[Aaron Ballman]

On Tue, Jun 23, 2015 at 5:48 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Jun 20, 2015, at 12:48 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Fri, Jun 19, 2015 at 2:27 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> Author: dgregor
> Date: Fri Jun 19 13:27:45 2015
> New Revision: 240158
>
> URL: http://llvm.org/viewvc/llvm-project?rev=240158&view=rev
> Log:
> Check for consistent use of nullability type specifiers in a header.
>
> Adds a new warning (under -Wnullability-completeness) that complains
> about pointer, block pointer, or member pointer declarations that have
> not been annotated with nullability information (directly or inferred)
> within a header that contains some nullability annotations. This is
> intended to be used to help maintain the completeness of nullability
> information within a header that has already been audited.
>
> Note that, for performance reasons, this warning will underrepresent
> the number of non-annotated pointers in the case where more than one
> pointer is seen before the first nullability type specifier, because
> we're only tracking one piece of information per header. Part of
> rdar://problem/18868820.
>
> Added:
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-1.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-2.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-3.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-4.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-5.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-6.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-7.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-8.h   (with
> props)
>    cfe/trunk/test/SemaObjCXX/nullability-consistency.mm
> Modified:
>    cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>    cfe/trunk/include/clang/Sema/AttributeList.h
>    cfe/trunk/include/clang/Sema/Sema.h
>    cfe/trunk/lib/Parse/ParseObjc.cpp
>    cfe/trunk/lib/Sema/SemaType.cpp
>    cfe/trunk/test/SemaObjCXX/Inputs/nullability-pragmas-1.h
>    cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=240158&r1=240157&r2=240158&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Fri Jun 19 13:27:45
> 2015
> @@ -251,6 +251,7 @@ def NewlineEOF : DiagGroup<"newline-eof"
> def Nullability : DiagGroup<"nullability">;
> def NullabilityDeclSpec : DiagGroup<"nullability-declspec">;
> def NullableToNonNullConversion :
> DiagGroup<"nullable-to-nonnull-conversion">;
> +def NullabilityCompleteness : DiagGroup<"nullability-completeness">;
> def NullArithmetic : DiagGroup<"null-arithmetic">;
> def NullCharacter : DiagGroup<"null-character">;
> def NullDereference : DiagGroup<"null-dereference">;
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=240158&r1=240157&r2=240158&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Jun 19 13:27:45
> 2015
> @@ -7718,6 +7718,11 @@ def warn_null_resettable_setter : Warnin
>   "synthesized setter %0 for null_resettable property %1 does not handle
> nil">,
>   InGroup<Nullability>;
>
> +def warn_nullability_missing : Warning<
> +  "%select{pointer|block pointer|member pointer}0 is missing a nullability
> "
> +  "type specifier (__nonnull, __nullable, or __null_unspecified)">,
> +  InGroup<NullabilityCompleteness>;
> +
> }
>
> } // end of sema component.
>
> Modified: cfe/trunk/include/clang/Sema/AttributeList.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/AttributeList.h?rev=240158&r1=240157&r2=240158&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/AttributeList.h (original)
> +++ cfe/trunk/include/clang/Sema/AttributeList.h Fri Jun 19 13:27:45 2015
> @@ -81,6 +81,8 @@ public:
>     AS_Declspec,
>     /// __ptr16, alignas(...), etc.
>     AS_Keyword,
> +    /// Context-sensitive version of a keyword attribute.
> +    AS_ContextSensitiveKeyword,
>
>
> I don't like this approach. These are meant to mirror spellings
> generated by tablegen that the user can write, so this breaks the
> attribute syntax model. Put slightly differently, as far as the table
> definition for an attribute is concerned, *all* keywords are currently
> context sensitive in that we don't (and likely will never) table
> generate parsing for attribute keywords. So this provides no useful
> distinction for the table definition of keyword attributes.
>
>
> The tablgen-generated table we’re talking about generates part of the
> documentation; it’s not some deep parser generation logic where the lack of
> information about the potential contexts for the keyword is signifiant.

The table-generated table I'm talking about is used in a dozen
different ways, including support for user-facing functionality like
__has_attribute. This definitely is significant, to me.

> We
> generally treat keywords and context-sensitive keywords the same way
> throughout, so using the syntax kind to describe this syntactic distinction
> seems totally reasonable to me.

I agree that we generally treat them the same way, that's why I was
asking you to remove the syntax kind for it. To me, there's no reason
we should have AS_Keyword *and* AS_ContextSensitiveKeyword, and
including it is a step backwards because we now have this bizarre
difference the user doesn't actually care about. They spelled the
attribute a particular way, and if it's one keyword vs a different
keyword, they don't care so long as the diagnostics display what they
wrote (it pretty prints properly, etc). None of that requires such a
distinction as to contextual or not, from what I can tell.

> From what I can tell, context sensitivity is only used for error
> reporting purposes (whether we want to print __nonnull or nonnull in a
> diagnostic). That's part of the spelling of the attribute itself,
> which AttributeList already tracks. Can that be used instead? If not,
> the previous approach was marginally less heinous (though still
> problematic).
>
>
> To move this information into the spelling of the attribute itself, we’ll
> need to either push the hack somewhere else (AttributeList::getKind) or
> admit context-sensitivity further into TableGen with a
> ContextSensitiveKeyword<“nonnull”> spelling. Neither seems like progress to
> me.

Spelling information is already a part of a parsed attribute (hence
getName() and getScopeName() mixed with isFooAttribute()).

To me, the AttributeList should represent the parsed information at
hand. We shouldn't have to reconstitute things like how an attribute
is identified based on the spelling and whether it was contextual or
not. We should have that information in getName() already, and tack on
the fact that it was contextual "on the side" if we ever need it
(which we currently don't). To me, the current approach is the hack.
When you attach the attribute information by creating an AttributeList
object, you should just be passing in AS_Keyword as the syntax kind,
and the identifier the user wrote as the name.

Basically, what I am suggesting is to not hard-code the attribute
identifier as part of the diagnostic, and instead pass in the
attribute's name when calling checkNullabilityTypeSpecifier. Then it
no longer matters whether it's context sensitive or not, the name in
the diagnostic will be whatever the user wrote. We may run into an
issue where the attributed type object doesn't carry sufficient
information about what the user wrote, but that's a separate issue
(and one we should solve regardless as it is a general problem we have
with type attributes).

~Aaron