Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix FAR counting for target-typed new #48434

Merged
merged 2 commits into from
Nov 8, 2020
Merged

Fix FAR counting for target-typed new #48434

merged 2 commits into from
Nov 8, 2020

Conversation

Youssef1313
Copy link
Member

Fixes #47987

Counting implicit object creations was depending on whether the whole document contains 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.

@Youssef1313 Youssef1313 requested a review from a team as a code owner October 8, 2020 13:07
Youssef1313
Youssef1313 commented Oct 8, 2020
View reviewed changes
src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs Outdated
@@ -637,6 +637,11 @@ static bool IsRelevantDocument(SyntaxTreeIndex syntaxTreeInfo)
void CollectMatchingReferences(ISymbol originalUnreducedSymbolDefinition, SyntaxNode node,
ISyntaxFactsService syntaxFacts, ISemanticFactsService semanticFacts, ArrayBuilder<FinderLocation> locations)
{
if (syntaxFacts.IsImplicitObjectCreation(node))
Copy link
Member Author

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 use IsImplicitObjectCreation instance method, or IsImplicitObjectCreationExpression, and why we need the two?

Youssef1313
Youssef1313 commented Oct 8, 2020
View reviewed changes
Copy link
Member Author

@Youssef1313 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 method FindReferencesInImplicitObjectCreationExpressionAsync is currently in AbstractReferenceFinder.cs. I think it makes more sense to move it as a private method to ConstructorSymbolReferenceFinder.cs.

I can't see a reason why to make it in AbstractReferenceFinder while its friend is in ConstructorSymbolReferenceFinder:

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

Line 107 in bc85a40

private static ValueTask<ImmutableArray<FinderLocation>> FindOrdinaryReferencesAsync(

If you agree, I'll fix that.

Youssef1313
Youssef1313 commented Oct 8, 2020
View reviewed changes
Copy link
Member Author

@Youssef1313 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 a Task, while FindOrdinaryReferencesAsync returns a ValueTask.

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

@sharwell
Copy link
Member

sharwell commented Oct 8, 2020 •
edited
Loading

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

@Youssef1313
Copy link
Member Author

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

@sharwell
Copy link
Member

sharwell commented Oct 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 you want to retarget this to release/dev16.8 after my change merges, that would likely be acceptable as a test-only change.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Oct 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:

image

I tried to restart Visual Studio a couple of times, but every time I just get a different weird error like:

image

@CyrusNajmabadi
Copy link
Member

Already has PR to fix in: #48402 :)

CyrusNajmabadi
CyrusNajmabadi previously requested changes Oct 8, 2020
View reviewed changes
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, however this is something Sam and I already have fixes for. #48402

Sorry for any time wasted.

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi No problem at all! Closing as duplicate.

@Youssef1313 Youssef1313 closed this Oct 8, 2020
@Youssef1313 Youssef1313 deleted the target-typed-new-ctor-counting branch October 8, 2020 15:48
@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Oct 8, 2020
@jinujoseph jinujoseph added this to InQueue in IDE: CommunityPR via automation Oct 8, 2020
@jinujoseph jinujoseph moved this from InQueue to Completed in IDE: CommunityPR Oct 8, 2020
@Youssef1313 Youssef1313 restored the target-typed-new-ctor-counting branch October 10, 2020 15:45
@Youssef1313 Youssef1313 reopened this Oct 10, 2020
@Youssef1313
Copy link
Member Author

The bug is already fixed, but I re-opened to add the test to prevent regressions while refactoring in the future.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Oct 10, 2020 •
edited
Loading

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

@sharwell This is no longer blocked as your PR is now merged.

@jinujoseph jinujoseph moved this from Completed to BuddyAssigned in IDE: CommunityPR Oct 12, 2020
@jcouv jcouv added the New Language Feature - Target-Typed New `new (args)` gets the created type inferred from context label Oct 15, 2020
@Youssef1313
Copy link
Member Author

@CyrusNajmabadi This is a test only change. Can you please review?

@sharwell sharwell dismissed CyrusNajmabadi’s stale review November 8, 2020 18:46

This has been reduced to a test-only change

IDE: CommunityPR automation moved this from BuddyAssigned to LGTM Nov 8, 2020
@sharwell sharwell merged commit 11e3b9e into dotnet:master Nov 8, 2020
IDE: CommunityPR automation moved this from LGTM to Completed Nov 8, 2020
@ghost ghost added this to the Next milestone Nov 8, 2020
@Youssef1313 Youssef1313 deleted the target-typed-new-ctor-counting branch November 8, 2020 19:19
@jinujoseph jinujoseph moved this from Completed-Sprint179 to Completed in IDE: CommunityPR Nov 9, 2020
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
@mikadumont mikadumont mentioned this pull request Dec 10, 2020
Closed
@jinujoseph jinujoseph moved this from Completed-180 to Completed 2020 in IDE: CommunityPR Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharwell sharwell sharwell approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

Assignees

@CyrusNajmabadi CyrusNajmabadi

@jcouv jcouv

Labels
Area-IDE Blocked Community The pull request was submitted by a contributor who is not a Microsoft employee. New Language Feature - Target-Typed New `new (args)` gets the created type inferred from context
Projects
IDE: CommunityPR
  
Completed 2020
Milestone
16.9.P2
Development

Successfully merging this pull request may close these issues.

Target typed new - bug with constructor reference counting
6 participants
@Youssef1313 @sharwell @CyrusNajmabadi @jinujoseph @jcouv @allisonchou