Skip to content

C#: Introduce inherited CFG completions#1486

Merged
semmle-qlci merged 5 commits intogithub:masterfrom
hvitved:csharp/inherited-completions
Jul 4, 2019
Merged

C#: Introduce inherited CFG completions#1486
semmle-qlci merged 5 commits intogithub:masterfrom
hvitved:csharp/inherited-completions

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Jun 21, 2019

When completions are inherited by elements inside finally blocks, we previously
threw away the underlying completion. For example, in

try
{
    if (b)
        throw new Exception();
}
finally
{
    if (b)
        ...
}

the completions for b inside the finally block are true and throw(Exception),
where the latter is inherited from the try block, with an underlying false
completion. Throwing away the false completion meant that we were unable to prune
the false edge (Boolean CFG splitting).

Profiling report here (internal link).

@hvitved hvitved force-pushed the csharp/inherited-completions branch 3 times, most recently from 15f7b6b to 292e5dc Compare June 26, 2019 09:10
hvitved added 2 commits June 28, 2019 15:41
When completions are inherited by elements inside `finally` blocks, we previously
threw away the underlying completion. For example, in

```
try
{
    if (b)
        throw new Exception();
}
finally
{
    if (b)
        ...
}
```

the completions for `b` inside the `finally` block are `true` and `throw(Exception)`,
where the latter is inherited from the `try` block, with an underlying `false`
completion. Throwing away the `false` completion meant that we were unable to prune
the `false` edge (Boolean CFG splitting).
@hvitved hvitved force-pushed the csharp/inherited-completions branch from 292e5dc to f91e460 Compare June 28, 2019 13:42
@hvitved hvitved requested a review from calumgrant July 1, 2019 07:11
@hvitved hvitved added the C# label Jul 1, 2019
@hvitved hvitved marked this pull request as ready for review July 1, 2019 07:11
@hvitved hvitved requested a review from a team as a code owner July 1, 2019 07:11
Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

As discussed offline, we agreed that NestedCompletion, Nested* , Nestable* would be slightly better, and also getInnerCompletion() and getOuterCompletion().

I wasn't sure about the need for the split the completion classes into -Inherited and -Direct (perhaps renamed to Nested- and NonNested-). I think it would be more maintainable to avoid the Nested- variants (and assume everything is NonNested-), and remember to use getInnerCompletion() where needed.

@hvitved hvitved force-pushed the csharp/inherited-completions branch from 09a2ec1 to 421e75d Compare July 4, 2019 09:58
Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

LGTM and happy to merge. A few very minor comments that could be fixed later/never if you prefer.

@@ -668,165 +658,142 @@ abstract private class InheritableCompletion extends AbnormalCompletion { }
* `System.Console.WriteLine("M called")` inherits the throw completion
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the word "inherits" and "underlying" and replace it with "has the outer" and "has the inner"

@semmle-qlci semmle-qlci merged commit 0290c79 into github:master Jul 4, 2019
@hvitved hvitved deleted the csharp/inherited-completions branch July 4, 2019 18:45
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.

3 participants