[Sema] Use dotted form of macOS version for unguarded availability FixIts

[jkorous]

Summary

E. g. use "10.11" instead of "10_11".

This is just a consistency fix - the dotted format is already used everywhere else.

rdar://problem/39845032

[erik.pilkington]

That test passes for me without this patch applied, why is UsesUnderscores actually getting set in the radar?

[jkorous]

Sorry, my bad. I tried to get rid of dependency on Foundation.h and didn't check the test is relevant for the fix after that.

#import <Foundation/Foundation.h>

@interface foo
- (void) method NS_AVAILABLE_MAC(10_12);
@end

int main() {
    [foo method];
    return 0;
}

Looks like NS_AVAILABLE_MAC is not equivalent to the attribute.

Interesting question about UsesUnderscores, thanks! It looks like it must be set through constructor somewhere - will figure this out.

[jkorous]

Fixed the test. It turned out that version component separator for availability is set during parsing which is why all other tests (including my bad one) are passing.

The only way how to set underscore as a separator is through VersionTuple constructor...

https://clang.llvm.org/doxygen/classclang_1_1VersionTuple.html#a71e5354ccb617494136bdcb4d208a573

clang::VersionTuple::VersionTuple	(	unsigned 	Major,
  unsigned 	Minor,
  unsigned 	Subminor,
  unsigned 	Build,
  bool 	UsesUnderscores = false 
)

...which is exactly what is happening during parsing version tuple...

VersionTuple Parser::ParseVersionTuple(SourceRange &Range) {
  // ...
  return VersionTuple(Major, Minor, Subminor, (AfterMajorSeparator == '_'));
}

...which function is called when parsing availability attribute:

void Parser::ParseAvailabilityAttribute(IdentifierInfo &Availability,
  // ...
  VersionTuple Version = ParseVersionTuple(VersionRange);
  // ...
}

[jkorous]

I am not 100% sure it's the best thing to set printing style at the point of parsing version tuples but I am not sure it's bad either. Unless someone convinces me otherwise I would rather not do any major changes.

[erik.pilkington]

Thanks for the explanation, LGTM.

[jkorous]

Sorry for me being dense here - since the output format is determined by input source code there's more work to do.

I tried to change format of introduced version in couple of tests and the output mirrors the change. That basically means that whenever someone uses underscore

__attribute__((availability(macosx, introduced = 10_12)))

instead of dot

__attribute__((availability(macosx, introduced = 10.12)))

Warnings and FixIts get underscore as well.

I am now thinking about just switching to dot format in ParseAvailabilityAttribute() because it's much simpler to maintain consistency that way.

[erik.pilkington]

I am now thinking about just switching to dot format in ParseAvailabilityAttribute() because it's much simpler to maintain consistency that way.

Okay, but if we're going to treat underscores as dots while printing diagnostics and fixits (which I think we should), then we shouldn't track the spelling in VersionTuple: we no longer have any use for it. The only reason that VersionTuple supports this is because of r219124, which was written so that diagnostic printing would be consistent with how the attribute was spelled. Maybe there was a good reason for this, but I can't see the radar that was linked in the commit message. Can you look up that radar to see what r219124 was actually trying to fix? I don't think we should move forward with this until we have that context.

Thanks!
Erik

[jkorous]

Eric, thanks for the context. I am now clarifying internally and if possible would like to simplify the whole format business by using just one delimiter everywhere. Would that make sense to you or do you think we should respect delimiter used in sources?

[jkorous]

After some internal discussion we agreed that we can simplify things and get consistent behaviour by using dot everywhere in diagnostics.

This is pretty much revert of r219124 + update of tests.

[erik.pilkington]

Great, LGTM!

