From: Jordan Rose Date: Tue, 17 Jul 2012 17:46:40 +0000 (+0000) Subject: Now that -Wobjc-literal-compare is a warning, put the fixit on a note. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=8d872ca7f10bb70d0757b894af79641679262bba;p=clang Now that -Wobjc-literal-compare is a warning, put the fixit on a note. Recovering as if the user had actually called -isEqual: is a bit too far from the semantics of the program as written, /even though/ it's probably what they intended. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@160377 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 5891274238..e5eea9d48a 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1613,8 +1613,9 @@ def err_box_literal_collection : Error< def warn_objc_literal_comparison : Warning< "direct comparison of %select{a string literal|an array literal|" "a dictionary literal|a numeric literal|a boxed expression|}0 has " - "undefined behavior%select{|; use -isEqual: instead}1">, - InGroup>; + "undefined behavior">, InGroup>; +def note_objc_literal_comparison_isequal : Note< + "use 'isEqual:' instead">; def err_attr_tlsmodel_arg : Error<"tls_model must be \"global-dynamic\", " "\"local-dynamic\", \"initial-exec\" or \"local-exec\"">; diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 9c5ecc405b..0548cccd1f 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -6688,11 +6688,60 @@ static bool isObjCObjectLiteral(ExprResult &E) { } } -static DiagnosticBuilder diagnoseObjCLiteralComparison(Sema &S, - SourceLocation Loc, - ExprResult &LHS, - ExprResult &RHS, - bool CanFix = false) { +static bool hasIsEqualMethod(Sema &S, const Expr *LHS, const Expr *RHS) { + // Get the LHS object's interface type. + QualType Type = LHS->getType(); + QualType InterfaceType; + if (const ObjCObjectPointerType *PTy = Type->getAs()) { + InterfaceType = PTy->getPointeeType(); + if (const ObjCObjectType *iQFaceTy = + InterfaceType->getAsObjCQualifiedInterfaceType()) + InterfaceType = iQFaceTy->getBaseType(); + } else { + // If this is not actually an Objective-C object, bail out. + return false; + } + + // If the RHS isn't an Objective-C object, bail out. + if (!RHS->getType()->isObjCObjectPointerType()) + return false; + + // Try to find the -isEqual: method. + Selector IsEqualSel = S.NSAPIObj->getIsEqualSelector(); + ObjCMethodDecl *Method = S.LookupMethodInObjectType(IsEqualSel, + InterfaceType, + /*instance=*/true); + if (!Method) { + if (Type->isObjCIdType()) { + // For 'id', just check the global pool. + Method = S.LookupInstanceMethodInGlobalPool(IsEqualSel, SourceRange(), + /*receiverId=*/true, + /*warn=*/false); + } else { + // Check protocols. + Method = S.LookupMethodInQualifiedType(IsEqualSel, + cast(Type), + /*instance=*/true); + } + } + + if (!Method) + return false; + + QualType T = Method->param_begin()[0]->getType(); + if (!T->isObjCObjectPointerType()) + return false; + + QualType R = Method->getResultType(); + if (!R->isScalarType()) + return false; + + return true; +} + +static void diagnoseObjCLiteralComparison(Sema &S, SourceLocation Loc, + ExprResult &LHS, ExprResult &RHS, + BinaryOperator::Opcode Opc){ Expr *Literal = (isObjCObjectLiteral(LHS) ? LHS : RHS).get(); unsigned LiteralKind; @@ -6740,94 +6789,20 @@ static DiagnosticBuilder diagnoseObjCLiteralComparison(Sema &S, llvm_unreachable("Unknown Objective-C object literal kind"); } - return S.Diag(Loc, diag::warn_objc_literal_comparison) - << LiteralKind << CanFix << Literal->getSourceRange(); -} - -static ExprResult fixObjCLiteralComparison(Sema &S, SourceLocation OpLoc, - ExprResult &LHS, - ExprResult &RHS, - BinaryOperatorKind Op) { - // Check early to see if the warning's on. - // If it's off, we should /not/ be auto-applying the accompanying fixit. - DiagnosticsEngine::Level Level = - S.getDiagnostics().getDiagnosticLevel(diag::warn_objc_literal_comparison, - OpLoc); - if (Level == DiagnosticsEngine::Ignored) - return ExprEmpty(); - - assert((Op == BO_EQ || Op == BO_NE) && "Cannot fix other operations."); + S.Diag(Loc, diag::warn_objc_literal_comparison) + << LiteralKind << Literal->getSourceRange(); - // Get the LHS object's interface type. - QualType Type = LHS.get()->getType(); - QualType InterfaceType; - if (const ObjCObjectPointerType *PTy = Type->getAs()) { - InterfaceType = PTy->getPointeeType(); - if (const ObjCObjectType *iQFaceTy = - InterfaceType->getAsObjCQualifiedInterfaceType()) - InterfaceType = iQFaceTy->getBaseType(); - } else { - // If this is not actually an Objective-C object, bail out. - return ExprEmpty(); - } - - // If the RHS isn't an Objective-C object, bail out. - if (!RHS.get()->getType()->isObjCObjectPointerType()) - return ExprEmpty(); - - // Try to find the -isEqual: method. - Selector IsEqualSel = S.NSAPIObj->getIsEqualSelector(); - ObjCMethodDecl *Method = S.LookupMethodInObjectType(IsEqualSel, - InterfaceType, - /*instance=*/true); - bool ReceiverIsId = (Type->isObjCIdType() || Type->isObjCQualifiedIdType()); + if (BinaryOperator::isEqualityOp(Opc) && + hasIsEqualMethod(S, LHS.get(), RHS.get())) { + SourceLocation Start = LHS.get()->getLocStart(); + SourceLocation End = S.PP.getLocForEndOfToken(RHS.get()->getLocEnd()); + SourceRange OpRange(Loc, S.PP.getLocForEndOfToken(Loc)); - if (!Method && ReceiverIsId) { - Method = S.LookupInstanceMethodInGlobalPool(IsEqualSel, SourceRange(), - /*receiverId=*/true, - /*warn=*/false); + S.Diag(Loc, diag::note_objc_literal_comparison_isequal) + << FixItHint::CreateInsertion(Start, Opc == BO_EQ ? "[" : "![") + << FixItHint::CreateReplacement(OpRange, "isEqual:") + << FixItHint::CreateInsertion(End, "]"); } - - if (!Method) - return ExprEmpty(); - - QualType T = Method->param_begin()[0]->getType(); - if (!T->isObjCObjectPointerType()) - return ExprEmpty(); - - QualType R = Method->getResultType(); - if (!R->isScalarType()) - return ExprEmpty(); - - // At this point we know we have a good -isEqual: method. - // Emit the diagnostic and fixit. - DiagnosticBuilder Diag = diagnoseObjCLiteralComparison(S, OpLoc, - LHS, RHS, true); - - Expr *LHSExpr = LHS.take(); - Expr *RHSExpr = RHS.take(); - - SourceLocation Start = LHSExpr->getLocStart(); - SourceLocation End = S.PP.getLocForEndOfToken(RHSExpr->getLocEnd()); - SourceRange OpRange(OpLoc, S.PP.getLocForEndOfToken(OpLoc)); - - Diag << FixItHint::CreateInsertion(Start, Op == BO_EQ ? "[" : "![") - << FixItHint::CreateReplacement(OpRange, "isEqual:") - << FixItHint::CreateInsertion(End, "]"); - - // Finally, build the call to -isEqual: (and possible logical not). - ExprResult Call = S.BuildInstanceMessage(LHSExpr, LHSExpr->getType(), - /*SuperLoc=*/SourceLocation(), - IsEqualSel, Method, - OpLoc, OpLoc, OpLoc, - MultiExprArg(S, &RHSExpr, 1), - /*isImplicit=*/false); - - ExprResult CallCond = S.CheckBooleanCondition(Call.get(), OpLoc); - - if (Op == BO_NE) - return S.CreateBuiltinUnaryOp(OpLoc, UO_LNot, CallCond.get()); - return CallCond; } // C99 6.5.8, C++ [expr.rel] @@ -7155,7 +7130,7 @@ QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS, diagnoseDistinctPointerComparison(*this, Loc, LHS, RHS, /*isError*/false); if (isObjCObjectLiteral(LHS) || isObjCObjectLiteral(RHS)) - diagnoseObjCLiteralComparison(*this, Loc, LHS, RHS); + diagnoseObjCLiteralComparison(*this, Loc, LHS, RHS, Opc); if (LHSIsNull && !RHSIsNull) LHS = ImpCastExprToType(LHS.take(), RHSType, CK_BitCast); @@ -8237,15 +8212,6 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc, break; case BO_EQ: case BO_NE: - if (getLangOpts().ObjC1) { - if (isObjCObjectLiteral(LHS) || isObjCObjectLiteral(RHS)) { - ExprResult IsEqualCall = fixObjCLiteralComparison(*this, OpLoc, - LHS, RHS, Opc); - if (IsEqualCall.isUsable()) - return IsEqualCall; - // Otherwise, fall back to the normal warning in CheckCompareOperands. - } - } ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, false); break; case BO_And: diff --git a/test/FixIt/objc-literals.m b/test/FixIt/objc-literals.m index b2cc3cd239..549cfde202 100644 --- a/test/FixIt/objc-literals.m +++ b/test/FixIt/objc-literals.m @@ -2,12 +2,10 @@ // RUN: cp %s %t // RUN: not %clang_cc1 -fsyntax-only -fixit -x objective-c %t // RUN: %clang_cc1 -fsyntax-only -pedantic -Werror -x objective-c %t -// RUN: FileCheck -input-file=%t %s typedef unsigned char BOOL; @interface NSObject -- (BOOL)isEqual:(id)other; @end @interface NSNumber : NSObject @@ -44,23 +42,3 @@ void fixes() { "blah" // expected-error{{string literal must be prefixed by '@'}} ]; } - -void testComparisons(id obj) { - if (obj == @"abc") return; // expected-warning{{direct comparison of a string literal has undefined behavior; use -isEqual: instead}} - if (obj != @"def") return; // expected-warning{{direct comparison of a string literal has undefined behavior; use -isEqual: instead}} - if (@"ghi" == obj) return; // expected-warning{{direct comparison of a string literal has undefined behavior; use -isEqual: instead}} - - // CHECK: void testComparisons(id obj) { - // Make sure these three substitutions aren't matching the CHECK lines. - // CHECK-NEXT: if ([obj isEqual: @"abc"]) return; - // CHECK-NEXT: if (![obj isEqual: @"def"]) return; - // CHECK-NEXT: if ([@"ghi" isEqual: obj]) return; - - if (@[] == obj) return; // expected-warning{{direct comparison of an array literal has undefined behavior; use -isEqual: instead}} - if (@{} == obj) return; // expected-warning{{direct comparison of a dictionary literal has undefined behavior; use -isEqual: instead}} - if (@12 == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior; use -isEqual: instead}} - if (@1.0 == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior; use -isEqual: instead}} - if (@__objc_yes == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior; use -isEqual: instead}} - if (@(1+1) == obj) return; // expected-warning{{direct comparison of a boxed expression has undefined behavior; use -isEqual: instead}} -} - diff --git a/test/SemaObjC/objc-literal-comparison.m b/test/SemaObjC/objc-literal-comparison.m index fea624f1a6..b0ca139961 100644 --- a/test/SemaObjC/objc-literal-comparison.m +++ b/test/SemaObjC/objc-literal-comparison.m @@ -28,17 +28,17 @@ typedef unsigned char BOOL; @end void testComparisonsWithFixits(id obj) { - if (obj == @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior; use -isEqual: instead}} - if (obj != @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior; use -isEqual: instead}} - if (@"" == obj) return; // expected-warning{{direct comparison of a string literal has undefined behavior; use -isEqual: instead}} - if (@"" == @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior; use -isEqual: instead}} - - if (@[] == obj) return; // expected-warning{{direct comparison of an array literal has undefined behavior; use -isEqual: instead}} - if (@{} == obj) return; // expected-warning{{direct comparison of a dictionary literal has undefined behavior; use -isEqual: instead}} - if (@12 == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior; use -isEqual: instead}} - if (@1.0 == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior; use -isEqual: instead}} - if (@__objc_yes == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior; use -isEqual: instead}} - if (@(1+1) == obj) return; // expected-warning{{direct comparison of a boxed expression has undefined behavior; use -isEqual: instead}} + if (obj == @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (obj != @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (@"" == obj) return; // expected-warning{{direct comparison of a string literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (@"" == @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + + if (@[] == obj) return; // expected-warning{{direct comparison of an array literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (@{} == obj) return; // expected-warning{{direct comparison of a dictionary literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (@12 == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (@1.0 == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (@__objc_yes == obj) return; // expected-warning{{direct comparison of a numeric literal has undefined behavior}} expected-note{{use 'isEqual:' instead}} + if (@(1+1) == obj) return; // expected-warning{{direct comparison of a boxed expression has undefined behavior}} expected-note{{use 'isEqual:' instead}} } @@ -52,17 +52,14 @@ void testComparisonsWithFixits(id obj) { void testComparisonsWithoutFixits() { - // All of these verifications use regex form to ensure we /don't/ append - // "use -isEqual: instead" to any of these. + if ([BaseObject new] == @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} - if ([BaseObject new] == @"") return; // expected-warning-re{{direct comparison of a string literal has undefined behavior$}} + if ([BadEqualReturnString new] == @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} + if ([BadEqualArgString new] == @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} - if ([BadEqualReturnString new] == @"") return; // expected-warning-re{{direct comparison of a string literal has undefined behavior$}} - if ([BadEqualArgString new] == @"") return; // expected-warning-re{{direct comparison of a string literal has undefined behavior$}} - - if (@"" < @"") return; // expected-warning-re{{direct comparison of a string literal has undefined behavior$}} - if (@"" > @"") return; // expected-warning-re{{direct comparison of a string literal has undefined behavior$}} - if (@"" <= @"") return; // expected-warning-re{{direct comparison of a string literal has undefined behavior$}} - if (@"" >= @"") return; // expected-warning-re{{direct comparison of a string literal has undefined behavior$}} + if (@"" < @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} + if (@"" > @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} + if (@"" <= @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} + if (@"" >= @"") return; // expected-warning{{direct comparison of a string literal has undefined behavior}} }