[PATCH] D123352: [analyzer] Add FixItHint to `nullability.NullReturnedFromNonnull` and `nullability.NullableReturnedFromNonnull`

[Moshe via Phabricator via cfe-commits]

MosheBerman marked 3 inline comments as done.
MosheBerman added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:592-602
+  // If we're inside of an NS_ASSUME, then the sourceRange will end before the
+  // asterisk.
+  const auto CanonicalTypeSize = CanonicalTypeStr.size();
+  const bool IsInsideOfAssume =
+      NullabilityLoc == RetTypeLoc.getSourceRange().getBegin().getLocWithOffset(
+                            CanonicalTypeSize - 1);
+
----------------
MosheBerman wrote:
> steakhal wrote:
> > Uh, this is really ugly. I don't believe this is the preferred way of detecting this. @NoQ WDYT?
> > Uh, this is really ugly.
> 
> It is really ugly. And a more correct implementation would probably handle some of the edge cases highlighted in the diff summary. It’s on me - being a n00b at llvm stuff. 
> 
> By “_this_” do you mean `IsInsideOfAssume` or `UseSyntaxSugar` in general?
> 
> > I don't believe this is the preferred way of detecting this. @NoQ WDYT?
> 
> I tried `isMacroExpansion` for assume nonnull, passing the end loc of the return type without success. 
> 
> A colleague suggested [SourceManager::isMacroBodyExpansion](https://clang.llvm.org/doxygen/classclang_1_1SourceManager.html#a04eadd011628b7c4cd3304712c8c221c). I’ll look at it again later today. 
> 
> Thank you so much for your patience and direction with reviews as I work on this. I really appreciate you making time for me!
Hello again! I tried a few things and looked at some of the clang source and I think this is the only way to check for this right now. 

The type system treats nullability attributes as Objective-C syntax sugar. It adds attributes on in SemaType.cpp, but does not track if an attribute is inferred by `NS_ASSUME` macros or if it's spelled-out. Consequentially, clang also does not preserve any information about the macro ID for the type. The reason this works right now is because the pointer character isn't included in `NullabilityLoc`. 

We can fix this by setting `isImplicit` on the inferred `Attr`s in SemaType.cpp, but I'm unsure if that's the correct approach. For now, this continues to work, and the tests will catch any breaking changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123352/new/

https://reviews.llvm.org/D123352