cfe/trunk/include/clang/Basic/VersionTuple.h

 #include "llvm/ADT/Optional.h"
 #include <string>
 #include <tuple>
 
 namespace clang {
 
 /// Represents a version number in the form major[.minor[.subminor[.build]]].
 class VersionTuple {
-  unsigned Major : 31;
-
-  unsigned UsesUnderscores : 1;
+  unsigned Major : 32;
 
   unsigned Minor : 31;
   unsigned HasMinor : 1;
 
   unsigned Subminor : 31;
   unsigned HasSubminor : 1;
 
   unsigned Build : 31;
   unsigned HasBuild : 1;
 
 public:
   VersionTuple()
-      : Major(0), UsesUnderscores(false), Minor(0), HasMinor(false),
-        Subminor(0), HasSubminor(false), Build(0), HasBuild(false) {}
+      : Major(0), Minor(0), HasMinor(false), Subminor(0), HasSubminor(false),
+        Build(0), HasBuild(false) {}
 
   explicit VersionTuple(unsigned Major)
-      : Major(Major), UsesUnderscores(false), Minor(0), HasMinor(false),
-        Subminor(0), HasSubminor(false), Build(0), HasBuild(false) {}
-
-  explicit VersionTuple(unsigned Major, unsigned Minor,
-                        bool UsesUnderscores = false)
-      : Major(Major), UsesUnderscores(UsesUnderscores), Minor(Minor),
-        HasMinor(true), Subminor(0), HasSubminor(false), Build(0),
-        HasBuild(false) {}
+      : Major(Major), Minor(0), HasMinor(false), Subminor(0),
+        HasSubminor(false), Build(0), HasBuild(false) {}
 
-  explicit VersionTuple(unsigned Major, unsigned Minor, unsigned Subminor,
-                        bool UsesUnderscores = false)
-      : Major(Major), UsesUnderscores(UsesUnderscores), Minor(Minor),
-        HasMinor(true), Subminor(Subminor), HasSubminor(true), Build(0),
-        HasBuild(false) {}
+  explicit VersionTuple(unsigned Major, unsigned Minor)
+      : Major(Major), Minor(Minor), HasMinor(true), Subminor(0),
+        HasSubminor(false), Build(0), HasBuild(false) {}
+
+  explicit VersionTuple(unsigned Major, unsigned Minor, unsigned Subminor)
+      : Major(Major), Minor(Minor), HasMinor(true), Subminor(Subminor),
+        HasSubminor(true), Build(0), HasBuild(false) {}
 
   explicit VersionTuple(unsigned Major, unsigned Minor, unsigned Subminor,
-                        unsigned Build, bool UsesUnderscores = false)
-      : Major(Major), UsesUnderscores(UsesUnderscores), Minor(Minor),
-        HasMinor(true), Subminor(Subminor), HasSubminor(true), Build(Build),
-        HasBuild(true) {}
+                        unsigned Build)
+      : Major(Major), Minor(Minor), HasMinor(true), Subminor(Subminor),
+        HasSubminor(true), Build(Build), HasBuild(true) {}
 
   /// Determine whether this version information is empty
   /// (e.g., all version components are zero).
   bool empty() const {
     return Major == 0 && Minor == 0 && Subminor == 0 && Build == 0;
   }
 
   /// Retrieve the major version number.
public:
   }
 
   /// Retrieve the build version number, if provided.
   Optional<unsigned> getBuild() const {
     if (!HasBuild)
       return None;
     return Build;
   }
   
-  bool usesUnderscores() const {
-    return UsesUnderscores;
-  }
-
-  void UseDotAsSeparator() {
-    UsesUnderscores = false;
-  }
-  
   /// Determine if two version numbers are equivalent. If not
   /// provided, minor and subminor version numbers are considered to be zero.
   friend bool operator==(const VersionTuple& X, const VersionTuple &Y) {
     return X.Major == Y.Major && X.Minor == Y.Minor &&
            X.Subminor == Y.Subminor && X.Build == Y.Build;
   }
 
   /// Determine if two version numbers are not equivalent.

cfe/trunk/lib/AST/DeclBase.cpp

