[EnC] Support more symbol deletes #62151
Conversation
davidwengier
force-pushed
the
EnCMoreDeletes
branch
from
a23a806 to
167091e
Compare
|
@tmat PTAL. Ignore the commit list, no idea why GitHub didn't compress it after the other branch merged |
This comment was marked as outdated.
This comment was marked as outdated.
davidwengier
force-pushed
the
EnCMoreDeletes
branch
from
2ef5582 to
293e17b
Compare
src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs
Show resolved
Hide resolved
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/EditAndContinue/TopLevelEditingTests.cs
Outdated
Show resolved
Hide resolved
| @@ -2655,7 +2655,7 @@ public ConstructorEdit(INamedTypeSymbol oldType) | |||
| // Associated member declarations must be in the same document as the symbol, so we don't need to resolve their symbol. | |||
| // In some cases the symbol even can't be resolved unambiguously. Consider e.g. resolving a method with its parameter deleted - | |||
| // we wouldn't know which overload to resolve to. | |||
| if (TryGetAssociatedMemberDeclaration(oldDeclaration, out var oldAssociatedMemberDeclaration)) | |||
| if (TryGetAssociatedMemberDeclaration(oldDeclaration, EditKind.Delete, out var oldAssociatedMemberDeclaration)) | |||
There was a problem hiding this comment.
I'm not sure I understand why this is needed. When a property is deleted we want to create semantic edits for all of its accessors, but not for the property itself. If an accessor is deleted we want a single semantic edit just for that accessor. Correct?
If the entire property is deleted wouldn't we want to skip creating deletes for its accessors represented by the deletes of their syntax nodes, since we will add deletes for all of the accessor for the deleted property node?
There was a problem hiding this comment.
I think this was just for ease of control flow, so that a delete of a single accessor didn't end up in passing this if, failing the inner one, and then not being considered as a method symbol delete. If I take your other suggestion about extracting the below to a method, I should be able to call that new method here and revert this change.
There was a problem hiding this comment.
Though, that would mean this branch would look identical to the next branch, but I can't combine them without effectively bringing this change back 🤷♂️
There was a problem hiding this comment.
Yeah, don't think I can do anything meaningful about this... The problem is type parameters, which would fail the newContainingType == null check below. So to remove this, means adding a "is type parameter" check around that, which doesn't seem like it actually saves anything.
There was a problem hiding this comment.
@tmat how strongly do you feel about this? The other feedback has been addressed.
ghost
added this to the 
Part of #59264 and follow up to #61806
This allows deleting of properties, indexers, operators and constructors, since they're all just methods with fancy hats.