C#: Rewrite nullness queries#593
Merged
Merged
Conversation
Port many of the nullness test from Java, as well as add new tests.
Port the SSA-based logic from the Java nullness analyses.
For example, in
```
void M(object x)
{
var y = x != null ? "" : null;
if (y != null)
x.ToString();
}
```
the guard `y != null` implies that the guard `x != null` must be true.
For example, in
```
void M(object x)
{
var y = x == null ? 1 : 2;
if (y == 2)
x.ToString();
}
```
the guard `y == 2` implies that the guard `x == null` must be false,
as the assignment of `2` to `y` is unique.
hvitved
force-pushed
the
csharp/nullness
branch
from
86084d0 to
d25bd59
Compare
jf205
reviewed
Dec 4, 2018
jf205
left a comment
jf205
left a comment
Contributor
There was a problem hiding this comment.
The 'Changes to existing queries' table in the change-note isn't formatted correctly at the moment. Otherwise, the text and the qhelp files LGTM.
| @@ -11,6 +11,8 @@ | |||
|
|
|||
| | *@name of query (Query ID)*| *Impact on results* | *How/why the query has changed* | | |||
| | Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. | | |||
Contributor
There was a problem hiding this comment.
This table needs a row of pipes and dashes (e.g. |--------|----------|---------|) beneath the column headings.
calumgrant
reviewed
Dec 6, 2018
calumgrant
left a comment
calumgrant
left a comment
Contributor
There was a problem hiding this comment.
I have to say this is extremely awesome! A few minor comments that's all.
| <overview> | ||
| <p>If a variable is dereferenced, and the variable has a null value on all possible execution paths | ||
| leading to the dereferencing, it is guaranteed to result in a <code>NullReferenceException</code>. | ||
| <p>If a variable is dereferenced, and the variable has a <code>null</code> |
Contributor
There was a problem hiding this comment.
Might be worth explaining what "dereference" means.
| return !(b1 == b2); | ||
| } | ||
|
|
||
| public struct CoOrds |
| { | ||
| public int x, y; | ||
|
|
||
| public CoOrds(int p1, int p2) |
| select access, "Variable $@ is always null here.", var, var.getName() | ||
| from Dereference d, Ssa::SourceVariable v | ||
| where d.isFirstAlwaysNull(v) | ||
| select d, "Variable '$@' is always null here.", v, v.toString() |
Contributor
There was a problem hiding this comment.
Single quotes are not needed around substitutions.
calumgrant
approved these changes
Dec 7, 2018
This was referenced Nov 29, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR rewrites the nullness queries from scratch, very much based on a similar rewrite for Java done by @aschackmull. Most of the work is done in 055351a, and in addition to reviewing commit-by-commit, I suggest reviewing
Nullness.qllon that commit as-is, rather than looking at the diff.The maybe-
nullquery still has some false positives, exemplified via tests ported from Java. My plan is to address those FPs (using control flow graph splitting) in follow-up pull requests.Review: @calumgrant (QL), @jf205 (qhelp + change notes).