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

[MosheBerman]

Summary

This diff adds FixItHints to clang's NullabilityChecker where a return type is erroneously marked as nonnull.

1. A CmdLineOption called ShowFixIts to the all of nullability checks. (We add to all of them because of the way NullabilityChecker.cpp registers all of the checkers.)
2. For each of the two *ReturnedFromNonnull methods, attaches a FixItHint to the output.

Why
So we can use NS_ASSUME_NONNULL and then apply the fixits to automatically and quickly annotate large amounts of source code.

What It Does
The checker will emit a FixIt the following cases for unparameterized pointer return types, like NSObject *:

Declaration	Annotation	Within NS_ASSUME_NONNULL	FixItHint
ObjCMethodDecl	_Nonnull after *	No	Replace with _Nullable.
ObjCMethodDecl	nonnull before type name	No	Replace with nullable.
ObjCMethodDecl	implicitly unspecified	No	No hint emitted.
ObjCMethodDecl	implicitly unspecified	Yes	Insert nullable
FunctionDecl	_Nonnull after *	No	Replace with _Nullable.
FunctionDecl	_Nonnull after *	Yes	Replace with _Nullable.
FunctionDecl	implicitly unspecified	Yes	Insert _Nullable.

How It Works

• If we're not inside an audited region, and there's no existing annotation, we'll insert an annotation, and choose the correct style, depending if we're annotating a function or an Obj-C method decl.
• We check for existing annotations without our accessing source directly by using TypeLoc's findNullabilityLoc.
  • An invalid loc means no prior annotations.
  • A valid loc tells us both that there's an existing annotation, and that we need to replace it.

Handling NS_ASSUME... macros

• There's one edge case where a valid nullability location is a false positive - inside of an NS_ASSUME_NONNULL pair of pragmas.
  • The Lexer can tell us if we're inside of such an audited region, but we probably shouldn't use it.
  • Luckily, there's a subtle nuance in findNullabilityLoc: If we are inside of an "assume region", and the annotation isn't really written out in source, then the location returned is going to be right *before* the asterisk/pointer symbol. Otherwise, if the code actually has _Nonnull written out, we're going to see the location of the nullability annotation correctly after the asterisk.

False Positives
Here's why I think this is safe:

1. This is an addition to the existing NullabilityChecker, which is already quite conservative in what it diagnoses, so I'm fairly confident that this won't break anything.
2. The fixits are only emitted if a ShowFixits flag is enabled on the two nullability return-type checkers, so the default behavior does not change.

Edge Cases

1. instancetype and id inside of "audited regions" are problematic, since they don't have the same difference in findNullability. a. We'll skip these for now. Once we have a better way to properly find macro expansions we might be able to enable this.
2. There are potentially edge cases here with parameterized types, such as NSArray and NSDictionary. a. I added test cases using a protocol to verify that angle brackets don't interfere with the logic, because the fake headers we use for testing don't properly support the collection types mentioned above.

Running Tests

LIT_FILTER=nullability-fixits  ninja check-clang

The tests have four checks.

1. Verify that the fixits are not emitted by default.
2. Verify that the fixits are not emitted for their respective checkers if -analyzer-config explicitly disables either nullability.NullReturnedFromNonnull:ShowFixIts and/or nullability.NullableReturnedFromNonnull:ShowFixIts.
3. Verify that the fixits are emitted for their respective checkers if -analyzer-config explicitly enables either nullability.NullReturnedFromNonnull:ShowFixIts and/or nullability.NullableReturnedFromNonnull:ShowFixIts.
4. Validates that the fixes are applied correctly and appear as we expect.

[steakhal]

You have a single bool property, yet you allow to enable/disable this by each sub-checker. It feels like the last checker in the registration process will rule them all.

That being said, in the fixit creation scope, you check for both this flag and the presence of the fixit location - which you only set if the flag is active.

IMO you should have this flag per-subchecker, and check for the presence of that and either pass the fixit location if you actually need to insert the fixit or pass it unconditionally and emit the fixit only if the flag of the given sub-checker is set.

Also, make sure the tests pass.

[MosheBerman]

In D123352#3438475, @steakhal wrote:

You have a single bool property, yet you allow to enable/disable this by each sub-checker. It feels like the last checker in the registration process will rule them all.

That being said, in the fixit creation scope, you check for both this flag and the presence of the fixit location - which you only set if the flag is active.

IMO you should have this flag per-subchecker, and check for the presence of that and either pass the fixit location if you actually need to insert the fixit or pass it unconditionally and emit the fixit only if the flag of the given sub-checker is set.

That’s a helpful observation. Besides for the code hygiene, I am completely new to LLVM and C++, so I wasn’t aware that the flags will override. I’ll think more about this and address this feedback in an update to this patch.

Also, make sure the tests pass.

Yep - the reason I posted this diff in the current state was because I’m unsure why the fixit isn’t appearing in the test output. I posted about it on Discourse.

Is there something I need to do besides for attaching the hint to the bug report?

I also found a really old post where @ted_kremenek says that FixItHints were - at the time - not implemented on BugReporter. I don’t know if that’s changed since then, or if the approach has been to use clang-tidy exclusive to supporting fixits in checkers.

Do you have any insight?

Thanks so much for taking the time to look at this and provide thoughtful feedback!

[steakhal]

tldr; static-analyzer fixits are not completely implemented. We don't even have tests for it, which is a shame.
When I passed the apply-fixits, it modified the input source file - as I expected.
Then I tried the -analyzer-output=text and suddenly it inserted the fixit 2 times xD, which is less than ideal and we should fix this.
And I'm expecting many more bugs with this feature.

There is a clang/test/Analysis/check-analyzer-fixit.py script which could be invoked by a RUN line like this:

// RUN: %check-analyzer-fixit %s %t -analyzer-checker=core

At least, the comment of that script says so.

---

