[PATCH] D123352: [analyzer] Add FixItHint to `nullability.NullReturnedFromNonnull` and `nullability.NullableReturnedFromNonnull`
Moshe via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 12 13:28:36 PDT 2022
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
More information about the cfe-commits
mailing list