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

Add ChecksumAlgorithm to project snapshot #62840

Merged
merged 32 commits into from
Oct 10, 2022
Merged

Add ChecksumAlgorithm to project snapshot #62840

merged 32 commits into from
Oct 10, 2022

Conversation

tmat
Copy link
Member

@tmat tmat commented Jul 21, 2022

The goal is to allow EnC analyzer to check whether content of a Document matches the checksum stored in the PDB when applying EnC/Hot Reload changes. Currently we read the content from disk since we can't get the correct checksum from Document and such approach runs into issues when the file is updated on disk before we can read its content. This results in EnC/Hot Reload incorrectly blocking or ignoring changes.

Adds ChecksumAlgorithm to Project snapshot (on ProjectAttributes). The value is derived from the corresponding msbuild property set in project file.

Checksum algorithm of a Document is stored on DocumentState and initialized from the TextLoader when the loader is set to the state. This is needed because the state does not directly hold on the loader, but instead it holds on a value source for the SourceText or SyntaxTree. It's not part of DocumentAttributes as we would need to keep the attributes in sync with the value of the loader.

Fixes #51993
Fixes AB#1567043
Implements #64220

Bans use of APIs in Roslyn whose implementations create SourceText but do not have a checksum algorithm parameter and always use the default (SHA1). In Roslyn product code we should be explicit about what checksum algorithm is used.

Allow usage of APIs listed in eng\config\BannedSymbols.txt in tests without warnings. These APIs would ideally not be used at all but usage in tests doesn't matter much. In fact, we still need to test these APIs even though we don't want them used in product code. Avoiding usage of APIs banned in this PR in all tests is not worth the significant effort it would take to clean up all tests.

This PR keeps new TextLoader API internal. These will become public in the follow up change: #64192

PdbMatchingSourceTextProvider (96e0f77)

EnC needs to ensure for each modified document that it has a baseline snapshot of that document whose content exactly matches the source used for compilation of the baseline binary. We use checksum stored in the PDB for the comparison.

When debugging session starts, we stash away the current Solution snapshot so that we can compare updated documents against it. However, this snapshot doesn't necessarily have all file content loaded at this time. By the time we need to compare the content the file content on disk might have changed. To mitigate this issue, we pre-loaded all open documents at the start of debugging session and then watched the workspace for new documents being opened. When the document was opened, we notified the EnC service to check if it already has the PDB matching content snapshot for this document. If not, it would try to read the current file content and see if it's matching the PDB.

This approach has an inherent race condition. This PR changes implements a different approach. We record document snapshots of interest (candidates for PDB matching content) in reaction to workspace events. Instead of notifying the EnC service at that point we let it to call back to the workspace listener when it encounters a document whose baseline it doesn't have yet.

VS validation 1: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/426348
VS validation 2: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6790099&view=results

@tmat
Copy link
Member Author

tmat commented Jul 27, 2022

@jasonmalinowski @dotnet/roslyn-compiler PTAL

@tmat tmat marked this pull request as draft July 28, 2022 00:50
@tmat
Copy link
Member Author

tmat commented Jul 28, 2022

Found some issues and redoing the representation of checksum alg in Document snapshot.

@tmat tmat marked this pull request as ready for review July 28, 2022 23:20
eng/config/BannedSymbols.txt Outdated Show resolved Hide resolved
src/Compilers/Test/Core/AssemblyLoadTestFixture.cs Outdated Show resolved Hide resolved
src/Compilers/Test/Core/Platform/Desktop/TestHelpers.cs Outdated Show resolved Hide resolved
src/Compilers/Test/Core/TestBase.cs Outdated Show resolved Hide resolved
@333fred 333fred added the Needs API Review Needs to be reviewed by the API review council label Aug 3, 2022
@tmat tmat requested a review from a team as a code owner October 7, 2022 20:50
@tmat tmat changed the base branch from release/dev17.4 to main October 7, 2022 20:50
@tmat tmat enabled auto-merge (squash) October 7, 2022 20:50
@tmat tmat disabled auto-merge October 8, 2022 03:10
@tmat
Copy link
Member Author

tmat commented Oct 10, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@tmat tmat merged commit 157d34a into dotnet:main Oct 10, 2022
@tmat tmat deleted the Checksum branch October 10, 2022 17:50
@ghost ghost added this to the Next milestone Oct 10, 2022
333fred added a commit to 333fred/roslyn that referenced this pull request Oct 10, 2022
* upstream/main: (252 commits)
  Use the source-built version of ref packs and don't use app host when building in source-build (dotnet#64055)
  Enable rich LSIF hover information. (dotnet#64580)
  Add ChecksumAlgorithm to project snapshot (dotnet#62840)
  Utility for uploading artifact on test failure (dotnet#64578)
  Enable diagnostics
  Revert "Remove unused TS brace completion code"
  Publish additional packages to vssdk feed (dotnet#64571)
  spelling
  Move check
  Simplify SymbolKey implementation
  lint
  Update publish data to test PR validation fix (dotnet#64559)
  Simplify
  Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/SymbolKey/SymbolKey.cs
  Lint
  Proper equality checks
  Update src/Workspaces/Remote/Core/RemoteCallback.cs
  Revert "Not wait for solution crawler because it can be very busy"
  Add and use TargetFramework.Net70 (dotnet#64490)
  Not wait for solution crawler because it can be very busy
  ...
@tmat tmat modified the milestones: Next, 17.5 P1 Oct 19, 2022
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.

MSBuildWorkspace creates documents with wrong checksum algorithm
6 participants