Skip to content

Fix 'Use Index' and 'Use Range' analysis for projects with multiple definitions#43205

Merged
sharwell merged 5 commits intodotnet:masterfrom
sharwell:slice-analysis
Apr 17, 2020
Merged

Fix 'Use Index' and 'Use Range' analysis for projects with multiple definitions#43205
sharwell merged 5 commits intodotnet:masterfrom
sharwell:slice-analysis

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Apr 8, 2020

No description provided.

@sharwell sharwell requested a review from a team as a code owner April 8, 2020 22:13
@sharwell sharwell marked this pull request as draft April 8, 2020 22:26
@sharwell sharwell marked this pull request as ready for review April 8, 2020 22:31
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Could you mark any interesting tests that needed to actually change behavior?

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Apr 9, 2020

Could you mark any interesting tests that needed to actually change behavior?

The only significant one which wasn't part of the final commit was changed after I filed #43202.

@sharwell sharwell merged commit 8a39dd2 into dotnet:master Apr 17, 2020
@ghost ghost added this to the Next milestone Apr 17, 2020
@sharwell sharwell deleted the slice-analysis branch April 17, 2020 07:05
@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
// Otherwise, try to get the unique accessible type with this name from a reference
if (type is null)
{
foreach (var module in compilation.Assembly.Modules)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

foreach (var module in compilation.Assembly.Modules) [](start = 16, length = 52)

@sharwell I believe traversing all modules here is an overkill. Added modules cannot successfully resolve an assembly reference unless the assembly is given to the compilation as a reference. In which case, it will be among referenced assemblies of the SourceModule. More over, it looks like traversing all modules is likely to cause this API to fail due to a false ambiguty. For example, two modules reference the same assembly defining the target type, the type will be found twice. It looks like that will be treated as a conflict.

if (currentType is null)
continue;

switch (currentType.GetResultantVisibility())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

switch (currentType.GetResultantVisibility()) [](start = 24, length = 45)

@sharwell Consider if using Compilation.IsSymbolAccessibleWithin for "visibility" check is more appropriate alternative.

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.

4 participants