Fix FAR counting for target-typed new#48434
Conversation
| void CollectMatchingReferences(ISymbol originalUnreducedSymbolDefinition, SyntaxNode node, | ||
| ISyntaxFactsService syntaxFacts, ISemanticFactsService semanticFacts, ArrayBuilder<FinderLocation> locations) | ||
| { | ||
| if (syntaxFacts.IsImplicitObjectCreation(node)) |
There was a problem hiding this comment.
@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
left a comment
There was a problem hiding this comment.
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:
If you agree, I'll fix that.
Youssef1313
left a comment
There was a problem hiding this comment.
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.
|
Blocked on #48401 that we are taking for 16.8. Keeping this PR open since the test is still helpful. |
|
@sharwell Do I need to retarget the PR to 16.8 branch? |
|
@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. |
@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:
I tried to restart Visual Studio a couple of times, but every time I just get a different weird error like:
|
|
Already has PR to fix in: #48402 :) |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Thanks for the PR, however this is something Sam and I already have fixes for. #48402
Sorry for any time wasted.
|
@CyrusNajmabadi No problem at all! Closing as duplicate. |
|
The bug is already fixed, but I re-opened to add the test to prevent regressions while refactoring in the future. |
|
@CyrusNajmabadi This is a test only change. Can you please review? |
This has been reduced to a test-only change
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.