Give NullabilityKind a printing operator<<

[sammccall]

Summary

This is more useful for debug/test than getNullabilitySpelling:

• default form has uglifying underscores
• non-default form crashes on NullableResult
• both return unhelpfully verbose strings for Unspecified
• operator<< works with gtest, formatv, etc

[sammccall]

clang/lib/Basic/IdentifierTable.cpp

852

IdentifierTable.cpp is a weird place to implement this function (I put it next to getNullabilitySpelling).
Happy to move to a different file, or add a new file, if you think either is significantly better.

Similarly, happy to add a test for this if it's worth having a unittest with only this (obviously nothing else in Specifiers is tested)

[aaron.ballman]

I guess I'm not seeing much motivation for this change. We already have clang::getNullabilitySpelling() and const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB, DiagNullabilityKind nullability) and now we're adding a third way to get this information. If this is just for debug/testing purposes, can we use existing debug formatters to convert the enumeration value into the enumerator name instead?

[sammccall]

In D149650#4312389, @aaron.ballman wrote:

I guess I'm not seeing much motivation for this change. We already have clang::getNullabilitySpelling() and const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB, DiagNullabilityKind nullability) and now we're adding a third way to get this information. If this is just for debug/testing purposes, can we use existing debug formatters to convert the enumeration value into the enumerator name instead?

We're using NullabilityKind in our dataflow-based null-safety clang-tidy check that we hope to upstream when it works.
(The idea is to use _Nullable and _Nonnull annotations on API boundaries to gradually type pointers, and to provide a verification clang-tidy check and libraries to infer annotations for existing code. The actual clang-tidy check wrapper isn't in that repo yet, but it's trivial).

It would be convenient if e.g. EXPECT_THAT(someVectorOfNK, ElementsAre(Nullable, Unspecified)) printed something useful when it fails, if we could write OS << NK and get Unspecified rather than OS << getSpelling(NK) and get _Unspecified_Nullability, etc.
This doesn't concern clang core (ha, there are no unit tests...) but there's no reasonable way to do this downstream without using a different type.

StreamingDiagnostic &clang::operator<<

This actually wants the user-visible spelling, so I think it can just use getSpelling... if I make that change we're back to just two implementations :-)

[sammccall]

Make diagnostic-printing use the (other) existing function, come out neutral!

(This adds quotes to some diagnostic, but their omission seems to be an error)

[aaron.ballman]

In D149650#4315211, @sammccall wrote:

In D149650#4312389, @aaron.ballman wrote:

I guess I'm not seeing much motivation for this change. We already have clang::getNullabilitySpelling() and const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB, DiagNullabilityKind nullability) and now we're adding a third way to get this information. If this is just for debug/testing purposes, can we use existing debug formatters to convert the enumeration value into the enumerator name instead?

We're using NullabilityKind in our dataflow-based null-safety clang-tidy check that we hope to upstream when it works.

Ah, good to know! I think it would be reasonable to add this functionality once there's some agreement on adding the clang-tidy check.

(The idea is to use _Nullable and _Nonnull annotations on API boundaries to gradually type pointers, and to provide a verification clang-tidy check and libraries to infer annotations for existing code. The actual clang-tidy check wrapper isn't in that repo yet, but it's trivial).

It would be convenient if e.g. EXPECT_THAT(someVectorOfNK, ElementsAre(Nullable, Unspecified)) printed something useful when it fails, if we could write OS << NK and get Unspecified rather than OS << getSpelling(NK) and get _Unspecified_Nullability, etc.
This doesn't concern clang core (ha, there are no unit tests...) but there's no reasonable way to do this downstream without using a different type.

Hmmm, but does this need to be added to Specifiers.h instead of being kept packaged up with clang-tidy? For things like AST matchers, we usually ask that the matcher remain local to the clang-tidy check unless the same matcher is needed in multiple checks. This feels somewhat similar, despite it not being related to AST matching -- if the only need for this functionality exists out of the clang tree, can we keep it out of the clang tree until we find more needs?