I'm quite skeptical about inserting such fixits in general - regarding path-sensitive analysis.
For fixits, we should be confident that some property holds, but the static analyzer might conclude erroneously that a given pointer is null resulting in a bad fixit.
Thus, relying on this might be dangerous.
You can read more about false-positive cases regarding null pointers in the clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def, namely suppress-null-return-paths, avoid-suppressing-null-argument-paths, suppress-inlined-defensive-checks. These options were introduced for mitigating such false-positive null pointer deref reports.

Let me invite @Szelethus for his expertise in the null pointer checker.

[MosheBerman]

In D123352#3439390, @steakhal wrote:

tldr; static-analyzer fixits are not completely implemented.

Where can I learn more about this? Would it be possible and idiomatically/architecturally sounds to write a clang-tidy that processes output from this checker?

When I passed the apply-fixits, it modified the input source file - as I expected.

Did you test this diff, or an existing checker? Would you please share the command you used to test?

Then I tried the -analyzer-output=text and suddenly it inserted the fixit 2 times xD, which is less than ideal and we should fix this.
And I'm expecting many more bugs with this feature.

This is why it's gated. xD.

There is a clang/test/Analysis/check-analyzer-fixit.py script which could be invoked by a RUN line like this:

// RUN: %check-analyzer-fixit %s %t -analyzer-checker=core

At least, the comment of that script says so.

---

I'm quite skeptical about inserting such fixits in general - regarding path-sensitive analysis.
For fixits, we should be confident that some property holds, but the static analyzer might conclude erroneously that a given pointer is null resulting in a bad fixit.
Thus, relying on this might be dangerous.

Understood, and I don't disagree (both because of lack of expertise, and because false-positives is a logical concern.) The intention here is to enable us to add NS_ASSUME macros to a bunch of files, then use the nullability return checkers to catch the functions/methods which violate the new contract. This assumes that the existing code is the source of truth, as opposed to, say, callsites.

You can read more about false-positive cases regarding null pointers in the clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def, namely suppress-null-return-paths, avoid-suppressing-null-argument-paths, suppress-inlined-defensive-checks. These options were introduced for mitigating such false-positive null pointer deref reports.

I'll take a look at that, thanks! Would those suppressions interact with the nullability checkers? They seem to have a lot of defenses against false positives.

Let me invite @Szelethus for his expertise in the null pointer checker.

Thanks!

[MosheBerman]

• Changed the ShowFixIts flag to be per-checker.
• Added support for syntax sugar (nullable vs _Nullable)
  • Pass FixItHint through, so we can do that.
• Changed tests to use -fdiagnostics-parseable-fixits.

This doesn't yet account for removing existing _Nonnull local annotations, but I want to make sure we can get tests working first and/or determine if a tidy is a better approach.

[MosheBerman]

Hey, bumping for feedback, please. :D

[MosheBerman]

The tests now pass and actually verify the behavior.

[steakhal]

In D123352#3439649, @MosheBerman wrote:

In D123352#3439390, @steakhal wrote:

tldr; static-analyzer fixits are not completely implemented.

Where can I learn more about this?

Grep through the code and look for references to the variable. It's not used widely.
In fact, there are no checkers facilitating fixits.

Would it be possible and idiomatically/architecturally sounds to write a clang-tidy that processes output from this checker?

I don't believe that clang-tidy should be involved in this. It would be better to keep it in-house (in the analyzer).

When I passed the apply-fixits, it modified the input source file - as I expected.

Did you test this diff, or an existing checker? Would you please share the command you used to test?

Sort of. I tried to apply some parts of it by hand, and check the output difference in my command.

I cannot immediately recall, but I cannot remember any major obstacles, which suggests that I had to set the apply-fixits=true analyzer config option and that's it.
Since there were no checkers emitting fixit hints, I modified an existing one to emit a dummy fixithint. Nothing fancy there.

Then I tried the -analyzer-output=text and suddenly it inserted the fixit 2 times xD, which is less than ideal and we should fix this.
And I'm expecting many more bugs with this feature.

This is why it's gated. xD.

What do you mean by 'gated'? It seems to be a bug, which we should fix prior to this.

PS: I'm also inviting @whisperity since he is more experienced with fixits and that sort of stuff.

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

160–162

You could pass the owning Decl to this function and directly figure out if it's an objc method or not.

693

?

[MosheBerman]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

160–162

We do this at the callsite. Does it matter where we do the check?

[MosheBerman]

• Improved handling of NS_ASSUME and other edge cases/
• Added test cases, and changed the test to actually validate the output (not just the presence of fixits.)
• Cleaned up the diff.
• Updated the summary to include more detail and address some of the comments.

[MosheBerman]

In D123352#3458530, @steakhal wrote:

In D123352#3439649, @MosheBerman wrote:

In D123352#3439390, @steakhal wrote:

tldr; static-analyzer fixits are not completely implemented.

Where can I learn more about this?

Grep through the code and look for references to the variable. It's not used widely.
In fact, there are no checkers facilitating fixits.

There's a DeadStore checker that does have some fixit code, which is where I found the FixItHint class. I did notice it isn't used in too many other places.

Would it be possible and idiomatically/architecturally sounds to write a clang-tidy that processes output from this checker?

I don't believe that clang-tidy should be involved in this. It would be better to keep it in-house (in the analyzer).

When I passed the apply-fixits, it modified the input source file - as I expected.

Did you test this diff, or an existing checker? Would you please share the command you used to test?

Sort of. I tried to apply some parts of it by hand, and check the output difference in my command.

I cannot immediately recall, but I cannot remember any major obstacles, which suggests that I had to set the apply-fixits=true analyzer config option and that's it.
Since there were no checkers emitting fixit hints, I modified an existing one to emit a dummy fixithint. Nothing fancy there.

Then I tried the -analyzer-output=text and suddenly it inserted the fixit 2 times xD, which is less than ideal and we should fix this.
And I'm expecting many more bugs with this feature.

This is why it's gated. xD.

What do you mean by 'gated'? It seems to be a bug, which we should fix prior to this.

The -analyzer-output=text comes from a frontend. (I misplaced the source right now, but I did see it recently on the LLVM github.) The reasons I'm not too concerned are:

1. It looks like a pre-existing behavior. (The text frontend consumer appeared to be printing fixits independently of whatever other diagnostic flags)
2. When I say gated, I mean that the default behavior of the checker hasn't changed. These fixits are opt-in. If you're opting in, you can also choose to not use the text output.

PS: I'm also inviting @whisperity since he is more experienced with fixits and that sort of stuff.

Thanks! More reviewers means I learn more and can be more accurate.

[MosheBerman]

Fix tests

[steakhal]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

578

You should avoid std::string comparisons. llvm::StringRefs are a better alternative.

581–591

Uh, this is really ugly. I don't believe this is the preferred way of detecting this. @NoQ WDYT?

679–680

clang/test/Analysis/nullability-fixits.mm

6–11

Why don't you use the %clang_analyze_cc1 ... form instead or even better the %check_analyzer_fixit tool-subst pattern?
See the clang/test/Analysis/dead-stores.c as an example.

[MosheBerman]

Thanks for all the thoughtful and fantastic feedback!

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

581–591

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. 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!

clang/test/Analysis/nullability-fixits.mm

6–11

That’s a good call out. I looked at that exact test and was having trouble testing. Instead, I implemented the RUN commands from the most basic invocation I could find.

I’m happy to update the tests that verify that fixits are emitted. For the one that checks that fixits are applied, is there a better way? The -verify flag doesn’t do that, from what I saw in the docs.

[whisperity]

Regarding FixIts... FixIts are implemented in the "Diagnostic" library, which is non-specific to neither Clang-Tidy nor Sema whatsoever, they use the same infrastructure under the hood. Why the apply logic in CSA might do the same FixIt multiple times is beyond me, but I know that both clang-apply-replacements and clang-tidy go to length to ensure that in case multiple checkers report to the same location with potentially conflicting FixIts, then none gets applied, because applying all of them would result in ridiculously broken source code. They internally become an object in the clang::tooling namespace which is implemented as a core Clang library. The relevant entrypoint to this logic, at least in Clang-Tidy, should be this one: http://github.com/llvm/llvm-project/blob/8f9dd5e608c0ac201ab682ccc89ac3be2dfd0d29/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L115-L134

FixIts are very rudimentary under the hood. Essentially, they allow you to, in a file at a given position, delete N characters and insert M characters. Removal fixits only delete, insertion fixits only insert (duh), and just like in version control, a modification is a "removal" and an "insertion" grouped into one action...

If FixIts are so broken in CSA's infrastructure as @steakhal observed then it might very well be a good thing to hold off on them and fix the infrastructure first. Or at least remove the ability to auto-apply them. You don't have to apply the fixes, but you can always get the neat little green-coloured notes as to what could the user should insert.

In D123352#3463063, @MosheBerman wrote:

There's a DeadStore checker that does have some fixit code, which is where I found the FixItHint class. I did notice it isn't used in too many other places.

In D123352#3439390, @steakhal wrote:

For fixits, we should be confident that some property holds, but the static analyzer might conclude erroneously that a given pointer is null resulting in a bad fixit.

Dead stores and dead code are, for the most part, rather easy to reason about and not make dangerous fixits by their removal. (I'd go far enough to say the DeadStore checker should be a run-of-the-mill warning in Sema, and not some extra fluff only available in CSA...)

In D123352#3439649, @MosheBerman wrote:

Where can I learn more about this? Would it be possible and idiomatically/architecturally sounds to write a clang-tidy that processes output from this checker?

Very unlikely. While Clang-Tidy can execute CSA checkers (because CSA is a library within Clang, but AFAIK calling CSA from Tidy doesn't handle things like CTU), Clang-Tidy does not contain any notion of dependency between checks and they are meant to be independent. Moreover, you would need to enforce a sequential execution by the user across binaries (not to mention the fact that the entire project needs to be parsed twice, which might not be a trivial cost!), which seems error-prone...

---

In general, why you are trying to achieve this with the static analyser? It looks to me as if what you want is to mechanically add an annotation to some types if they are in between specific macros / annotations. More specifically, the table that is in your original post doesn't seem to involve path-sensitive information. So could this be a purely (platform-specific?) Tidy check? Your test code also doesn't contain anything that seems to depend on path-sensitive information.

1. Where is this NS_ annotation coming from? Is this a thing of a specific platform, or specific to your company's products?
2. If you are sure you will not depend on any path-sensitive symbolic execution information (which you currently appear you do not), and the answer to 1. is meaningful enough to be useful for a wider community (that is, not just you or your company?), then I would suggest rewriting the whole thing within Clang-Tidy. Clang-Tidy works off of AST matchers directly. The nullability annotations are attributes, which are also AST nodes, so you can handle them similarly to matching types or Decls.

[MosheBerman]

In D123352#3474033, @whisperity wrote:

Regarding FixIts... FixIts are implemented in the "Diagnostic" library, which is non-specific to neither Clang-Tidy nor Sema whatsoever, they use the same infrastructure under the hood. Why the apply logic in CSA might do the same FixIt multiple times is beyond me, but I know that both clang-apply-replacements and clang-tidy go to length to ensure that in case multiple checkers report to the same location with potentially conflicting FixIts, then none gets applied, because applying all of them would result in ridiculously broken source code. They internally become an object in the clang::tooling namespace which is implemented as a core Clang library. The relevant entrypoint to this logic, at least in Clang-Tidy, should be this one: http://github.com/llvm/llvm-project/blob/8f9dd5e608c0ac201ab682ccc89ac3be2dfd0d29/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L115-L134

FixIts are very rudimentary under the hood. Essentially, they allow you to, in a file at a given position, delete N characters and insert M characters. Removal fixits only delete, insertion fixits only insert (duh), and just like in version control, a modification is a "removal" and an "insertion" grouped into one action...

If FixIts are so broken in CSA's infrastructure as @steakhal observed then it might very well be a good thing to hold off on them and fix the infrastructure first. Or at least remove the ability to auto-apply them. You don't have to apply the fixes, but you can always get the neat little green-coloured notes as to what could the user should insert.

The default behavior _does not change_ with this diff. These FixIts are gated by the flags`nullability.NilReturnedToNonnull:ShowFixIts` and nullability.NilReturnedToNonnull:ShowFixIts. Even then -apply-fixits is required to actually apply them.

In D123352#3463063, @MosheBerman wrote:

There's a DeadStore checker that does have some fixit code, which is where I found the FixItHint class. I did notice it isn't used in too many other places.

In D123352#3439390, @steakhal wrote:

For fixits, we should be confident that some property holds, but the static analyzer might conclude erroneously that a given pointer is null resulting in a bad fixit.

In this case, the consequence of a bad fixit is pretty harmless from the perspective of a developer compiling their code. These annotations are not detemining if a pointer is nullptr, but if it can be. The code will still compile, and callsites might have an extra check. (Swift or Obj-C/Cxx) would have an extra check, but it would always pass. The NullabilityChecker has lots of conservative logic to prevent false positives. If we trust the existing checker, we are already pretty low-risk to begin with.

Dead stores and dead code are, for the most part, rather easy to reason about and not make dangerous fixits by their removal. (I'd go far enough to say the DeadStore checker should be a run-of-the-mill warning in Sema, and not some extra fluff only available in CSA...)

In D123352#3439649, @MosheBerman wrote:

Where can I learn more about this? Would it be possible and idiomatically/architecturally sounds to write a clang-tidy that processes output from this checker?

Very unlikely. While Clang-Tidy can execute CSA checkers (because CSA is a library within Clang, but AFAIK calling CSA from Tidy doesn't handle things like CTU), Clang-Tidy does not contain any notion of dependency between checks and they are meant to be independent. Moreover, you would need to enforce a sequential execution by the user across binaries (not to mention the fact that the entire project needs to be parsed twice, which might not be a trivial cost!), which seems error-prone...

Ok, thanks!

---

In general, why you are trying to achieve this with the static analyser? It looks to me as if what you want is to mechanically add an annotation to some types _if they are in between specific macros / annotations._ More specifically, the table that is in your original post doesn't seem to involve path-sensitive information. So could this be a purely (platform-specific?) Tidy check? Your test code also doesn't contain anything that seems to depend on path-sensitive information.

We want is to mechanically add a _nullable_ annotation to some types _when they are incorrectly annotated as nonnull_. A nonnull annotation is "incorrect" when a return value is a nil literal, or the result of a nullable expression. (Macros are only one way to specify a return type as nonnull.) The Nullability Checker emits warnings for exactly this, but doesn't go as far as displaying or applying fixits, and that's what this diff is about.

1. Where is this NS_ annotation coming from? Is this a thing of a specific platform, or specific to your company's products?

You can disregard the NS_ comment. It refers to Apple's NS_ prefixed Objective-C collection classes which the checker appears to special-case no-op on. I was thinking if we could leverage it for those cases, but this is completely auxiliary to what I'm trying to do here, though. I'll delete the comment about that.

2. If you are sure you will not depend on any path-sensitive symbolic execution information (which you currently appear you do not), and the answer to 1. is meaningful enough to be useful for a wider community (that is, not just you or your company?), then I would suggest rewriting the whole thing within Clang-Tidy. Clang-Tidy works off of AST matchers directly.

I've looked at clang-tidy before, and found that matching on return types gets complex once you account for all of the various cases like:
a) implicit cast nodes generated by arc vs no different AST trees when arc is disabled
b) returns of calls to other functions or methods.

While working on this, I referenced NullabilityChecker for a better way to reliably detect these cases and realized I was looking right at what I wanted.

is meaningful enough to be useful for a wider community (that is, not just you or your company

This change will make it easier for everyone to annotate codebases, although has greater impact for larger codebases. Annotating a smaller codebase manually might manageable for a single engineer through the following process:

1. Check the implementations for nil/nullable returns.
2. Write annotations for nullable return types.
3. We can optionally add NS_ASSUME_NONNULL macros to cover the rest.

This is tedious, and with the CSA, we can do better. Today, the CSA already does step 1. With our changes here, we get:

1. CSA checks the implementations for nil/nullable returns.
2. CSA optionally writes annotations for nullable return types.
3. We can optionally add NS_ASSUME_NONNULL macros to cover the rest.

The nullability annotations are attributes, which are also AST nodes, so you can handle them similarly to matching types or Decls.

This raises another issue, which is that inferred nullability Attributes do not carry any information (from what I can tell) that tells us if they are explicitly spelled out or are macro expansions.

They are added in SemaTypes and don't currently track that they've been generated. I think that we can use IsImplicit, but I'm not sure if that is in the spirit of the original diff where it was introduced.

I'm eager to hear your thoughts, and happy to answer any more questions.

[NoQ]

I'm worried that even if the warning is correct, the suggested fix is not necessarily the right solution.

The very nature of path-sensitive warnings requires multiple events to happen on the path in order for the problem to manifest. Eliminating even one of those critical events from the path would eliminate the warning even if all other events are still present. The fix-it hint you're adding targets only one of those events. So it'll eliminate the warning, but so would 2-3 other potential fixes, and there's no way to know which one is actually preferred.

For example, you can eliminate a null-pointer-to-nonnull warning by either changing the "to-nonnull" part (changing the attribute) or by changing the "null-pointer" part (eg., adding a null check before the call). Both fixes make sense depending on the circumstances. Say, if the callee function always crashes on null pointer parameter, annotating the parameter as nullable is a totally wrong thing to do.

So I generally advice against fix-it hints for path-sensitive warnings. I'm curious how you plan to deal with this problem in your specific workflow.

[MosheBerman]

In D123352#3475889, @NoQ wrote:

I'm worried that even if the warning is correct, the suggested fix is not necessarily the right solution.

The very nature of path-sensitive warnings requires multiple events to happen on the path in order for the problem to manifest. Eliminating even one of those critical events from the path would eliminate the warning even if all other events are still present. The fix-it hint you're adding targets only one of those events. So it'll eliminate the warning, but so would 2-3 other potential fixes, and there's no way to know which one is actually preferred.

For example, you can eliminate a null-pointer-to-nonnull warning by either changing the "to-nonnull" part (changing the attribute) or by changing the "null-pointer" part (eg., adding a null check before the call). Both fixes make sense depending on the circumstances. Say, if the callee function always crashes on null pointer parameter, annotating the parameter as nullable is a totally wrong thing to do.

So I generally advice against fix-it hints for path-sensitive warnings. I'm curious how you plan to deal with this problem in your specific workflow.

That's a great point. The FixItHints are only emitted for nil/null-returned, not NTNC or null dereference checks, and this is why. They also only cover return type annotations, and not arguments.

Annotating arguments is more complex, because such an annotation changes the contract between callers to the function/method and also changes -as you noted - uses of the argument inside of the function/method.

(I actually meant to include tests for when arguments are directly returned by a code path. I'll add them.)

[MosheBerman]

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

581–591

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 Attrs 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.

[MosheBerman]

Incorporated feedback:

• Removed confusing comment about NS-prefixed classes
• Use StringRef instead of str
• Fixed case where isa was more appropriate

[MosheBerman]

clang/test/Analysis/nullability-fixits.mm

6–11

Why don't you use the %clang_analyze_cc1 ... form instead or even better the %check_analyzer_fixit tool-subst pattern?
See the clang/test/Analysis/dead-stores.c as an example.

I tried both, and %clang_analyze_cc1 will work. The check_analyzer_fixit pattern fails because the test imports a fake header, which exists at input, but not in the output location.

[MosheBerman]

Update tests to use %clang_analyze_cc1.

[MosheBerman]

Addressed feedback.

• Removed comment about NS-classes
• Changed some of the LIT test invocations
• Added StringRef where appropriate.

[MosheBerman]

My IDE auto-formatted away some whitespace, causing a merge conflict for the test CI.

[MosheBerman]

Missed a whitespace. (Looks like there may be another merge conflict, in NullabilityChecker.cpp:117, but not sure if that's the issue. Let's try this just in case it's not.)

[MosheBerman]

In the interest of not leaving detritus on the internet, I'm going to abandon this diff. I believe there's an alternate approach worth pursuing, with clang tidy. Thanks for your reviews and constructive feedback!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def C11LockChecker : Checker<"C11Lock">,
   Documentation<HasDocumentation>;
 
 } // end "alpha.core"
 
 //===----------------------------------------------------------------------===//
 // Nullability checkers.
 //===----------------------------------------------------------------------===//
 
+// Although only some checkers support fix-its, all of the nullability checkers
+// are registered by the same macro. So let's keep it simple by re-using the
+// same option definition, and document where it is actually used in the helptext.
+// Added bonus: if we _do_ add more nullability hints, the flag is already in place.
+def NullabilityFixIts: CmdLineOption<Boolean,
+                  "ShowFixIts",
+                  "Enable fix-it hints for this nullability checkers. "
+                  "Fix-it hints are only supported by the checkers "
+                  "NullReturnedFromNonnull and NullableReturnedFromNonnull",
+                  "false",
+                  Released>;
+
 let ParentPackage = Nullability in {
 
 def NullabilityBase : Checker<"NullabilityBase">,
   HelpText<"Stores information during the analysis about nullability.">,
+  CheckerOptions<[NullabilityFixIts]>,
   Documentation<NotDocumented>,
   Hidden;
 
 def NullPassedToNonnullChecker : Checker<"NullPassedToNonnull">,
   HelpText<"Warns when a null pointer is passed to a pointer which has a "
            "_Nonnull type.">,
+  CheckerOptions<[NullabilityFixIts]>,
   Dependencies<[NullabilityBase]>,
   Documentation<HasDocumentation>;
 
 def NullReturnedFromNonnullChecker : Checker<"NullReturnedFromNonnull">,
   HelpText<"Warns when a null pointer is returned from a function that has "
            "_Nonnull return type.">,
+  CheckerOptions<[NullabilityFixIts]>,
   Dependencies<[NullabilityBase]>,
   Documentation<HasDocumentation>;
 
 def NullableDereferencedChecker : Checker<"NullableDereferenced">,
   HelpText<"Warns when a nullable pointer is dereferenced.">,
+  CheckerOptions<[NullabilityFixIts]>,
   Dependencies<[NullabilityBase]>,
   Documentation<HasDocumentation>;
 
 def NullablePassedToNonnullChecker : Checker<"NullablePassedToNonnull">,
   HelpText<"Warns when a nullable pointer is passed to a pointer which has a "
            "_Nonnull type.">,
+  CheckerOptions<[NullabilityFixIts]>,
   Dependencies<[NullabilityBase]>,
   Documentation<HasDocumentation>;
 
 def NullableReturnedFromNonnullChecker : Checker<"NullableReturnedFromNonnull">,
   HelpText<"Warns when a nullable pointer is returned from a function that has "
            "_Nonnull return type.">,
+  CheckerOptions<[NullabilityFixIts]>,
   Dependencies<[NullabilityBase]>,
   Documentation<NotDocumented>;
 
 } // end "nullability"
 
 //===----------------------------------------------------------------------===//
 // APIModeling.
 //===----------------------------------------------------------------------===//

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

enum CheckKind {
     CK_NullableDereferenced,
     CK_NullablePassedToNonnull,
     CK_NullableReturnedFromNonnull,
     CK_NumCheckKinds
   };
 
   bool ChecksEnabled[CK_NumCheckKinds] = {false};
   CheckerNameRef CheckNames[CK_NumCheckKinds];
+  DefaultBool ShowFixIts[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds];
 
   const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const {
     if (!BTs[Kind])
       BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability",
                                   categories::MemoryError));
     return BTs[Kind];
   }
private:
     // The tracked region.
     const MemRegion *Region;
   };
 
   /// When any of the nonnull arguments of the analyzed function is null, do not
   /// report anything and turn off the check.
   ///
   /// When \p SuppressPath is set to true, no more bugs will be reported on this
   /// path by this checker.
   void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
                                  ExplodedNode *N, const MemRegion *Region,
                                  CheckerContext &C,
                                  const Stmt *ValueExpr = nullptr,
-                                 bool SuppressPath = false) const;
+                            bool SuppressPath = false,
+                            const llvm::Optional<FixItHint> Hint = None) const;
 
   void reportBug(StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
                  const MemRegion *Region, BugReporter &BR,
-                 const Stmt *ValueExpr = nullptr) const {
+                 const Stmt *ValueExpr = nullptr,
+                 const llvm::Optional<FixItHint> Hint = None) const {
     const std::unique_ptr<BugType> &BT = getBugType(CK);
     auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
     if (Region) {
       R->markInteresting(Region);
       R->addVisitor<NullabilityBugVisitor>(Region);
     }
     if (ValueExpr) {
       R->addRange(ValueExpr->getSourceRange());
       if (Error == ErrorKind::NilAssignedToNonnull ||
           Error == ErrorKind::NilPassedToNonnull ||
           Error == ErrorKind::NilReturnedToNonnull)
         if (const auto *Ex = dyn_cast<Expr>(ValueExpr))
           bugreporter::trackExpressionValue(N, Ex, *R);
     }
+    if (ShowFixIts[CK] && Hint) {
+      R->addFixItHint(*Hint);
+    }
     BR.emitReport(std::move(R));
   }
 
   /// If an SVal wraps a region that should be tracked, it will return a pointer
   /// to the wrapped region. Otherwise it will return a nullptr.
   const SymbolicRegion *getTrackRegion(SVal Val,
                                        bool CheckSuperRegion = false) const;
 
