Emit attribute changes for parameters#54835
Conversation
|
Oh, as painful as some of this is, most of it is necessary for parameter renames anyway, so we get bang for our buck! |
2e80170 to
b8a09cb
Compare
|
/azp run |
|
The build errors don't match the file contents ¯\_(ツ)_/¯ |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Let's make PopulateEncTables virtual and PopulateEncLogTableRows and PopulateEncMapTableRows private non-virtual. Then _customAttributeEncMapRows and _paramEncMapRows can be passed between these methods isntead of being fields defined on the writer. Refers to: src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs:782 in c13f258. [](commit_id = c13f258, deletion_comment = False) |
…lways emit parameters when a method is updated
c13f258 to
2dd3ad6
Compare
|
Tomas and I discussed offline and we decided to just have the compiler emit Param rows whenever a method is updated, rather than try to be smart about it. That simplifies this PR a lot, but unfortunately, and I normally hate to do this after a review has started, it did mean I had to rebase and force push to drop the relevant commits. |
|
@cston Care to have another look? Would love to get this in before snap tomorrow. |
| } | ||
| } | ||
|
|
||
| if (methodChange == SymbolChange.Added) |
There was a problem hiding this comment.
I'm assuming the behavior of type parameters is unchanged here, i.e. adding an attribute to a type parameter during an EnC session is not supported.
There was a problem hiding this comment.
Yes, editing generic types is not supported.
|
|
||
| private void EmitParametersFromDelta(IMethodDefinition methodDef, MethodDefinitionHandle handle) | ||
| { | ||
| var ok = _previousGeneration.FirstParamRowMap.TryGetValue(handle, out var firstRowId); |
There was a problem hiding this comment.
It feels like it might be reasonable to fail in retail builds here if the handle isn't present, e.g. by changing the code to var firstRowId = _previousGeneration.FirstParamRowMap[handle]. However, I don't know exactly what the failure mode is here if we proceed into the loop with a firstRowId which is just 0 instead of being something meaningful for the given handle.
There was a problem hiding this comment.
This would still fail later, when trying to emit an EncLog row for Param row 0, and I think that would give a more descriptive exception that "element not found".
Realistically this can only fail if someone changes how EnC works in a way that would break a lot of things, like perhaps allowing direct creation of a delta without a baseline, so a debug assert feels appropriate to me.
|
Thanks all! |
Fixes #54777
This was a pain! In a way I'm glad I didn't do this while doing custom attributes because I probably would have gone crosseyed.