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

[Moshe via Phabricator via cfe-commits]

MosheBerman abandoned this revision.
MosheBerman added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 430691.
MosheBerman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,185 @@
+// 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 *_Nullable)returnsNilProtocol
+- (TestObject *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject *)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 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 430686.
MosheBerman marked an inline comment as not done.
MosheBerman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,185 @@
+// 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 *_Nullable)returnsNilProtocol
+- (TestObject *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject *)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 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 430408.
MosheBerman marked an inline comment as done and 2 inline comments as not done.
MosheBerman added a comment.

Addressed feedback.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,185 @@
+// 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 *_Nullable)returnsNilProtocol
+- (TestObject *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject *)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 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 429068.
MosheBerman added a comment.

Update tests to use %clang_analyze_cc1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,185 @@
+// 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_cc1 -analyze \
+// 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_cc1 -analyze \
+// 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_cc1 -analyze \
+// 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_cc1 -analyze \
+// 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 *_Nullable)returnsNilProtocol
+- (TestObject *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject *)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;
+}
+
+// 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added inline comments.



Comment at: clang/test/Analysis/nullability-fixits.mm:5-10
+// RUN: %clang_cc1 -analyze \
+
+// RUN: -Wno-nonnull \
+// RUN: 
-analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull
 \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT

MosheBerman wrote:
> steakhal wrote:
> > 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.
> 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. 
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 429065.
MosheBerman added a comment.

Incorporated feedback:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,185 @@
+// 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_cc1 -analyze \
+// 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_cc1 -analyze \
+// 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_cc1 -analyze \
+// 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_cc1 -analyze \
+// 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 *_Nullable)returnsNilProtocol
+- (TestObject *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject *)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 

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added a comment.

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 fixits are only generated for nil/null-returned, not 
the other nullability checks, and this is why. I actually meant to include 
tests for when arguments are directly returned by a code path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added a comment.

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 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman planned changes to this revision.
MosheBerman added a comment.

Thanks for all the thoughtful and fantastic feedback!




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);
+

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!



Comment at: clang/test/Analysis/nullability-fixits.mm:5-10
+// RUN: %clang_cc1 -analyze \
+
+// RUN: -Wno-nonnull \
+// RUN: 
-analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull
 \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT

steakhal wrote:
> 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.
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 424048.
MosheBerman added a comment.

Fix tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,184 @@
+// 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_cc1 -analyze \
+
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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 *_Nullable)returnsNilProtocol
+- (TestObject *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject *)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: - 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 424010.
MosheBerman edited the summary of this revision.
MosheBerman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,185 @@
+// 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_cc1 -analyze \
+
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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 *_Nullable)returnsNilProtocol
+- (TestObject *_Nonnull)returnsNilProtocol {
+  return nil;
+}
+
+// CHECK-FIXIT-VERIFY-OUT: - (nullable TestObject *)returnsNilProtocolSyntaxSugar
+- (nonnull TestObject *)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 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:162-164
+  /// 2. If we are annotating an Objective-C method, and not a function, we
+  ///want to use the `nullable` form instead of `_Nullable`.
+  ///When \p SyntaxSugar is true, we handle the second case.

steakhal wrote:
> You could pass the owning Decl to this function and directly figure out if 
> it's an objc method or not.
We do this at the callsite. Does it matter where we do the check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 422922.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,70 @@
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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
+
+
+// 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
+
+NSObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+  TestObject *local = nil;
+  return local;
+}
+
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+  return nil;
+}
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+- (TestObject *_Nonnull)returnsNilUnsuppressed {
+  return nil;
+}
+
+- (TestObject *_Nullable)returnsNil {
+  return (TestObject *_Nonnull)nil;
+}
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+  TestObject *o = [self returnsNil];
+  return o;
+}
+@end
+
+NS_ASSUME_NONNULL_BEGIN
+
+NSObject * returnsNilAssumeNonnull() {
+  return nil;
+}
+
+NS_ASSUME_NONNULL_END
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -117,6 +117,9 @@
 
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckerNameRef CheckNames[CK_NumCheckKinds];
+  // FIXME: Should we consider changing the invariant behavior for `nil`
+  // and/or NS collection classes if this is enabled?
+  DefaultBool ShowFixIts[CK_NumCheckKinds];
   mutable std::unique_ptr BTs[CK_NumCheckKinds];
 
   const std::unique_ptr (CheckKind Kind) const {
@@ -152,6 +155,20 @@
 const MemRegion *Region;
   };
 
