[RFC] Emit note pointing to a discarded qualifier [-Wincompatible-pointer-types-discards-qualifiers]

[adek05]

Summary

Finding original declaration which is suffering from discarding a qualifier is somewhat tricky and it doesn't always make sense in all of the contexts that DiagnoseAssignmentResult is called.
I haven't figured out yet how to make this change work for ObjC Setters which I think is the only place left which should handle this diagnostic.
Current approach is sort of best-effort, but doesn't really guarantee much.

Alternative way to go would be to factor this extra diagnostic out and write it in a similar fashion to DiagnoseSelfAssignment and call it in some of the contexts when note could be useful.
The problem I see with that, is that it would have issues with handling overriden assignment operators or would not work for non built-in types.

Test Plan:

$ cat test.c
int main() {
  char * c;
  char const ** c2 = &c;

  char * d;
  char volatile ** d2 = &d;
}

test.c:3:17: warning: initializing 'const char **' with an expression of type 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
  char const ** c2 = &c;
                ^    ~~
test.c:3:3: note: nested pointer type with discarded 'const' qualifier
  char const ** c2 = &c;
  ^~~~~~~~~~~~~~~~
test.c:6:20: warning: initializing 'volatile char **' with an expression of type 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
  char volatile ** d2 = &d;
                   ^    ~~
test.c:6:3: note: nested pointer type with discarded 'const' qualifier
  char volatile ** d2 = &d;
  ^~~~~~~~~~~~~~~~~~~

[jroelofs]

Can you write up a couple of testcases for the new diagnostic? There should be some examples you can follow in clang/test/Sema ans clang/test/SemaCXX.

I'd also be interested in seeing a concrete example of the setter thing you were talking about in ObjC/ObjC++.

[aaron.ballman]

I would be interested in seeing test cases where this is an improvement. For instance, would it be equally as useful to pass in the type range to the existing diagnostic so that the type gets highlighted as well (and skip the note entirely)?

[adek05]

@aaron.ballman I think this could be hard to achieve without an extra note if you have something like:

cat test2.c
int main() {
        char *c = 'a';

        char volatile** cc = &c;
        cc = &c;
}

test2.c:2:15: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
        char *c = 'a';
              ^   ~~~
test2.c:4:25: warning: initializing 'volatile char **' with an expression of type 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
        char volatile** cc = &c;
                        ^    ~~
test2.c:4:9: note: nested pointer type with discarded 'const' qualifier
        char volatile** cc = &c;
        ^~~~~~~~~~~~~~~~~~
test2.c:5:12: warning: assigning to 'volatile char **' from 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
        cc = &c;
           ^ ~~
3 warnings generated.

Sadly, my code doesn't print a note in the second case, which I would have to debug. In case of initialization, I think we can just print one line and omit note.
For assignment which is not an initialization, it might be useful to point to the declaration of a variable which is assigned.

I will try to handle the second case and write tests for this. Let me know if you feel like it is still worth proceeding.

[adek05]

lib/Sema/SemaPseudoObject.cpp

739–751

This is the place which I suspect may trigger this warning, but I need to read the code more careful to come up with an example.

[aaron.ballman]

In D15486#310572, @adek05 wrote:

@aaron.ballman I think this could be hard to achieve without an extra note if you have something like:

cat test2.c
int main() {
        char *c = 'a';

        char volatile** cc = &c;
        cc = &c;
}

test2.c:2:15: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
        char *c = 'a';
              ^   ~~~
test2.c:4:25: warning: initializing 'volatile char **' with an expression of type 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
        char volatile** cc = &c;
                        ^    ~~
test2.c:4:9: note: nested pointer type with discarded 'const' qualifier
        char volatile** cc = &c;
        ^~~~~~~~~~~~~~~~~~
test2.c:5:12: warning: assigning to 'volatile char **' from 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
        cc = &c;
           ^ ~~
3 warnings generated.

Sadly, my code doesn't print a note in the second case, which I would have to debug. In case of initialization, I think we can just print one line and omit note.
For assignment which is not an initialization, it might be useful to point to the declaration of a variable which is assigned.

I will try to handle the second case and write tests for this. Let me know if you feel like it is still worth proceeding.

I'm still not convinced this adds value as a note. The diagnostic has the type information for the destination as well as the source. A note pointing where the destination type lives doesn't seem to add any new information, but pointing out which qualifiers are discarded as part of the original diagnostic does add value. So I think that the idea has merit, but I don't think the note is the way to go. Instead, I wonder if it makes sense to just update the existing diagnostic to have further information when it is available.

Also, the note is currently incorrect -- it is claiming that the nested pointer type has a discarded const qualifier, but it has a discarded volatile qualifier. The code needs to figure out which qualifiers are discarded (and those qualifiers may be disjoint in the type specifier). Consider a type like "const char * volatile * const * restrict * const" -- the diagnostic would have to point out which qualifiers are discarded, and it could be that restrict and volatile are discarded but the consts are fine.

