Skip to content

Conversation

tamasvajk
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the C# label Nov 5, 2020
@tamasvajk tamasvajk force-pushed the feature/csharp9-target-typed branch from 98cc972 to 3ef211b Compare November 18, 2020 09:51
@tamasvajk tamasvajk marked this pull request as ready for review November 18, 2020 09:51
@tamasvajk tamasvajk requested a review from a team as a code owner November 18, 2020 09:51
@@ -0,0 +1,2 @@
| TargetType.cs:12:18:14:18 | ... ? ... : ... | int? | int? | null |
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that the extractor does not insert implicit casts in the branches, I think it should. What do you think?

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 don't think we need the implicit casts, because that's not what happens in IL:

int? x = condition ? 5 : null;

becomes the following in IL:

Nullable<int> num = condition ? new Nullable<int>(5) : new Nullable<int>();

So there's no implicit cast. The problem is more like that we're not extracting nullables in the most accurate way. This could probably become part of https://github.com/github/codeql-csharp-team/issues/82.

Does it cause any issue in QL if the two branches of the conditional have different types? If so, then the short term fix is probably what you suggest, to add the implicit cast.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK; nullables are special (I forgot). But how about the other test case below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no conv. or castclass in IL for the latter case either, so why would we add a cast?
On the other hand, I think we add an implicit cast for the extraction of IEnumerable<int> xs = new int[]{};. And if we really do, then I think we should/must also add the cast for ?: too.

What is the motivation behind having the implicit cast in the first place?
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not convinced that the implicit casts are in fact that important. However, in cases where the implicit cast is via an operator, we should add it (both in assignments and in ternary conditionals).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hvitved I pushed an extra commit with some additional tests. Implicit casts and conversion calls do show up in the output.

hvitved
hvitved previously approved these changes Nov 26, 2020
@tamasvajk tamasvajk merged commit c751c51 into github:main Nov 27, 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