Skip to content

Conversation

@calumgrant
Copy link
Contributor

@calumgrant calumgrant commented Feb 11, 2020

This addresses an issue that occurs particularly with .NET Core, where classes can be defined in more than one assembly. Particularly, the assembly Private.CoreLib duplicates much of the standard library, from System.Runtime and so on. Relaxing the qualifiers on IDs in some places removes many of the dangling references in the database.

@calumgrant calumgrant force-pushed the cs/unqualify-trap-ids branch 2 times, most recently from a2d682b to e3ac473 Compare February 13, 2020 19:32
@@ -1,3 +1,9 @@
| Program.cs:18:21:18:22 | c1 | Assembly1.dll:0:0:0:0 | Class |
| Program.cs:18:21:18:22 | c1 | Assembly2.dll:0:0:0:0 | Class |
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'm not sure whether this is a desired change in behaviour. The reason for the change is that the local variable has a type given by a typeref which is no longer qualified with the assembly id. Therefore, the type of the local variable maps to all matching types, which is three different types in this case.

To recover the original behaviour, the extractor simply needs to populate a type as opposed to a typeref in the local variable's type field.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think types of local variables should be treated specially, what do you think?

@calumgrant calumgrant marked this pull request as ready for review February 17, 2020 09:56
@calumgrant calumgrant requested a review from a team as a code owner February 17, 2020 09:56
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.

A few minor things, otherwise LGTM. Could you elaborate a bit on the type of DB consistency errors that are removed by this change?

@calumgrant calumgrant force-pushed the cs/unqualify-trap-ids branch from e3ac473 to 4c7d413 Compare April 7, 2020 19:03
…rwise, an entity with the wrong (cached) nullability could be created.
@lgtm-com
Copy link

lgtm-com bot commented Apr 20, 2020

This pull request introduces 1 alert when merging ead9167 into 243dea7 - view on LGTM.com

new alerts:

  • 1 for Missed 'readonly' opportunity

}

readonly IDictionary<object, ICachedEntity> objectEntityCache = new Dictionary<object, ICachedEntity>();
readonly IDictionary<object, ICachedEntity> symbolEntityCache = new Dictionary<object, ICachedEntity>(10000, new SymbolComparer());
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 be able to change object to ISymbol here and in SymbolComparer, right?

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 originally had some difficulty converting an IDictionary<ISymbol, ICachedEntity> to IDictionary<object, ICachedEntity> but managed to work around it in the end. The result is better, so thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants