Counting implicit object creations was depending on whether the whole documentcontains any implicit object creations or not.
That caused normal object creations to count as implicit when the document contains implicit creations. So, the normal object creations were counted twice.

Fix FAR counting for target-typed new

ad092cb

Youssef1313 requested a review froma team as acode owner

October 8, 2020 13:07

Youssef1313 commented

Oct 8, 2020

View reviewed changes

src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs Outdated

		voidCollectMatchingReferences(ISymboloriginalUnreducedSymbolDefinition,SyntaxNodenode,
		ISyntaxFactsServicesyntaxFacts,ISemanticFactsServicesemanticFacts,ArrayBuilder<FinderLocation>locations)
		{
		if(syntaxFacts.IsImplicitObjectCreation(node))

Copy link

MemberAuthor

Youssef1313Oct 8, 2020

Choose a reason for hiding this comment

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

@jcouv In

https://github.com/dotnet/roslyn/pull/43645/files#diff-aeace626a93e2448b1aec4bc466c9931R336-R337

I see you introduced an extension method called "IsImplicitObjectCreationExpression". So I'm not sure whether I should useIsImplicitObjectCreation instance method, orIsImplicitObjectCreationExpression, and why we need the two?

Youssef1313 commented

Oct 8, 2020

View reviewed changes

Copy link

MemberAuthor

Youssef1313 left a comment

Choose a reason for hiding this comment

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

The methodFindReferencesInImplicitObjectCreationExpressionAsync is currently inAbstractReferenceFinder.cs. I think it makes more sense to move it as a private method toConstructorSymbolReferenceFinder.cs.

I can't see a reason why to make it inAbstractReferenceFinder while its friend is inConstructorSymbolReferenceFinder:

roslyn/src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/ConstructorSymbolReferenceFinder.cs

Line 107 inbc85a40

privatestaticValueTask<ImmutableArray<FinderLocation>>FindOrdinaryReferencesAsync(

If you agree, I'll fix that.

Youssef1313 commented

Oct 8, 2020

View reviewed changes

Copy link

MemberAuthor

Youssef1313 left a comment

Choose a reason for hiding this comment

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

Note 2:

FindReferencesInImplicitObjectCreationExpressionAsync currently returns aTask, whileFindOrdinaryReferencesAsync returns aValueTask.

I'm not sure if there is a performance benefit if both are returning aValueTask. It worth having a look.

Copy link

Contributor

sharwell commentedOct 8, 2020•
edited
Loading

Blocked on#48401 that we are taking for 16.8. Keeping this PR open since the test is still helpful.

sharwell added the Blocked label

Oct 8, 2020

Copy link

MemberAuthor

Youssef1313 commentedOct 8, 2020

@sharwell Do I need to retarget the PR to 16.8 branch?

Copy link

Contributor

sharwell commentedOct 8, 2020•
edited
Loading

@Youssef1313 No we can keep the test in the master branch. I was fixing a performance issue for 16.8 and it just happens to have the same root cause as this. However, if youwant to retarget this to release/dev16.8 after my change merges, that would likely be acceptable as a test-only change.

Copy link

MemberAuthor

Youssef1313 commentedOct 8, 2020•
edited
Loading

No we can keep the test in the master branch.

@sharwell Thanks. I'm keeping it in master.

There does seem to be test failures here related to the FAR feature. Can you please re-run the failed job to see whether it's a consistent failure or just flaky? (I'd be surprised if it's a consistent failure because these tests didn't fail in your PR)

I can't currently debug this because of some weird errors I'm getting in Visual Studio: