EnC - Tell the debugger about updated type def tokens #53217
Conversation
src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
Outdated
Show resolved
Hide resolved
Do we have tests that verify updated types for synthesized types? Local function/lambda's closures, state machines, the Refers to: src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs:29 in d37f7cc. [](commit_id = d37f7cce7f3dbe1038c8f9a8991cc916dc008722, deletion_comment = False) |
|
Let's target |
davidwengier
force-pushed
the
EnCUpdatedTypeDefs
branch
from
d37f7cc
to
f6337d4
Compare
The debugger API that this needs isn't in main yet, which is why I targeted 16.10-vs-deps. I'm happy to not take this for 16.1x, but at the moment we still need to target main-vs-deps for the debugger contracts library. |
davidwengier
force-pushed
the
EnCUpdatedTypeDefs
branch
from
f6337d4
to
050c8da
Compare
|
The changes in the PR do not depend on the debugger API. Passing it to the debugger can only be done in -vs-deps, but all other changes can be in main. |
_typeDefs is used to keep track of added type defs, but if we use it for updated type defs too then we start getting duplicate entries in the EnC log, but at the time that we output that log we can't tell which is the better entries to output. Easier to leave the EnC log alone, since it works, and just add something new to keep track of every update.
davidwengier
force-pushed
the
EnCUpdatedTypeDefs
branch
from
050c8da
to
333cdbe
Compare
|
Okay, I've retargeted to main.
I've added verification to existing tests that cover those scenarios. Couldn't get |
src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs
Show resolved
Hide resolved
|
@dotnet/roslyn-compiler can I get a couple of reviews? |
| EmitResult.UpdatedMethods.Select(methodHandle => $"0x{MetadataTokens.GetToken(methodHandle):X8}")); | ||
| } | ||
|
|
||
| public void VerifyUpdatedTypes(params string[] expecedTypeTokens) |
There was a problem hiding this comment.
Thank you! Will fix, and merge
| @@ -34,6 +34,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Emit | |||
| Dim serializationProperties = compilation.ConstructModuleSerializationProperties(emitOpts, runtimeMDVersion, baseline.ModuleVersionId) | |||
| Dim manifestResources = SpecializedCollections.EmptyEnumerable(Of ResourceDescription)() | |||
|
|
|||
| Dim updatedMethods = ArrayBuilder(Of MethodDefinitionHandle).GetInstance() | |||
There was a problem hiding this comment.
I was confused by this as these don't look used (aside from ToImmutableAndFree()). Is there more work coming later?
There was a problem hiding this comment.
They're passed to SerializeToDeltaStreams on line 86. On the off chance this comment was meant for another location, there is another spot that probably has these arrays and there are changes coming in future, but the API is only in -vs-deps.
There was a problem hiding this comment.
I think I failed to use one of the following correctly:
- My eyes
- My code review tool
😄
ghost
added this to the 
…vice-featureslayer * upstream/main: (857 commits) Update contrib documentation (dotnet#53504) SImplify Fix out of bound crash in lsp navto. Revert changes to TypeScriptWaitContext wrappers Switch to ROSLYN_TEST_CI for CI detection SImplify Simplify LoggerTestChannel using BlockingCollection Only require telemetry validation in CI Fix out of bound crash in lsp navto. Fix locked comment Update Compiler Test Plan.md (dotnet#53420) Adjust doc comment for NullableWalker.VisitConversion (dotnet#53429) Revert "Infer delegate types with -langversion:preview only (dotnet#53241)" (dotnet#53466) Fix syntax normalizer to add space around before colon in constructor initializer (dotnet#53326) Remove unnecessary property (dotnet#53406) EnC - Tell the debugger about updated type def tokens (dotnet#53217) Revert an error Update PublishData.json Keep trailing trivia so single line if statements don't get badly formatted (dotnet#53414) Fix dead test code (dotnet#53416) ...
Fixes #52585