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.

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

Fix crash in FAR helpers when only part of a linked-document set has been updated #66805

Merged
merged 2 commits into from
Feb 13, 2023

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 10, 2023 16:02
// It's possible for us to have a solution snapshot where only part of a linked set of documents has
// been updated. As such, the other linked docs may have different contents/sizes than the original
// doc we started with. Skip those files as there's no sensible way to say that we have linked
// symbols here when the contents are not the same.
Copy link
Member

Choose a reason for hiding this comment

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

❔ Can we take this one step further and check that the SourceText is the same? While the syntax trees might differ, I would expect linked documents to at least share the SourceText internally.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'd prefer to not accidentally cause us to break where someone legally updates the different forks with different source-texts :)

Copy link
Member

Choose a reason for hiding this comment

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

Is there another way to do content validation efficiently? Or perhaps the number of cases of mismatched SourceText is so rare that we can do checksum comparison if the length is the same but the instance is different?

Copy link
Member Author

Choose a reason for hiding this comment

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

generally speaking, i think this check, along with the check below (For name/kind) is sufficient.

@@ -271,7 +272,15 @@ public static IEnumerable<TSymbol> FindSimilarSymbols<TSymbol>(TSymbol symbol, C
foreach (var linkedDocumentId in originalDocument.GetLinkedDocumentIds())
{
var linkedDocument = solution.GetRequiredDocument(linkedDocumentId);

// It's possible for us to have a solution snapshot where only part of a linked set of documents has
Copy link
Member

Choose a reason for hiding this comment

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

b

I'm wondering if we need to handle this in completion as well. Here for example: https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Completion/Providers/AbstractSymbolCompletionProvider.cs,324

Copy link
Member Author

Choose a reason for hiding this comment

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

ANy place that assume the files must have the same content is definitely in error and needs to be fixed up.

Copy link
Member

@genlu genlu left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit c768df7 into dotnet:main Feb 13, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the farCrash branch February 13, 2023 21:49
@ghost ghost added this to the Next milestone Feb 13, 2023
@RikkiGibson RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023
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