Perhaps as a first pass, you could handle the case where only a single qualifier is discarded, and add the qualifier and a SourceRange for that particular qualifier to the existing diagnostic. e.g., Diag(Loc, diag::warn_incompatible_qualified_id) << FirstType << SecondType << DiscardedQualifier << TypeSourceRange; Future iterations could then track multiple discarded qualifiers and their source ranges. (Note, I have no idea how easy or hard this might be.)

include/clang/Basic/DiagnosticSemaKinds.td

def ext_typecheck_convert_discards_qualifiers : ExtWarn<
   "|%diff{converting $ to type $|converting between types}0,1"
   "|%diff{initializing $ with an expression of type $|"
   "initializing with expression of different type}0,1"
   "|%diff{sending $ to parameter of type $|"
   "sending to parameter of different type}0,1"
   "|%diff{casting $ to type $|casting between types}0,1}2"
   " discards qualifiers">,
   InGroup<IncompatiblePointerTypesDiscardsQualifiers>;
+def note_nested_pointer_discarded_qualifier : Note<
+  "nested pointer type with discarded 'const' qualifier">;
 def ext_nested_pointer_qualifier_mismatch : ExtWarn<
   "%select{%diff{assigning to $ from $|assigning to different types}0,1"
   "|%diff{passing $ to parameter of type $|"
   "passing to parameter of different type}0,1"
   "|%diff{returning $ from a function with result type $|"
   "returning from function with different return type}0,1"
   "|%diff{converting $ to type $|converting between types}0,1"
   "|%diff{initializing $ with an expression of type $|"

include/clang/Sema/Sema.h

enum AssignConvertType {
     /// Incompatible - We reject this conversion outright, it is invalid to
     /// represent it in the AST.
     Incompatible
   };
 
   /// DiagnoseAssignmentResult - Emit a diagnostic, if required, for the
   /// assignment conversion type specified by ConvTy.  This returns true if the
   /// conversion was invalid or false if the conversion was accepted.
-  bool DiagnoseAssignmentResult(AssignConvertType ConvTy,
-                                SourceLocation Loc,
-                                QualType DstType, QualType SrcType,
-                                Expr *SrcExpr, AssignmentAction Action,
+  bool DiagnoseAssignmentResult(AssignConvertType ConvTy, SourceLocation Loc,
+                                QualType DstType, VarDecl *DstVarDecl,
+                                QualType SrcType, Expr *SrcExpr,
+                                AssignmentAction Action,
                                 bool *Complained = nullptr);
 
   /// IsValueInFlagEnum - Determine if a value is allowed as part of a flag
   /// enum. If AllowMask is true, then we also allow the complement of a valid
   /// value, to be used as a mask.
   bool IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
                          bool AllowMask) const;
 

lib/Sema/SemaChecking.cpp

QualType EltTy =
         getNeonEltType(NeonTypeFlags(TV), Context, IsPolyUnsigned, IsInt64Long);
     if (HasConstPtr)
       EltTy = EltTy.withConst();
     QualType LHSTy = Context.getPointerType(EltTy);
     AssignConvertType ConvTy;
     ConvTy = CheckSingleAssignmentConstraints(LHSTy, RHS);
     if (RHS.isInvalid())
       return true;
-    if (DiagnoseAssignmentResult(ConvTy, Arg->getLocStart(), LHSTy, RHSTy,
+    if (DiagnoseAssignmentResult(ConvTy, Arg->getLocStart(), LHSTy,
+                                 /* lhs declaration */ nullptr, RHSTy,
                                  RHS.get(), AA_Assigning))
       return true;
   }
 
   // For NEON intrinsics which take an immediate value as part of the
   // instruction, range check them here.
   unsigned i = 0, l = 0, u = 0;
   switch (BuiltinID) {

lib/Sema/SemaExpr.cpp

if (ConvTy == Compatible) {
         checkUnsafeExprAssigns(Loc, LHSExpr, RHS.get());
       }
     }
   } else {
     // Compound assignment "x += y"
     ConvTy = CheckAssignmentConstraints(Loc, LHSType, RHSType);
   }
 
-  if (DiagnoseAssignmentResult(ConvTy, Loc, LHSType, RHSType,
-                               RHS.get(), AA_Assigning))
+  DeclRefExpr *DeclRef = dyn_cast<DeclRefExpr>(LHSExpr);
+  if (DiagnoseAssignmentResult(ConvTy, Loc, LHSType,
+                               DeclRef->getDecl() ? DeclRef->getDecl()
+                                                  : nullptr,
+                               RHSType, RHS.get(), AA_Assigning))
     return QualType();
 
   CheckForNullPointerDereference(*this, LHSExpr);
 
   // C99 6.5.16p3: The type of an assignment expression is the type of the
   // left operand unless the left operand has qualified type, in which case
   // it is the unqualified version of the type of the left operand.
   // C99 6.5.16.1p2: In simple assignment, the value of the right operand