static AvailabilityResult CheckAvailability(ASTContext &Context,
 
   // Make sure that this declaration has already been introduced.
   if (!A->getIntroduced().empty() && 
       EnclosingVersion < A->getIntroduced()) {
     if (Message) {
       Message->clear();
       llvm::raw_string_ostream Out(*Message);
       VersionTuple VTI(A->getIntroduced());
-      VTI.UseDotAsSeparator();
       Out << "introduced in " << PrettyPlatformName << ' ' 
           << VTI << HintMessage;
     }
 
     return A->getStrict() ? AR_Unavailable : AR_NotYetIntroduced;
   }
 
   // Make sure that this declaration hasn't been obsoleted.
   if (!A->getObsoleted().empty() && EnclosingVersion >= A->getObsoleted()) {
     if (Message) {
       Message->clear();
       llvm::raw_string_ostream Out(*Message);
       VersionTuple VTO(A->getObsoleted());
-      VTO.UseDotAsSeparator();
       Out << "obsoleted in " << PrettyPlatformName << ' ' 
           << VTO << HintMessage;
     }
     
     return AR_Unavailable;
   }
 
   // Make sure that this declaration hasn't been deprecated.
   if (!A->getDeprecated().empty() && EnclosingVersion >= A->getDeprecated()) {
     if (Message) {
       Message->clear();
       llvm::raw_string_ostream Out(*Message);
       VersionTuple VTD(A->getDeprecated());
-      VTD.UseDotAsSeparator();
       Out << "first deprecated in " << PrettyPlatformName << ' '
           << VTD << HintMessage;
     }
     
     return AR_Deprecated;
   }
 
   return AR_Available;

cfe/trunk/lib/Basic/VersionTuple.cpp

std::string VersionTuple::getAsString() const {
   }
   return Result;
 }
 
 raw_ostream& clang::operator<<(raw_ostream &Out, 
                                      const VersionTuple &V) {
   Out << V.getMajor();
   if (Optional<unsigned> Minor = V.getMinor())
-    Out << (V.usesUnderscores() ? '_' : '.') << *Minor;
+    Out << '.' << *Minor;
   if (Optional<unsigned> Subminor = V.getSubminor())
-    Out << (V.usesUnderscores() ? '_' : '.') << *Subminor;
+    Out << '.' << *Subminor;
   if (Optional<unsigned> Build = V.getBuild())
-    Out << (V.usesUnderscores() ? '_' : '.') << *Build;
+    Out << '.' << *Build;
   return Out;
 }
 
 static bool parseInt(StringRef &input, unsigned &value) {
   assert(value == 0);
   if (input.empty()) return true;
 
   char next = input[0];

cfe/trunk/lib/Parse/ParseDecl.cpp

 static bool VersionNumberSeparator(const char Separator) {
   return (Separator == '.' || Separator == '_');
 }
 
 /// Parse a version number.
 ///
 /// version:
 ///   simple-integer
-///   simple-integer ',' simple-integer
-///   simple-integer ',' simple-integer ',' simple-integer
+///   simple-integer '.' simple-integer
+///   simple-integer '_' simple-integer
+///   simple-integer '.' simple-integer '.' simple-integer
+///   simple-integer '_' simple-integer '_' simple-integer
 VersionTuple Parser::ParseVersionTuple(SourceRange &Range) {
   Range = SourceRange(Tok.getLocation(), Tok.getEndLoc());
 
   if (!Tok.is(tok::numeric_constant)) {
     Diag(Tok, diag::err_expected_version);
     SkipUntil(tok::comma, tok::r_paren,
               StopAtSemi | StopBeforeMatch | StopAtCodeCompletion);
     return VersionTuple();
if (AfterMinor == ActualLength) {
     ConsumeToken();
 
     // We had major.minor.
     if (Major == 0 && Minor == 0) {
       Diag(Tok, diag::err_zero_version);
       return VersionTuple();
     }
 
-    return VersionTuple(Major, Minor, (AfterMajorSeparator == '_'));
+    return VersionTuple(Major, Minor);
   }
 
   const char AfterMinorSeparator = ThisTokBegin[AfterMinor];
   // If what follows is not a '.' or '_', we have a problem.
   if (!VersionNumberSeparator(AfterMinorSeparator)) {
     Diag(Tok, diag::err_expected_version);
     SkipUntil(tok::comma, tok::r_paren,
               StopAtSemi | StopBeforeMatch | StopAtCodeCompletion);
VersionTuple Parser::ParseVersionTuple(SourceRange &Range) {
 
   if (AfterSubminor != ActualLength) {
     Diag(Tok, diag::err_expected_version);
     SkipUntil(tok::comma, tok::r_paren,
               StopAtSemi | StopBeforeMatch | StopAtCodeCompletion);
     return VersionTuple();
   }
   ConsumeToken();
-  return VersionTuple(Major, Minor, Subminor, (AfterMajorSeparator == '_'));
+  return VersionTuple(Major, Minor, Subminor);
 }
 
 /// Parse the contents of the "availability" attribute.
 ///
 /// availability-attribute:
 ///   'availability' '(' platform ',' opt-strict version-arg-list,
 ///                      opt-replacement, opt-message')'
 ///

cfe/trunk/test/Misc/ast-print-objectivec.m

 // CHECK: - (void)MethP __attribute__((availability(macos, introduced=10.1.0, deprecated=10.2)));
 // CHECK: @end
 
 // CHECK: @interface I : NSObject<P> 
 // CHECK: - (void)MethI __attribute__((availability(macos, introduced=10.1.0, deprecated=10.2)));
 // CHECK: @end
 
 // CHECK: @interface I(CAT)
-// CHECK: - (void)MethCAT __attribute__((availability(macos, introduced=10_1_0, deprecated=10_2)));
+// CHECK: - (void)MethCAT __attribute__((availability(macos, introduced=10.1.0, deprecated=10.2)));
 // CHECK: @end
 
 // CHECK: @implementation I
 // CHECK: - (void)MethP __attribute__((availability(macos, introduced=10.1.0, deprecated=10.2))) {
 // CHECK: }
 
 // CHECK: - (void)MethI __attribute__((availability(macos, introduced=10.1.0, deprecated=10.2))) {
 // CHECK: }

cfe/trunk/test/Sema/availability-guard-format.mm

+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.11 -Wunguarded-availability -fdiagnostics-parseable-fixits -fsyntax-only -verify %s
+
+// Testing that even for source code using '_' as a delimiter in availability version tuple '.' is actually used in diagnostic output as a delimiter.
+
+@interface foo
+- (void) method_bar __attribute__((availability(macosx, introduced = 10_12))); // expected-note {{'method_bar' has been explicitly marked partial here}}
+@end
+
+int main() {
+    [foo method_bar]; // \
+    // expected-warning {{'method_bar' is only available on macOS 10.12 or newer}} \
+    // expected-note {{enclose 'method_bar' in an @available check to silence this warning}} \
+    // CHECK: "fix-it:.*if (@available(macOS 10.12, *))"
+    return 0;
+}

cfe/trunk/test/SemaObjC/attr-availability-1.m

 - (void)overridden4 __attribute__((availability(macosx,deprecated=10_3))); // expected-note{{overridden method is here}}
 - (void)overridden5 __attribute__((availability(macosx,unavailable)));
 - (void)overridden6 __attribute__((availability(macosx,introduced=10_3))); // expected-note{{overridden method is here}}
 @end
 
 // rdar://11475360
 @interface B : A
 - (void)method; // NOTE: we expect 'method' to *not* inherit availability.
-- (void)overridden __attribute__((availability(macosx,introduced=10_4))); // expected-warning{{overriding method introduced after overridden method on macOS (10_4 vs. 10_3)}}
+- (void)overridden __attribute__((availability(macosx,introduced=10_4))); // expected-warning{{overriding method introduced after overridden method on macOS (10.4 vs. 10.3)}}
 - (void)overridden2 __attribute__((availability(macosx,introduced=10_2)));
 - (void)overridden3 __attribute__((availability(macosx,deprecated=10_4)));
-- (void)overridden4 __attribute__((availability(macosx,deprecated=10_2))); // expected-warning{{overriding method deprecated before overridden method on macOS (10_3 vs. 10_2)}}
+- (void)overridden4 __attribute__((availability(macosx,deprecated=10_2))); // expected-warning{{overriding method deprecated before overridden method on macOS (10.3 vs. 10.2)}}
 - (void)overridden5 __attribute__((availability(macosx,introduced=10_3)));
 - (void)overridden6 __attribute__((availability(macosx,unavailable))); // expected-warning{{overriding method cannot be unavailable on macOS when its overridden method is available}}
 @end
 
 void f(A *a, B *b) {
   [a method]; // expected-warning{{'method' is deprecated: first deprecated in macOS 10.2}}
   [b method]; // no-warning
   [a proto_method]; // expected-warning{{'proto_method' is deprecated: first deprecated in macOS 10.2}}