StreamingDiagnostic &clang::operator<<

This actually wants the user-visible spelling, so I think it can just use getSpelling... if I make that change we're back to just two implementations :-)

Oh, I really like those changes! Feel free to land that as an NFC change if you'd like (the addition of quotes is a bug fix, but not really a functional change).

[sammccall]

In D149650#4316138, @aaron.ballman wrote:

In D149650#4315211, @sammccall wrote:

In D149650#4312389, @aaron.ballman wrote:

I guess I'm not seeing much motivation for this change. We already have clang::getNullabilitySpelling() and const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB, DiagNullabilityKind nullability) and now we're adding a third way to get this information. If this is just for debug/testing purposes, can we use existing debug formatters to convert the enumeration value into the enumerator name instead?

We're using NullabilityKind in our dataflow-based null-safety clang-tidy check that we hope to upstream when it works.

Ah, good to know! I think it would be reasonable to add this functionality once there's some agreement on adding the clang-tidy check.

(The idea is to use _Nullable and _Nonnull annotations on API boundaries to gradually type pointers, and to provide a verification clang-tidy check and libraries to infer annotations for existing code. The actual clang-tidy check wrapper isn't in that repo yet, but it's trivial).

It would be convenient if e.g. EXPECT_THAT(someVectorOfNK, ElementsAre(Nullable, Unspecified)) printed something useful when it fails, if we could write OS << NK and get Unspecified rather than OS << getSpelling(NK) and get _Unspecified_Nullability, etc.
This doesn't concern clang core (ha, there are no unit tests...) but there's no reasonable way to do this downstream without using a different type.

Hmmm, but does this need to be added to Specifiers.h instead of being kept packaged up with clang-tidy? For things like AST matchers, we usually ask that the matcher remain local to the clang-tidy check unless the same matcher is needed in multiple checks. This feels somewhat similar, despite it not being related to AST matching -- if the only need for this functionality exists out of the clang tree, can we keep it out of the clang tree until we find more needs?

This can't be added downstream, the type needs to provide these.
ADL extension points like operators need to be in namespace clang[1] so that ADL will find them. If non-owners of the type define these, this ends up in violating ODR when two such non-owners are linked together. Hacking around this for the operator<< symbol itself (e.g. putting it inline in anonymous namespaces) results in ODR violations in templates that call them (in this case, gtest). Even defining them centrally in a separate header is risky, including/not including the header produces similar violations.

There's never going to be a hard need for this - having an ergonomic way to print things that composes with the rest of our infra is useful but it's always possible to live without it.
If we don't want such clang-as-a-library features upstream that's OK. We can define our own NullabilityKind and marshal between them - I need to be able to answer "why are we hacking around clang rather than improving it" in code review :-)

[1] (or llvm, or ::, or ::testing - these all have the same issues).

StreamingDiagnostic &clang::operator<<

This actually wants the user-visible spelling, so I think it can just use getSpelling... if I make that change we're back to just two implementations :-)

Oh, I really like those changes! Feel free to land that as an NFC change if you'd like (the addition of quotes is a bug fix, but not really a functional change).

Thanks - will do!

[aaron.ballman]

In D149650#4319461, @sammccall wrote:

In D149650#4316138, @aaron.ballman wrote:

In D149650#4315211, @sammccall wrote:

In D149650#4312389, @aaron.ballman wrote:

I guess I'm not seeing much motivation for this change. We already have clang::getNullabilitySpelling() and const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB, DiagNullabilityKind nullability) and now we're adding a third way to get this information. If this is just for debug/testing purposes, can we use existing debug formatters to convert the enumeration value into the enumerator name instead?

We're using NullabilityKind in our dataflow-based null-safety clang-tidy check that we hope to upstream when it works.

Ah, good to know! I think it would be reasonable to add this functionality once there's some agreement on adding the clang-tidy check.

