C#: C#8 Nullable expressions and type annotations#1408

Merged

aibaars merged 13 commits intogithub:masterfrom

calumgrant:cs/suppress-null-expr

Jun 28, 2019

Contributor

calumgrant commented Jun 5, 2019

No description provided.

Contributor

pavgust commented Jun 10, 2019

This pull request introduces 3 alerts and fixes 1 when merging e219a77 into c03e804 - view on LGTM.com

new alerts:

1 for Generic catch clause
1 for Useless assignment to local variable
1 for Futile conditional

fixed alerts:

1 for Generic catch clause

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

calumgrant commented

View reviewed changes

csharp/ql/test/library-tests/csharp8/NullableRefTypes.ql

		@@ -0,0 +1,17 @@
		import csharp

		query predicate suppressNullableWarnings(SuppressNullableWarningExpr e, Expr op) {

Contributor Author

calumgrant Jun 11, 2019

Need to add an exclusion in the nullness library that this is a non-null expression.

calumgrant commented

View reviewed changes

csharp/ql/src/semmlecode.csharp.dbscheme Outdated

+               * 1 = not null
+               * 2 = may be null
+               */
+              nullable_flow_state(unique int expr: @expr ref, int value: int ref);

Contributor Author

calumgrant Jun 11, 2019

Delete this for now.

calumgrant commented

View reviewed changes

csharp/ql/src/semmlecode.csharp.dbscheme Outdated

                 int id: @type_parameter_constraints ref,
-                int base_id: @type_or_ref ref);
+                int base_id: @type_or_ref ref,
+                int nullability: int ref);

Contributor Author

calumgrant Jun 11, 2019

nullability -> annotation

Contributor Author

calumgrant Jun 11, 2019

Maybe this should live on the side, for example

specific_type_parameter_annotation(id, base_id, annotation);

calumgrant commented

View reviewed changes

csharp/ql/src/semmlecode.csharp.dbscheme Outdated

+              /** The nullability of type arguments of a constructed type or method. */
+              #keyset[constructedgeneric, position]
+              nullable_type_arguments(int constructedgeneric: @generic ref, int position: int ref, int nullability: int ref);

Contributor Author

calumgrant Jun 11, 2019

generalize, nullability -> annotation

calumgrant commented

View reviewed changes

csharp/ql/test/library-tests/csharp8/NullableRefTypes.cs Outdated

+                      Console.WriteLine(x);
+                  }
+                  string? field3;

Contributor Author

calumgrant Jun 11, 2019

Delete

calumgrant commented

View reviewed changes

csharp/ql/src/semmle/code/csharp/Nullability.qll Outdated

		@@ -0,0 +1,144 @@
		/**
		* Provides predicates classes for representing the nullability of various C# declarations.

Contributor Author

calumgrant Jun 11, 2019

Generalize this to type annotations.

Contributor Author

calumgrant Jun 11, 2019

Including ref and out

calumgrant commented

View reviewed changes

csharp/ql/src/semmle/code/csharp/Nullability.qll Outdated

+              module Nullability {
+                /** Holds if the flow state of the expression has determined that `e` may be null. */
+                predicate exprMayBeNull(Expr e) { nullable_flow_state(e, 2) }

Contributor Author

calumgrant Jun 11, 2019

Delete for now.

calumgrant commented

View reviewed changes

csharp/ql/src/semmle/code/csharp/Nullability.qll Outdated