if (!FD)
     return false;
 
   return !S.checkAddressOfFunctionIsAvailable(FD,
                                               /*Complain=*/true,
                                               SrcExpr->getLocStart());
 }
 
 bool Sema::DiagnoseAssignmentResult(AssignConvertType ConvTy,
-                                    SourceLocation Loc,
-                                    QualType DstType, QualType SrcType,
+                                    SourceLocation Loc, QualType DstType,
+                                    VarDecl *DstVarDecl, QualType SrcType,
                                     Expr *SrcExpr, AssignmentAction Action,
                                     bool *Complained) {
   if (Complained)
     *Complained = false;
 
   // Decode the result (notice that AST's are still created for extensions).
   bool CheckInferredResultType = false;
   bool isInvalid = false;
if (!ConvHints.isNull()) {
     FDiag << Hint;
   }
   if (MayHaveConvFixit) { FDiag << (unsigned) (ConvHints.Kind); }
 
   if (MayHaveFunctionDiff)
     HandleFunctionTypeMismatch(FDiag, SecondType, FirstType);
 
   Diag(Loc, FDiag);
+
+  // Emit original location of a variable declaration which is on the LHS
+  if (DiagKind == diag::ext_nested_pointer_qualifier_mismatch && DstVarDecl) {
+    SourceRange DstVarDeclSR = DstVarDecl->getSourceRange();
+    PartialDiagnostic Note =
+        PDiag(diag::note_nested_pointer_discarded_qualifier);
+    Note << DstVarDeclSR;
+    Diag(DstVarDeclSR.getBegin(), Note);
+  }
+
   if (DiagKind == diag::warn_incompatible_qualified_id &&
       PDecl && IFace && !IFace->hasDefinition())
       Diag(IFace->getLocation(), diag::not_incomplete_class_and_qualified_id)
         << IFace->getName() << PDecl->getName();
     
   if (SecondType == Context.OverloadTy)
     NoteAllOverloadCandidates(OverloadExpr::find(SrcExpr).Expression,
                               FirstType, /*TakingAddress=*/true);

lib/Sema/SemaInit.cpp

case SK_CAssignment: {
           S.CheckTransparentUnionArgumentConstraints(Step->Type, CurInitExprRes)
             == Sema::Compatible)
         ConvTy = Sema::Compatible;
       if (CurInitExprRes.isInvalid())
         return ExprError();
       CurInit = CurInitExprRes;
 
       bool Complained;
-      if (S.DiagnoseAssignmentResult(ConvTy, Kind.getLocation(),
-                                     Step->Type, SourceType,
-                                     InitialCurInit.get(),
-                                     getAssignmentAction(Entity, true),
-                                     &Complained)) {
+      if (S.DiagnoseAssignmentResult(
+              ConvTy, Kind.getLocation(), Step->Type,
+              Entity.getDecl() ? dyn_cast<VarDecl>(Entity.getDecl()) : nullptr,
+              SourceType, InitialCurInit.get(),
+              getAssignmentAction(Entity, true), &Complained)) {
         PrintInitLocationNote(S, Entity);
         return ExprError();
       } else if (Complained)
         PrintInitLocationNote(S, Entity);
       break;
     }
 
     case SK_StringInit: {

lib/Sema/SemaPseudoObject.cpp

ExprResult ObjCPropertyOpBuilder::buildSet(Expr *op, SourceLocation opcLoc,
   if (SyntacticRefExpr)
     SyntacticRefExpr->setIsMessagingSetter();
 
   QualType receiverType = RefExpr->getReceiverType(S.Context);
 
   // Use assignment constraints when possible; they give us better
   // diagnostics.  "When possible" basically means anything except a
   // C++ class type.
   if (!S.getLangOpts().CPlusPlus || !op->getType()->isRecordType()) {
     QualType paramType = (*Setter->param_begin())->getType()
                            .substObjCMemberType(
                              receiverType,
                              Setter->getDeclContext(),
                              ObjCSubstitutionContext::Parameter);
     if (!S.getLangOpts().CPlusPlus || !paramType->isRecordType()) {
       ExprResult opResult = op;
       Sema::AssignConvertType assignResult
         = S.CheckSingleAssignmentConstraints(paramType, opResult);
-      if (S.DiagnoseAssignmentResult(assignResult, opcLoc, paramType,
+      if (S.DiagnoseAssignmentResult(assignResult, opcLoc, paramType, nullptr,
                                      op->getType(), opResult.get(),
                                      Sema::AA_Assigning))
         return ExprError();
 
       op = opResult.get();
       assert(op && "successful assignment left argument invalid?");
     }
   }
 
   // Arguments.

lib/Sema/SemaStmt.cpp

if (!E->isTypeDependent()) {
     QualType ETy = E->getType();
     QualType DestTy = Context.getPointerType(Context.VoidTy.withConst());
     ExprResult ExprRes = E;
     AssignConvertType ConvTy =
       CheckSingleAssignmentConstraints(DestTy, ExprRes);
     if (ExprRes.isInvalid())
       return StmtError();
     E = ExprRes.get();
-    if (DiagnoseAssignmentResult(ConvTy, StarLoc, DestTy, ETy, E, AA_Passing))
+    if (DiagnoseAssignmentResult(ConvTy, StarLoc, DestTy, nullptr, ETy, E,
+                                 AA_Passing))
       return StmtError();
   }
 
   ExprResult ExprRes = ActOnFinishFullExpr(E);
   if (ExprRes.isInvalid())
     return StmtError();
   E = ExprRes.get();