if (checkParamsForPreconditionViolation(Params, State, LocCtxt) ||
     return true;
   }
   return false;
 }
 
 void NullabilityChecker::reportBugIfInvariantHolds(
     StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
     const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr,
-    bool SuppressPath) const {
+    bool SuppressPath, const llvm::Optional<FixItHint> Hint) const {
   ProgramStateRef OriginalState = N->getState();
 
   if (checkInvariantViolation(OriginalState, N, C))
     return;
   if (SuppressPath) {
     OriginalState = OriginalState->set<InvariantViolated>(true);
     N = C.addTransition(OriginalState, N);
   }
 
-  reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr);
+  reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr, Hint);
 }
 
 /// Cleaning up the program state.
 void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
                                           CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   NullabilityMapTy Nullabilities = State->get<NullabilityMap>();
   for (NullabilityMapTy::iterator I = Nullabilities.begin(),

 /// Find the outermost subexpression of E that is not an implicit cast.
 /// This looks through the implicit casts to _Nonnull that ARC adds to
 /// return expressions of ObjC types when the return type of the function or
 /// method is non-null but the express is not.
 static const Expr *lookThroughImplicitCasts(const Expr *E) {
   return E->IgnoreImpCasts();
 }
 
+llvm::Optional<FixItHint> getFixItHint(TypeLoc RetTypeLoc,
+                                       bool CallsiteSupportsSyntaxSugar) {
+  const SourceLocation NullabilityLoc = RetTypeLoc.findNullabilityLoc();
+  const auto CanonicalTypeStr =
+      RetTypeLoc.getType().getCanonicalType().getAsString();
+  if (StringRef(CanonicalTypeStr).equals("id") ||
+      StringRef(CanonicalTypeStr).equals("instancetype")) {
+    return None;
+  }
+  // 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);
+
+  const bool UseSyntaxSugar = CallsiteSupportsSyntaxSugar &&
+                              (!(NullabilityLoc.isValid() &&
+                                 RetTypeLoc.getBeginLoc() < NullabilityLoc) ||
+                               IsInsideOfAssume);
+
+  const auto NewToken =
+      getNullabilitySpelling(NullabilityKind::Nullable, UseSyntaxSugar);
+  const auto ExistingToken =
+      getNullabilitySpelling(NullabilityKind::NonNull, UseSyntaxSugar);
+
+  FixItHint Hint;
+  if (NullabilityLoc.isValid() && !IsInsideOfAssume) {
+    const size_t ExistingTokenSize = ExistingToken.size();
+    Hint = FixItHint::CreateReplacement(
+        SourceRange(NullabilityLoc,
+                    NullabilityLoc.getLocWithOffset(ExistingTokenSize - 1)),
+        NewToken);
+  } else {
+
+    Hint = FixItHint::CreateInsertion(
+        UseSyntaxSugar
+            ? RetTypeLoc.getBeginLoc()
+            : RetTypeLoc.getSourceRange().getBegin().getLocWithOffset(
+                  CanonicalTypeSize),
+        llvm::Twine(NewToken + " ").str());
+  }
+  return Hint;
+}
+
 /// This method check when nullable pointer or null value is returned from a
 /// function that has nonnull return type.
 void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
                                       CheckerContext &C) const {
   auto RetExpr = S->getRetValue();
   if (!RetExpr)
     return;
 
   if (!RetExpr->getType()->isAnyPointerType())
     return;
 
   ProgramStateRef State = C.getState();
   if (State->get<InvariantViolated>())
     return;
 
   auto RetSVal = C.getSVal(S).getAs<DefinedOrUnknownSVal>();
   if (!RetSVal)
     return;
 
   bool InSuppressedMethodFamily = false;
 
   QualType RequiredRetType;
+  TypeLoc RetTypeLoc;
   AnalysisDeclContext *DeclCtxt =
       C.getLocationContext()->getAnalysisDeclContext();
   const Decl *D = DeclCtxt->getDecl();
   if (auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
     // HACK: This is a big hammer to avoid warning when there are defensive
     // nil checks in -init and -copy methods. We should add more sophisticated
     // logic here to suppress on common defensive idioms but still
     // warn when there is a likely problem.
     ObjCMethodFamily Family = MD->getMethodFamily();
     if (OMF_init == Family || OMF_copy == Family || OMF_mutableCopy == Family)
       InSuppressedMethodFamily = true;
 
     RequiredRetType = MD->getReturnType();
+    if (const auto TSI = MD->getReturnTypeSourceInfo()) {
+      RetTypeLoc = TSI->getTypeLoc();
+    }
   } else if (auto *FD = dyn_cast<FunctionDecl>(D)) {
     RequiredRetType = FD->getReturnType();
+    RetTypeLoc = FD->getFunctionTypeLoc().getReturnLoc();
   } else {
     return;
   }
 
   NullConstraint Nullness = getNullConstraint(*RetSVal, State);
 
   Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
 
   // If the returned value is null but the type of the expression
   // generating it is nonnull then we will suppress the diagnostic.
   // This enables explicit suppression when returning a nil literal in a
   // function with a _Nonnull return type:
   //    return (NSString * _Nonnull)0;
   Nullability RetExprTypeLevelNullability =
         getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType());
 
   bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull &&
                                   Nullness == NullConstraint::IsNull);