(The idea is to use _Nullable and _Nonnull annotations on API boundaries to gradually type pointers, and to provide a verification clang-tidy check and libraries to infer annotations for existing code. The actual clang-tidy check wrapper isn't in that repo yet, but it's trivial).

It would be convenient if e.g. EXPECT_THAT(someVectorOfNK, ElementsAre(Nullable, Unspecified)) printed something useful when it fails, if we could write OS << NK and get Unspecified rather than OS << getSpelling(NK) and get _Unspecified_Nullability, etc.
This doesn't concern clang core (ha, there are no unit tests...) but there's no reasonable way to do this downstream without using a different type.

Hmmm, but does this need to be added to Specifiers.h instead of being kept packaged up with clang-tidy? For things like AST matchers, we usually ask that the matcher remain local to the clang-tidy check unless the same matcher is needed in multiple checks. This feels somewhat similar, despite it not being related to AST matching -- if the only need for this functionality exists out of the clang tree, can we keep it out of the clang tree until we find more needs?

This can't be added downstream, the type needs to provide these.
ADL extension points like operators need to be in namespace clang[1] so that ADL will find them. If non-owners of the type define these, this ends up in violating ODR when two such non-owners are linked together. Hacking around this for the operator<< symbol itself (e.g. putting it inline in anonymous namespaces) results in ODR violations in templates that call them (in this case, gtest). Even defining them centrally in a separate header is risky, including/not including the header produces similar violations.

Oh shoot, you're right. I hadn't thought about the fact that this is an ADL extension point specifically.

There's never going to be a hard need for this - having an ergonomic way to print things that composes with the rest of our infra is useful but it's always possible to live without it.
If we don't want such clang-as-a-library features upstream that's OK. We can define our own NullabilityKind and marshal between them - I need to be able to answer "why are we hacking around clang rather than improving it" in code review :-)

I think you've convinced me this is reasonable to keep upstream despite not really being used in tree (yet). If it turns out that the clang-tidy check doesn't get upstreamed, we can remove this easily enough in the future if needed. I don't think we want to expose this sort of thing as a matter of course, but when the alternatives are significantly more dangerous, it is reasonable. If we find we're adding more clang-as a-library features upstream like this, we might want to revisit whether there's a better design approach for these needs.

LG!

[sammccall]

Thanks Aaron!

The check is not going to be ready for upstreaming really soon - plenty of thorny issues to work out first, and checking in a half-working version would do more harm than good.
But that's definitely the goal once we have some deployment experience.

[cmtice]

This is breaking some of our tests. I will give the author a reproducible test case. I am going to revert this while waiting for a fix.

clang/include/clang/Basic/Specifiers.h

 
 #ifndef LLVM_CLANG_BASIC_SPECIFIERS_H
 #define LLVM_CLANG_BASIC_SPECIFIERS_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/ErrorHandling.h"
 
+namespace llvm {
+class raw_ostream;
+} // namespace llvm
 namespace clang {
 
   /// Define the meaning of possible values of the kind in ExplicitSpecifier.
   enum class ExplicitSpecKind : unsigned {
     ResolvedFalse,
     ResolvedTrue,
     Unresolved,
   };
enum class NullabilityKind : uint8_t {
     /// though it has been considered.
     Unspecified,
     // Generally behaves like Nullable, except when used in a block parameter
     // that was imported into a swift async method. There, swift will assume
     // that the parameter can get null even if no error occurred. _Nullable
     // parameters are assumed to only get null on error.
     NullableResult,
   };
+  /// Prints human-readable debug representation.
+  llvm::raw_ostream &operator<<(llvm::raw_ostream&, NullabilityKind);
 
   /// Return true if \p L has a weaker nullability annotation than \p R. The
   /// ordering is: Unspecified < Nullable < NonNull.
   inline bool hasWeakerNullability(NullabilityKind L, NullabilityKind R) {
     return uint8_t(L) > uint8_t(R);
   }
 
   /// Retrieve the spelling of the given nullability kind.

clang/lib/Basic/Diagnostic.cpp

 #include <string>
 #include <utility>
 #include <vector>
 
 using namespace clang;
 
 const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
                                              DiagNullabilityKind nullability) {
-  StringRef string;
-  switch (nullability.first) {
-  case NullabilityKind::NonNull:
-    string = nullability.second ? "'nonnull'" : "'_Nonnull'";
-    break;
-
-  case NullabilityKind::Nullable:
-    string = nullability.second ? "'nullable'" : "'_Nullable'";
-    break;
-
-  case NullabilityKind::Unspecified:
-    string = nullability.second ? "'null_unspecified'" : "'_Null_unspecified'";
-    break;
-
-  case NullabilityKind::NullableResult:
-    assert(!nullability.second &&
-           "_Nullable_result isn't supported as context-sensitive keyword");
-    string = "_Nullable_result";
-    break;
-  }
-
-  DB.AddString(string);
+  DB.AddString(
+      ("'" +
+       getNullabilitySpelling(nullability.first,
+                              /*isContextSensitive=*/nullability.second) +
+       "'")
+          .str());
   return DB;
 }
 
 const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
                                              llvm::Error &&E) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }

clang/lib/Basic/IdentifierTable.cpp

case NullabilityKind::NullableResult:
     return "_Nullable_result";
 
   case NullabilityKind::Unspecified:
     return isContextSensitive ? "null_unspecified" : "_Null_unspecified";
   }
   llvm_unreachable("Unknown nullability kind.");
 }
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, NullabilityKind NK) {
+  switch (NK) {
+  case NullabilityKind::NonNull:
+    return OS << "NonNull";
+  case NullabilityKind::Nullable:
+    return OS << "Nullable";
+  case NullabilityKind::NullableResult:
+    return OS << "NullableResult";
+  case NullabilityKind::Unspecified:
+    return OS << "Unspecified";
+  }
+  llvm_unreachable("Unknown nullability kind.");
+}
+
 diag::kind
 IdentifierTable::getFutureCompatDiagKind(const IdentifierInfo &II,
                                          const LangOptions &LangOpts) {
   assert(II.isFutureCompatKeyword() && "diagnostic should not be needed");
 
   unsigned Flags = llvm::StringSwitch<unsigned>(II.getName())
 #define KEYWORD(NAME, FLAGS) .Case(#NAME, FLAGS)
 #include "clang/Basic/TokenKinds.def"

clang/test/SemaObjC/nullable-result.m

void test_conversion(void) {
   X *_Nonnull l = nr; // expected-warning{{implicit conversion from nullable pointer 'X * _Nullable_result' to non-nullable pointer type 'X * _Nonnull'}}
 
   (void)^(X * _Nullable_result p) {
     X *_Nonnull l = p; // expected-warning{{implicit conversion from nullable pointer 'X * _Nullable_result' to non-nullable pointer type 'X * _Nonnull'}}
   };
 }
 
 void test_dup(void) {
-  id _Nullable_result _Nullable_result a; // expected-warning {{duplicate nullability specifier _Nullable_result}}
-  id _Nullable _Nullable_result b; // expected-error{{nullability specifier _Nullable_result conflicts with existing specifier '_Nullable'}}
-  id _Nullable_result _Nonnull c; // expected-error{{nullability specifier '_Nonnull' conflicts with existing specifier _Nullable_result}}
+  id _Nullable_result _Nullable_result a; // expected-warning {{duplicate nullability specifier '_Nullable_result'}}
+  id _Nullable _Nullable_result b; // expected-error{{nullability specifier '_Nullable_result' conflicts with existing specifier '_Nullable'}}
+  id _Nullable_result _Nonnull c; // expected-error{{nullability specifier '_Nonnull' conflicts with existing specifier '_Nullable_result'}}
 }
 
 @interface NoContextSensitive
 -(nullable_result id)m; // expected-error {{expected a type}}
 @property(assign, nullable_result) id p; // expected-error{{unknown property attribute 'nullable_result'}}
 @end