+  /// There are a few cases where we want to move the insertion point.
+  /// 1. If we are inserting after the pointer, we need to offset ourselves,
+  ///because `getReturnTypeSourceRange` does not include qualifiers so the
+  ///annotation would be inserted incorrectly before the pointer symbol.
+  /// 2. If we are annotating an Objective-C method, and not a function, we
+  ///want to use the `nullable` form instead of `_Nullable`.
+  ///When \p SyntaxSugar is true, we handle the second case.
+  SourceLocation getInsertionLocForTypeInfo(TypeSourceInfo *TypeSourceInfo,
+const size_t Offset = 0,
+bool SyntaxSugar = false) const {
+return TypeSourceInfo->getTypeLoc().getBeginLoc().getLocWithOffset(
+SyntaxSugar ? 0 : Offset);
+  }
+
   /// When any of the nonnull arguments of the analyzed function is null, do not
   /// report anything and turn off the check.
   ///
@@ -160,12 +177,13 @@
   void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
  ExplodedNode *N, const MemRegion *Region,
  CheckerContext ,
-  

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 422902.
MosheBerman added a comment.

The tests now pass and actually verify the behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,70 @@
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// RUN: -fdiagnostics-parseable-fixits %s 2>%t
+// RUN: cat %t | FileCheck %s -check-prefix=CHECK-FIXIT-DISABLED-IMPLICIT
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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_cc1 -analyze \
+// RUN: -Wno-nonnull \
+// RUN: -analyzer-checker=core,nullability.NullableReturnedFromNonnull,nullability.NullReturnedFromNonnull \
+// 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
+
+
+// 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
+
+NSObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+  TestObject *local = nil;
+  return local;
+}
+
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+  return nil;
+}
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+- (TestObject *_Nonnull)returnsNilUnsuppressed {
+  return nil;
+}
+
+- (TestObject *_Nullable)returnsNil {
+  return (TestObject *_Nonnull)nil;
+}
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+  TestObject *o = [self returnsNil];
+  return o;
+}
+@end
+
+NS_ASSUME_NONNULL_BEGIN
+
+NSObject * returnsNilAssumeNonnull() {
+  return nil;
+}
+
+NS_ASSUME_NONNULL_END
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -117,6 +117,9 @@
 
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckerNameRef CheckNames[CK_NumCheckKinds];
+  // FIXME: Should we consider changing the invariant behavior for `nil`
+  // and/or NS collection classes if this is enabled?
+  DefaultBool ShowFixIts[CK_NumCheckKinds];
   mutable std::unique_ptr BTs[CK_NumCheckKinds];
 
   const std::unique_ptr (CheckKind Kind) const {
@@ -152,6 +155,20 @@
 const MemRegion *Region;
   };
 
+  /// There are a few cases where we want to move the insertion point.
+  /// 1. If we are inserting after the pointer, we need to offset ourselves,
+  ///because `getReturnTypeSourceRange` does not include qualifiers so the
+  ///annotation would be inserted incorrectly before the pointer symbol.
+  /// 2. If we are annotating an Objective-C method, and not a function, we
+  ///want to use the `nullable` form instead of `_Nullable`.
+  ///When \p SyntaxSugar is true, we handle the second case.
+  SourceLocation getInsertionLocForTypeInfo(TypeSourceInfo *TypeSourceInfo,
+const size_t Offset = 0,
+bool SyntaxSugar = false) const {
+return TypeSourceInfo->getTypeLoc().getBeginLoc().getLocWithOffset(
+SyntaxSugar ? 0 : Offset);
+  }
+
   /// When any of the nonnull arguments of the analyzed function is null, do not
   /// report anything and turn off the check.
   ///
@@ -160,12 +177,13 @@
   void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
  ExplodedNode *N, const 

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added a comment.

Hey, bumping for feedback, please. :D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman updated this revision to Diff 421587.
MosheBerman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,52 @@
+// RUN: %clang_analyzer_cc1 %s -analyzer-checker=core,nullability.NullReturnedFromNonnull,nullability.NullableReturnedFromNonnull \
+// RUN:   -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN:   -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN:   -fobjc-arc \
+// RUN:   -fdiagnostics-parseable-fixits \
+// RUN:   2&>1 -o - | FileCheck %s -check-prefix=FIXIT
+
+// This test runs the nullability checkers with fixits enabled. We're piping
+// the diagnostic output to FileCheck, not output of the clang invocation.
+
+#define nil 0
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+- (instancetype)autorelease;
+- (void)release;
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject
+@end
+
+@interface TestObject : NSObject
+@end
+
+TestObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+  TestObject *local = nil; // FIXIT: fix-it:{{.*}}:27}:"nullable TestObject"
+  return local;
+}
+
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+  return nil; // FIXIT: fix-it:{{.*}}:32}:"TestObject *_Nullable"
+}
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+- (TestObject *_Nonnull)returnsNilUnsuppressed {
+  return nil; // FIXIT: fix-it:{{.*}}:41}:"nullable TestObject *"
+}
+
+- (TestObject *_Nullable)returnsNil {
+  return (TestObject *_Nonnull)nil;
+}
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+  TestObject *o = [self returnsNil];
+  return o;
+}
+@end
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -117,6 +117,9 @@
 
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckerNameRef CheckNames[CK_NumCheckKinds];
+  // FIXME: Should we consider changing the invariant behavior for `nil`
+  // and/or NS collection classes if this is enabled?
+  DefaultBool ShowFixIts[CK_NumCheckKinds];
   mutable std::unique_ptr BTs[CK_NumCheckKinds];
 
   const std::unique_ptr (CheckKind Kind) const {
@@ -152,6 +155,20 @@
 const MemRegion *Region;
   };
 
+  /// There are a few cases where we want to move the insertion point.
+  /// 1. If we are inserting after the pointer, we need to offset ourselves,
+  ///because `getReturnTypeSourceRange` does not include qualifiers so the
+  ///annotation would be inserted incorrectly before the pointer symbol.
+  /// 2. If we are annotating an Objective-C method, and not a function, we
+  ///want to use the `nullable` form instead of `_Nullable`.
+  ///When \p SyntaxSugar is true, we handle the second case.
+  SourceLocation getInsertionLocForTypeInfo(TypeSourceInfo *TypeSourceInfo,
+const size_t Offset = 0,
+bool SyntaxSugar = false) const {
+return TypeSourceInfo->getTypeLoc().getBeginLoc().getLocWithOffset(
+SyntaxSugar ? 0 : Offset);
+  }
+
   /// When any of the nonnull arguments of the analyzed function is null, do not
   /// report anything and turn off the check.
   ///
@@ -160,12 +177,13 @@
   void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
  ExplodedNode *N, const MemRegion *Region,
  CheckerContext ,
- const Stmt *ValueExpr = nullptr,
- bool SuppressPath = false) const;
+  const Stmt *ValueExpr = nullptr, bool SuppressPath = false,
+  const FixItHint *Hint = nullptr) const;
 
   void reportBug(StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N,
  const MemRegion *Region, BugReporter ,
- const Stmt *ValueExpr = nullptr) const {

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman added a comment.

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][1].

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

I also found [a really old post][2] where @ted_kremenek says that `FixItHint`s 
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!

[1]: 
https://discourse.llvm.org/t/help-testing-fix-it-hints-in-existing-checker/61565
[2]: https://discourse.llvm.org/t/analysis-vs-fixit/14882/2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123352

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Moshe via Phabricator via cfe-commits]

MosheBerman created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, 
xazax.hun.
Herald added a project: All.
MosheBerman requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diff adds:

- 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.)
- For each of the two `*ReturnedFromNonnull` methods, attaches a `FixItHint` to 
the output.

Use Case:
This enables us to automate the process of annotating headers with 
`NS_ASSUME_NONNULL_BEGIN/END` because the checker can fix callsites where we 
would otherwise break the nullability contract.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123352

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/test/Analysis/nullability-fixits.mm

Index: clang/test/Analysis/nullability-fixits.mm
===
--- /dev/null
+++ clang/test/Analysis/nullability-fixits.mm
@@ -0,0 +1,56 @@
+// RUN: %clang_analyze_cc1 %s -analyzer-checker=core,nullability.NullReturnedFromNonnull,nullability.NullableReturnedFromNonnull \
+// RUN:   -analyzer-config nullability.NullReturnedFromNonnull:ShowFixIts=true \
+// RUN:   -analyzer-config nullability.NullableReturnedFromNonnull:ShowFixIts=true \
+// RUN:   -analyzer-config apply-fixits=true \
+// RUN:   -analyzer-output=plist \
+// RUN:   -fobjc-arc \
+// RUN:   -o - | FileCheck %s
+
+#define nil 0
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+- (instancetype)autorelease;
+- (void)release;
+@end
+
+__attribute__((objc_root_class))
+@interface
+NSObject
+@end
+
+
+@interface TestObject : NSObject
+@end
+
+// CHECK: TestObject *_Nullable
+TestObject *_Nonnull returnsNilObjCInstanceIndirectly() {
+  TestObject *local = nil;
+  return local; // expected-warning {{nil returned from a function that is expected to return a non-null value}}
+}
+
+TestObject *_Nonnull returnsNilObjCInstanceDirectly() {
+  return nil; // expected-warning {{nil returned from a function that is expected to return a non-null value}}
+}
+
+@interface ClassWithInitializers : NSObject
+@end
+
+@implementation ClassWithInitializers
+
+// FIXME: We want this check for when we add support for syntactic sugar, and
+// put the annotation before the type name. `nullable` vs `_Nonnull`.
+// CHECK: - (TestObject *_Nonnull) returnsNilUnsuppressed {
+- (TestObject *_Nonnull)returnsNilUnsuppressed {
+  return nil; // expected-warning {{nil returned from a method that is expected to return a non-null value}}
+}
+
+- (TestObject *_Nullable)returnsNil {
+  return (TestObject *_Nonnull)nil;
+}
+- (TestObject *_Nonnull)inlineOfReturnsNilObjCInstanceDirectlyWithSuppressingCast {
+  TestObject *o = [self returnsNil];
+  return o;
+}
+@end
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -92,6 +92,11 @@
   // libraries.
   DefaultBool NoDiagnoseCallsToSystemHeaders;
 
+  // If true, the checker will generate Fix-it-hints for *ReturnedToNonnull
+  // warnings. Should we consider changing the invariant behavior for `nil`
+  // and/or NS collection classes if this is enabled?
+  DefaultBool ShowFixIts;
+
   void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext ) const;
   void checkPostStmt(const ExplicitCastExpr *CE, CheckerContext ) const;
   void checkPreStmt(const ReturnStmt *S, CheckerContext ) const;
@@ -152,6 +157,14 @@
 const MemRegion *Region;
   };
 
+  // `getReturnTypeSourceRange` does not include qualifiers. As a result, the
+  // nullability annotation will precede the asterisk, instead of following it.
+  // This helper function accounts for that.
+  SourceLocation getInsertionLocForTypeInfo(TypeSourceInfo *typeSourceInfo,
+const size_t offset = 0) const {
+return typeSourceInfo->getTypeLoc().getBeginLoc().getLocWithOffset(offset);
+  }
+
   /// When any of the nonnull arguments of the analyzed function is null, do not
   /// report anything and turn off the check.
   ///
@@ -160,12 +173,13 @@
   void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK,
  ExplodedNode *N, const MemRegion *Region,
  CheckerContext ,
- const Stmt *ValueExpr = nullptr,
- bool SuppressPath = false) const;
+  const Stmt