+  const bool ContextSupportsSyntaxSugar = isa<ObjCMethodDecl>(D);
+  llvm::Optional<FixItHint> Hint =
+      RetTypeLoc ? getFixItHint(RetTypeLoc, ContextSupportsSyntaxSugar) : None;
   if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
       RetExprTypeLevelNullability != Nullability::Nonnull &&
       !InSuppressedMethodFamily && C.getLocationContext()->inTopFrame()) {
     static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
     ExplodedNode *N = C.generateErrorNode(State, &Tag);
     if (!N)
       return;
 
     SmallString<256> SBuf;
     llvm::raw_svector_ostream OS(SBuf);
     OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null");
     OS << " returned from a " << C.getDeclDescription(D) <<
           " that is expected to return a non-null value";
     reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull,
                               CK_NullReturnedFromNonnull, N, nullptr, C,
-                              RetExpr);
+                              RetExpr, false, Hint);
     return;
   }
 
   // If null was returned from a non-null function, mark the nullability
   // invariant as violated even if the diagnostic was suppressed.
   if (NullReturnedFromNonNull) {
     State = State->set<InvariantViolated>(true);
     C.addTransition(State);
if (ChecksEnabled[CK_NullableReturnedFromNonnull] &&
       ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
 
       SmallString<256> SBuf;
       llvm::raw_svector_ostream OS(SBuf);
       OS << "Nullable pointer is returned from a " << C.getDeclDescription(D) <<
             " that is expected to return a non-null value";
 
       reportBugIfInvariantHolds(OS.str(), ErrorKind::NullableReturnedToNonnull,
-                                CK_NullableReturnedFromNonnull, N, Region, C);
+                                CK_NullableReturnedFromNonnull, N, Region, C,
+                                nullptr, false, Hint);
     }
     return;
   }
   if (RequiredNullability == Nullability::Nullable) {
     State = State->set<NullabilityMap>(Region,
                                        NullabilityState(RequiredNullability,
                                                         S));
     C.addTransition(State);

 
 void ento::registerNullabilityBase(CheckerManager &mgr) {
   mgr.registerChecker<NullabilityChecker>();
 }
 
 bool ento::shouldRegisterNullabilityBase(const CheckerManager &mgr) {
   return true;
 }
-
 #define REGISTER_CHECKER(name, trackingRequired)                               \
   void ento::register##name##Checker(CheckerManager &mgr) {                    \
     NullabilityChecker *checker = mgr.getChecker<NullabilityChecker>();        \
     checker->ChecksEnabled[NullabilityChecker::CK_##name] = true;              \
     checker->CheckNames[NullabilityChecker::CK_##name] =                       \
         mgr.getCurrentCheckerName();                                           \
     checker->NeedTracking = checker->NeedTracking || trackingRequired;         \
     checker->NoDiagnoseCallsToSystemHeaders =                                  \
         checker->NoDiagnoseCallsToSystemHeaders ||                             \
         mgr.getAnalyzerOptions().getCheckerBooleanOption(                      \
             checker, "NoDiagnoseCallsToSystemHeaders", true);                  \
+    checker->ShowFixIts[NullabilityChecker::CheckKind::CK_##name] =            \
+        mgr.getAnalyzerOptions().getCheckerBooleanOption(                      \
+            mgr.getCurrentCheckerName(), "ShowFixIts");                        \
   }                                                                            \
                                                                                \
   bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) {        \
     return true;                                                               \
   }
 
 // The checks are likely to be turned on by default and it is possible to do
 // them without tracking any nullability related information. As an optimization
 // no nullability information will be tracked when only these two checks are
 // enables.
 REGISTER_CHECKER(NullPassedToNonnull, false)
 REGISTER_CHECKER(NullReturnedFromNonnull, false)
 
 REGISTER_CHECKER(NullableDereferenced, true)
 REGISTER_CHECKER(NullablePassedToNonnull, true)
 REGISTER_CHECKER(NullableReturnedFromNonnull, true)

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: max-inlinable-size = 100
 // CHECK-NEXT: max-nodes = 225000
 // CHECK-NEXT: max-symbol-complexity = 35
 // CHECK-NEXT: max-times-inline-large = 32
 // CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: model-path = ""
 // CHECK-NEXT: notes-as-events = false
+// CHECK-NEXT: nullability.NullPassedToNonnull:ShowFixIts = false
+// CHECK-NEXT: nullability.NullReturnedFromNonnull:ShowFixIts = false
+// CHECK-NEXT: nullability.NullabilityBase:ShowFixIts = false
+// CHECK-NEXT: nullability.NullableDereferenced:ShowFixIts = false
+// CHECK-NEXT: nullability.NullablePassedToNonnull:ShowFixIts = false
+// CHECK-NEXT: nullability.NullableReturnedFromNonnull:ShowFixIts = false
 // CHECK-NEXT: nullability:NoDiagnoseCallsToSystemHeaders = false
 // CHECK-NEXT: objc-inlining = true
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:CheckPointeeInitialization = false
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:IgnoreGuardedFields = false
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:IgnoreRecordsWithField = ""
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:NotesAsWarnings = false
 // CHECK-NEXT: optin.cplusplus.UninitializedObject:Pedantic = false
 // CHECK-NEXT: optin.cplusplus.VirtualCall:PureOnly = false

clang/test/Analysis/nullability-fixits.mm

+// This test runs the nullability checkers with fixits enabled to verify that the
+// fixits are produced as expected. Note that we disable `-Wnonnull` since it
+// pollutes the output and doesn't provide fixits.
+
+// RUN: %clang_analyze_cc1 \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT
+
+// RUN: %clang_analyze_cc1 \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=false \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=false \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-EXPLICIT
+
+// RUN: %clang_analyze_cc1 \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-ENABLED
+
+// RUN: cp %s %t
+// RUN: %clang_analyze_cc1 \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability \
+// RUN: -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN: -analyzer-config apply-fixits=true %s
+
+// Use grep to skip lines with comments to avoid matching on checks or RUN lines:
+// RUN: cat %s | grep -v -E "\/\/|^\s*\$" | FileCheck %t -check-prefix=CHECK-FIXIT-VERIFY-OUT
+// RUN: cp %t %s
+
+// CHECK-FIXIT-ENABLED: fix-it:"{{.*[/\\]*}}clang/test/Analysis/nullability-fixits.mm"
+// CHECK-FIXIT-DISABLED-EXPLICIT-NOT: fix-it:"{{.*[/\\]*}}clang/test/Analysis/nullability-fixits.mm"
+// CHECK-FIXIT-DISABLED-IMPLICIT-NOT: fix-it:"{{.*[/\\]*}}clang/test/Analysis/nullability-fixits.mm"
+
+#include "Inputs/system-header-simulator-for-nullability.h"
+
+@interface TestObject : NSObject
+@end
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nullable)returnsNil
+- (TestObject *_Nonnull)returnsNil {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject<NSObject> *_Nullable)returnsNilProtocol
+- (TestObject<NSObject> *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject<NSObject> *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject<NSObject> *)returnsNilProtocolSyntaxSugar {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (id)returnsNilId
+- (id)returnsNilId {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (instancetype)returnsNilInstancetype
+- (instancetype)returnsNilInstancetype {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilSyntaxSugar
+- (nonnull TestObject *)returnsNilSyntaxSugar {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nonnull)returnsNilSuppressed
+- (TestObject *_Nonnull)returnsNilSuppressed {
+  return (TestObject *_Nonnull)nil;
+}
+
+// Implicitly unspecified are not auto-fixed.
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *)returnsNilUnannotated
+- (TestObject *)returnsNilUnannotated {
+  return nil;
+}
+
+// Implicitly unspecified are not auto-fixed.
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Null_unspecified)returnsNilUnannotatedExplicitlyUnspecified
+- (TestObject *_Null_unspecified)returnsNilUnannotatedExplicitlyUnspecified {
+  return nil;
+}
+
+NS_ASSUME_NONNULL_BEGIN
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilAssumeInClass
+- (TestObject *)returnsNilAssumeInClass {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nullable)returnsNilAssumeInClass
+- (TestObject *_Nonnull)returnsNilAssumeInClassAnnotated {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilAssumeInClassAnnotatedSyntaxSugar
+- (nonnull TestObject *)returnsNilAssumeInClassAnnotatedSyntaxSugar {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Null_unspecified)returnsNilAssumeInClassExplicitlyUnspecified
+- (TestObject *_Null_unspecified)returnsNilAssumeInClassExplicitlyUnspecified {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (id)returnsNilIdAssume
+- (id)returnsNilIdAssume {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (instancetype)returnsNilInstancetypeAssume
+- (instancetype)returnsNilInstancetypeAssume {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject<NSObject> *)returnsNilProtocolAssume
+- (TestObject<NSObject> *)returnsNilProtocolAssume {
+  return nil;
+}
+NS_ASSUME_NONNULL_END
+
+// Self is not nil here.
+// CHECK-FIXIT-VERIFY-OUT: - (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+  TestObject *o = [self returnsNil];
+  return o;
+}
+@end
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Nullable returnsNilObjCInstanceIndirectly() {
+NSObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+  TestObject *local = nil;
+  return local;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: TestObject *_Nullable returnsNilObjCInstanceDirectly() {
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+  return nil;
+}
+
+NS_ASSUME_NONNULL_BEGIN
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Nullable returnsNilFuncAssume() {
+NSObject *returnsNilFuncAssume() {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: TestObject<NSObject> *_Nullable returnsNilFuncProtocolAssume() {
+TestObject<NSObject> *returnsNilFuncProtocolAssume() {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Null_unspecified returnsNilFuncAssumeNonnullExplicitlyUnspecified() {
+NSObject *_Null_unspecified returnsNilFuncAssumeNonnullExplicitlyUnspecified() {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: NSObject *_Nullable returnsNilFuncAssumeNonnullExplicitlyAnnotated() {
+NSObject *_Nonnull returnsNilFuncAssumeNonnullExplicitlyAnnotated() {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: id returnsIdFunc() {
+id returnsIdFunc() {
+  return nil;
+}
+
+// We don't need to check instancetype because this is outside a class
+
+NS_ASSUME_NONNULL_END