+                }
+                private newtype TAnnotatedType =
+                  TAnnotatedTypeNullability(Type type, int nullability) {

Contributor Author

calumgrant Jun 11, 2019

Generalize this, to string, string? ref, string ref, string?
Don't just talk about Nullability.

Contributor Author

calumgrant Jun 11, 2019

Represent this as a set of integer, represented as a bit field.

calumgrant commented

View reviewed changes

csharp/ql/src/semmle/code/csharp/Nullability.qll Outdated

+                class AnnotatedType extends TAnnotatedType {
+                  Type underlyingType;
+                  int nullability;

Contributor Author

calumgrant Jun 11, 2019

Change this to annotation, and predicate predicate hasAnnotation(TypeAnnotation a). Maybe an IPA type for now.

calumgrant commented

View reviewed changes

csharp/ql/src/semmle/code/csharp/Nullability.qll Outdated

+                /** Gets the annotated type of element `e`. */
+                cached
+                AnnotatedType getAnnotatedType(Element e) {

Contributor Author

calumgrant Jun 11, 2019

library predicate

Contributor

pavgust commented Jun 11, 2019

This pull request introduces 1 alert and fixes 1 when merging eb20d64 into 7790ac4 - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

calumgrant force-pushed the cs/suppress-null-expr branch from eb20d64 to d9e6602 Compare

June 14, 2019 10:47

calumgrant changed the title ~~C#: C#8 Nullable warning suppression expressions~~ C#: C#8 Nullable expressions and type annotations

Contributor

pavgust commented Jun 14, 2019

This pull request introduces 1 alert and fixes 1 when merging d9e6602 into 41d5d5a - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

Contributor

pavgust commented Jun 14, 2019

This pull request introduces 1 alert and fixes 1 when merging 01895bb into 41d5d5a - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

calumgrant commented

View reviewed changes

csharp/ql/src/semmle/code/csharp/Callable.qll

                 /** Holds if this callable returns a `ref readonly`. */
-                predicate returnsRefReadonly() { ref_readonly_returns(this) }
+                deprecated predicate returnsRefReadonly() {

Contributor Author

calumgrant Jun 17, 2019

Say what to use instead.

calumgrant force-pushed the cs/suppress-null-expr branch from 01895bb to 4356cbd Compare

June 17, 2019 12:13

hvitved requested changes

View reviewed changes

Contributor

hvitved left a comment

First review of the dbscheme changes, and (light) review of AnnotatedType.qll. I really like the design 👍

csharp/ql/src/semmlecode.csharp.dbscheme Outdated Show resolved Hide resolved

csharp/ql/src/semmlecode.csharp.dbscheme Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/AnnotatedType.qll Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/AnnotatedType.qll Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/AnnotatedType.qll Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/AnnotatedType.qll Show resolved Hide resolved

csharp/ql/src/semmlecode.csharp.dbscheme Show resolved Hide resolved

calumgrant force-pushed the cs/suppress-null-expr branch 2 times, most recently from e8a1288 to 6c12c88 Compare

June 19, 2019 14:28

calumgrant marked this pull request as ready for review

June 19, 2019 14:56

calumgrant requested a review from a team as a code owner

June 19, 2019 14:56

calumgrant force-pushed the cs/suppress-null-expr branch from a458f40 to e60ae58 Compare

June 21, 2019 12:04

hvitved requested changes

View reviewed changes

Contributor

hvitved left a comment

Truly awesome stuff, Calum. As I have said earlier, I really like the type annotation design in QL.

change-notes/1.22/analysis-csharp.md Outdated Show resolved Hide resolved

csharp/extractor/Semmle.Extraction.CSharp/Tuples.cs Outdated Show resolved Hide resolved

csharp/extractor/Semmle.Extraction.CSharp/Kinds/TypeAnnotation.cs Outdated Show resolved Hide resolved

csharp/extractor/Semmle.Extraction.CSharp/SymbolExtensions.cs Outdated Show resolved Hide resolved

csharp/extractor/Semmle.Extraction.CSharp/SymbolExtensions.cs Outdated Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/AnnotatedType.qll Outdated Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/AnnotatedType.qll Outdated Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/Generics.qll Outdated Show resolved Hide resolved

csharp/ql/src/semmle/code/csharp/Type.qll Show resolved Hide resolved

csharp/ql/test/library-tests/csharp8/NullableRefTypes.cs Outdated Show resolved Hide resolved

hvitved reviewed

View reviewed changes

csharp/ql/test/library-tests/csharp8/NullableRefTypes.expected Outdated Show resolved Hide resolved

calumgrant force-pushed the cs/suppress-null-expr branch 2 times, most recently from 8d154c4 to fc15412 Compare

June 26, 2019 11:48

calumgrant added 2 commits

June 26, 2019 20:24


          C#: Implement type annotations for nullability, parameter kinds and m…

4aa1947

…ethod returns.


          C#: Stats update

31655c2

calumgrant added 9 commits

June 26, 2019 20:24


          C#: Do not extract type annotations that can be deduced. Put specific…

f909965

…_type_parameter_annotation on the side of specific_type_parameter_constraints.


          C#: DB upgrade script.

b7e8f46


          C#: Analysis change notes

64534d4


          C#: Make Annotations module private. Update comments.

d46848e


          C#: Remove disabled and not-applicable nullability annotations since …

abe961f

…they can be deduced.


          C#: Fix qltests.

35ecb94


          C#: Address review comments part 1.

620ecc8


          C#: Address review comments. Fix up toStringWithTypes(), and deprecat…

abf43da

…e predicates in TypeParameterConstraints.


          C#: Fix formatting of arrays and NullableTypes

76454ed

calumgrant force-pushed the cs/suppress-null-expr branch from fc15412 to 76454ed Compare

June 26, 2019 19:25

lgtm-com bot commented Jun 26, 2019

This pull request introduces 1 alert and fixes 1 when merging 76454ed into 1a9f362 - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause


          C#: Remove use of deprecated predicates.

lgtm-com bot commented Jun 27, 2019

This pull request introduces 1 alert and fixes 1 when merging 2504754 into 7ff6d82 - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

hvitved reviewed

View reviewed changes

Contributor

hvitved left a comment

LGTM, just two small comments.

csharp/ql/src/semmle/code/csharp/Generics.qll Outdated

                 override UnboundGenericType getUnboundGeneric() { constructed_generic(this, getTypeRef(result)) }
+                language[monotonicAggregates]
+                string annotatedTypeArgumentsToString() {

Contributor

hvitved Jun 28, 2019

Make private.

csharp/extractor/Semmle.Extraction.CSharp/Kinds/TypeAnnotation.cs

+              namespace Semmle.Extraction.Kinds
+              {
+                  [Flags]

Contributor

hvitved Jun 28, 2019

Where is it used like a flag set in the code?

Contributor Author

calumgrant Jun 28, 2019

It isn't, currently, however it is "morally" a set and could be used in that way.


          C#: Address review comment

4d38300

hvitved approved these changes

View reviewed changes

lgtm-com bot commented Jun 28, 2019

This pull request introduces 1 alert and fixes 1 when merging 4d38300 into a554369 - view on LGTM.com

new alerts:

1 for Generic catch clause

fixed alerts:

1 for Generic catch clause

Contributor

aibaars commented Jun 28, 2019

The related internal PR has been merged.

aibaars merged commit af68fd4 into github:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet