Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 5, 2020

This PR is based on @calumgrant 's #3350 (only the last five commits are new), and takes the approach one step further by totally removing assembly prefixes. Removing assembly prefixes means we have to perform extra disambiguation when constructing labels, to avoid DB inconsistencies. However, I strongly believe that this extra logic is worthwhile the effort, as identical identities belonging to different assemblies will no longer result in two entities.

Even though only the last five commits are new, it probably makes more sense to review this PR in its entirety.

TRAP ID changes

  • Return types added for fields and properties; this was needed for the case when the same type implemented the same field/property in two different assemblies with different return types.
  • Return types added for methods; same reason as above.
  • Parameter names added; sometimes the same callable in two different assemblies would use different paramater names. To avoid DB inconsistencies such callables are still considered different.
  • Types such as <>f__AnonymousType0 are not characterized as anonymous types by Roslyn, but we still need to handle them as such for disambiguation.
  • Base classes are added to avoid inconsistencies.

Other extractor changes

  • Base classes are only extracted when not object; object is added back in QL when needed.
  • Effective base classes for type parameters were not used, so are no longer extracted.
  • The method BuildTypeId has been simplified as it no longer takes an Action<Context, TextWriter, ITypeSymbol> subTermAction argument; instead we always call the newly introduced BuildOrWriteId in the recursive cases, which makes sure to avoid cyclic labels.
  • Entity caching has also been simplified, so we now only have two methods for creating entities: CreateEntity and CreateEntityFromSymbol. The former requires an explicit cache key while the latter caches based on the ISymbol used to initialize the entity.
  • It is still possible to run the extractor with assembly-sensitive TRAP by prefixing --assemblysensitivetrap onto the extractor invocation, but this is now opt-in.

Control flow graph changes

Previously, methods with multiple implementations had "the most likely" implementation chosen, based on counting CFG nodes. So for instance if

// A.cs
class C1
{
    public int M() => 0;
}

and

// B.cs
class C1
{
    public int M() => throw null;
}

were both compiled to the same assembly, then the implementation from A.cs was chosen as the body of C1.M. Now, however, even if A.cs and B.cs are compiled to different assemblies, there will still be only one C1.M method in the database. Since this appears to be much more common, I decided to completely abandon the "most likely implementation" heuristics, and instead treat both 0 and throw null as possible bodies of C1.M. In some sense this also fits better with how we deal with virtual dispatch where all viable dispatch targets are visited; here dispatch happens at linking-time, though. This means that the CFG for C1.M now looks like:

Screenshot 2020-08-28 at 10 02 42

Performance

CSharp-Differences job here here.

@hvitved hvitved force-pushed the csharp/unqualify-trap-ids3 branch from 76bad19 to 873061e Compare August 6, 2020 10:55
@hvitved hvitved changed the title C#: Improve db consistency by removing assembly id C#: Remove assembly prefixes from TRAP labels Aug 10, 2020
@hvitved hvitved force-pushed the csharp/unqualify-trap-ids3 branch 8 times, most recently from f553d41 to 9fe12f5 Compare August 13, 2020 11:22
@hvitved hvitved added the C# label Aug 13, 2020
@adityasharad adityasharad changed the base branch from master to main August 14, 2020 18:33
@hvitved hvitved force-pushed the csharp/unqualify-trap-ids3 branch 9 times, most recently from 2531682 to 7b50815 Compare August 21, 2020 11:03
@hvitved hvitved force-pushed the csharp/unqualify-trap-ids3 branch 7 times, most recently from c199d4e to 7b9d295 Compare August 27, 2020 13:01
@hvitved hvitved marked this pull request as ready for review August 28, 2020 08:04
calumgrant and others added 21 commits September 2, 2020 10:52
…rwise, an entity with the wrong (cached) nullability could be created.
…e` needed to be the same as `objectEntityCache` to fix nullability warnings.
…ble implementations

Previosly, we returned only the body belonging to "the most likely" implementation,
based on a CFG size heuristics. However, now that more callables are mapped to the
same entity, it makes more sense to treat such callables (to some extent) like
partial methods. This means, for instance, that data flow will branch out to all possible
implementations, much like we do for virtual dispatch.
@hvitved hvitved force-pushed the csharp/unqualify-trap-ids3 branch from 51243f1 to 701e189 Compare September 2, 2020 08:52
@hvitved
Copy link
Contributor Author

hvitved commented Sep 2, 2020

Rebased to resolve merge conflict in the change notes.

@hvitved
Copy link
Contributor Author

hvitved commented Sep 2, 2020

Here is the new CSharp-Differences job: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/408/. I will update the PR description to link to this as soon as it completes.

@hvitved
Copy link
Contributor Author

hvitved commented Sep 3, 2020

I will update the PR description to link to this as soon as it completes.

Done.

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.

I'm happy with this. Looks good!

@hvitved
Copy link
Contributor Author

hvitved commented Sep 4, 2020

OK, lets get it in 🤞

@hvitved hvitved merged commit 7f18c33 into github:main Sep 4, 2020
@hvitved hvitved deleted the csharp/unqualify-trap-ids3 branch September 4, 2020 07:20
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