Skip to content

Enable nullability checks on Semmle.Extraction.CIL #4113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 28, 2020

Conversation

tamasvajk
Copy link
Contributor

No description provided.

@tamasvajk tamasvajk added the C# label Aug 20, 2020
@tamasvajk tamasvajk requested a review from a team August 20, 2020 14:46
@tamasvajk tamasvajk force-pushed the feature/nullability-extraction-cil branch from 679b9bb to 9cdee63 Compare August 20, 2020 14:47
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work, Tamas. I have mostly some minor comments, and we should also run a CSharp-Differences job as this touches a lot of code.

@@ -208,7 +208,7 @@ public override IEnumerable<IExtractionProduct> Contents

PopulateParameters(typeSignature.ParameterTypes);

foreach (var c in Parameters)
foreach (var c in Parameters!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just produce no results if Parameters is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameters is never null here, because it's accessed after PopulateParameters() so we can safely use the !. But adding ?? Array.Empty<Parameter>() also works for me if that's what you meant?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use the EnumerateNull extension method from Semmle.Util.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed this to use EnumerateNull, but we're not winning anything because a couple of lines below we're indexing into the Parameters list, and I would need to add the ! there to not get a warning.
So in the end, I'm not going to change this line, I'll leave it as foreach (var c in Parameters!).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm going to change PopulateParameters to return the parameters, and make the assignment to Parameters in the method, and then the compiler will know that it's assigned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -465,7 +467,7 @@ public override IEnumerable<IExtractionProduct> Contents
var typeSignature = mr.DecodeMethodSignature(cx.TypeSignatureDecoder, this);

PopulateParameters(typeSignature.ParameterTypes);
foreach (var p in Parameters) yield return p;
foreach (var p in Parameters!) yield return p;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, just skip the loop if Parameters is null.

@@ -548,7 +551,7 @@ public override IEnumerable<IExtractionProduct> Contents
}

PopulateParameters(constructedTypeSignature.ParameterTypes);
foreach (var p in Parameters)
foreach (var p in Parameters!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

@hvitved hvitved merged commit 6eca97b into github:main Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants