Skip to content

[SourceLink] Use correct encoding for embedded text#57683

Merged
davidwengier merged 9 commits intodotnet:mainfrom
davidwengier:EmbeddedTextEncoding
Nov 22, 2021
Merged

[SourceLink] Use correct encoding for embedded text#57683
davidwengier merged 9 commits intodotnet:mainfrom
davidwengier:EmbeddedTextEncoding

Conversation

@davidwengier
Copy link
Member

@davidwengier davidwengier commented Nov 10, 2021

Fixes #57350

There is a minor code move here, so commit-by-commit is probably easiest.

@davidwengier davidwengier requested a review from tmat November 10, 2021 22:19
@davidwengier davidwengier requested a review from a team as a code owner November 10, 2021 22:19
@ghost ghost added the Area-IDE label Nov 10, 2021
ryzngard
ryzngard reviewed Nov 10, 2021
View reviewed changes
@tmat
Copy link
Member

tmat commented Nov 10, 2021 

        var sourceText = SourceText.From(fileStream, Encoding.UTF8, sourceDocument.HashAlgorithm);

pass encoding?

Refers to: src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentLoaderService.cs:92 in dbef806. [](commit_id = dbef806, deletion_comment = False)

@tmat
Copy link
Member

tmat commented Nov 10, 2021 

        var sourceText = SourceText.From(fileStream, Encoding.UTF8, sourceDocument.HashAlgorithm);

Seems like "from disk" and "from embedded" code paths can converge once the Stream of raw data is available and the encoding can be handled in the same way.

In reply to: 965855046

Refers to: src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentLoaderService.cs:92 in dbef806. [](commit_id = dbef806, deletion_comment = False)

tmat
tmat reviewed Nov 11, 2021
View reviewed changes
src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs
Encoding? defaultEncoding = null;
if (pdbCompilationOptions.TryGetValue("default-encoding", out var encodingString))
{
defaultEncoding = Encoding.GetEncoding(encodingString);
Copy link
Member

@tmat tmat Nov 11, 2021

Choose a reason for hiding this comment

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

GetEncoding

May throw ArgumentException

Copy link
Member Author

@davidwengier davidwengier Nov 11, 2021

Choose a reason for hiding this comment

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

If we can't get the encoding, do we bail out? Or try with the default encoding and let the checksum handle if it is wrong?

Copy link
Contributor

@ryzngard ryzngard Nov 19, 2021

Choose a reason for hiding this comment

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

Since we own all the involved bits it should be fine to let an exception be exceptional here. We put the wrong information in the pdb

@runfoapp runfoapp bot mentioned this pull request Nov 11, 2021
Open
jasonmalinowski
jasonmalinowski reviewed Nov 19, 2021
View reviewed changes
@runfoapp runfoapp bot mentioned this pull request Nov 19, 2021
Closed
@davidwengier
Copy link
Member Author

davidwengier commented Nov 19, 2021

Ping @ryzngard @jasonmalinowski anything blocking here?

Now that the debugger work is in I'd love to keep this moving to support source link, but that need vs-deps changes and I don't really know how to do that without conflicts, but I'm pretty sure an open pr won't help :)

@davidwengier davidwengier enabled auto-merge (squash) November 21, 2021 23:45
@davidwengier davidwengier merged commit d39d68c into dotnet:main Nov 22, 2021
@ghost ghost added this to the Next milestone Nov 22, 2021
@davidwengier davidwengier deleted the EmbeddedTextEncoding branch November 22, 2021 01:35
@allisonchou allisonchou removed this from the Next milestone Nov 30, 2021
@allisonchou allisonchou added this to the 17.1.P2 milestone Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat left review comments

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

+1 more reviewer

@ryzngard ryzngard ryzngard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

17.1.P2

Development

Successfully merging this pull request may close these issues.

[SourceLink] Ensure embedded source is shown with the right encoding

5 participants

@davidwengier @tmat @jasonmalinowski @ryzngard @allisonchou