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.

Sign up for GitHub

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

Jump to bottom

Runtime capability to allow attribute editing #52986

Merged
merged 64 commits into from
Jun 8, 2021
Merged

Runtime capability to allow attribute editing #52986

merged 64 commits into from
Jun 8, 2021

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Apr 28, 2021 •
edited
Loading

Fixes #49012

Adds a new runtime capability to control attribute edits. If the capability is not present, rude edits are reported. If the capability is present, semantic edits are reported.

This still needs more work done to only allow changing attributes that are stored in the CustomAttributes table.

@davidwengier davidwengier changed the base branch from release/dev16.10 to release/dev16.10-vs-deps April 28, 2021 05:24
@davidwengier davidwengier marked this pull request as ready for review May 4, 2021 00:30
@davidwengier davidwengier requested a review from a team as a code owner May 4, 2021 00:30
@tmat
Copy link
Member

tmat commented May 8, 2021 •
edited
Loading

Let's target main. I don't think we need this in 16.1x. If we need to we can then back port. In general we shouldn't make changes to -vs-deps unless absolutely necessary since it just makes harder to work in main.

tmat
tmat requested changes May 8, 2021
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

🕐

@davidwengier
Copy link
Contributor Author

@tmat Added the deny list for non-custom attributes, PTAL.

@davidwengier davidwengier removed request for a team June 5, 2021 12:56
@davidwengier
Copy link
Contributor Author

Merged and resolved conflicts. That was a pain :P
Still a couple of test failures I need to investigate in active statements, presumably something clashing between this PR and the line mapping PR just merged.

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs Outdated
{
continue;
}

#if TODO_ACCESSORS
Copy link
Member

Choose a reason for hiding this comment

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

TODO_ACCESSORS

Did I forget to delete this in my PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Did you mean to delete it? The linked issue is still unresolved so I left it just in case.

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs Outdated

foreach (var parameter in newSymbol.GetParameters())
{
var oldParameter = oldSymbol?.GetParameters().SingleOrDefault(p => p.Name.Equals(parameter.Name));
Copy link
Member

Choose a reason for hiding this comment

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

SingleOrDefault

Is it possible there are parameters with duplicate names in error scenarios? We should not crash; just ignore such case.

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs Outdated
if (newSymbol is INamedTypeSymbol or IFieldSymbol or IPropertySymbol)
{
var symbolKey = SymbolKey.Create(newSymbol, cancellationToken);
semanticEdits.Add(new SemanticEditInfo(SemanticEditKind.Update, symbolKey, syntaxMap, null, null));
Copy link
Member

Choose a reason for hiding this comment

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

null

nit: name null args

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs Outdated

public int GetHashCode(KeyValuePair<string, TypedConstant> obj)
=> obj.GetHashCode();
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: let's move this code above the test accessor, e.g. to the Helpers region

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs Outdated

#region Comparers

private class TypedConstantComparer : IEqualityComparer<TypedConstant>
Copy link
Member

Choose a reason for hiding this comment

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

class

nit: sealed

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs Outdated
=> obj.GetHashCode();
}

private class NamedArgumentComparer : IEqualityComparer<KeyValuePair<string, TypedConstant>>
Copy link
Member

Choose a reason for hiding this comment

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

private

nit: sealed

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/EditAndContinueCapabilities.cs Outdated
NewTypeDefinition = 1 << 4,

/// <summary>
/// Adding, updating and deleting of custom attributes (as distinct from attributes that effect metadata)
Copy link
Member

Choose a reason for hiding this comment

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

attributes that effect metadata

pseudo-custom attributes

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/EditAndContinueCapabilities.cs Outdated
/// <summary>
/// Adding, updating and deleting of custom attributes (as distinct from attributes that effect metadata)
/// </summary>
UpdateCustomAttributes = 1 << 5,
Copy link
Member

Choose a reason for hiding this comment

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

UpdateCustomAttributes

Perhaps ChangeCustomAttributes would be better since update might mean just updating existing values (this is how we usually use the word in EnC).

tmat
tmat reviewed Jun 7, 2021
View reviewed changes
src/Features/CSharp/Portable/EditAndContinue/CSharpEditAndContinueAnalyzer.cs
case SyntaxKind.Attribute:
case SyntaxKind.AttributeList:
ReportError(RudeEditKind.Insert);
// To allow inserting of attributes we need to check if the inserted attribute
// is a pseudo-custom attribute that CLR allows us to change, or if it is a compiler well-know attribute
Copy link
Member

Choose a reason for hiding this comment

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

allows

"does not allow"?

tmat
tmat approved these changes Jun 7, 2021
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@davidwengier davidwengier enabled auto-merge (squash) June 7, 2021 23:27
@davidwengier davidwengier merged commit 88bb074 into dotnet:main Jun 8, 2021
@ghost ghost added this to the Next milestone Jun 8, 2021
@davidwengier davidwengier deleted the EnCAttributeEdits branch June 8, 2021 00:40
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmat tmat tmat approved these changes

Assignees
No one assigned
Labels
Area-Interactive
Projects
None yet
Milestone
17.0.P2
Development

Successfully merging this pull request may close these issues.

EnC: Allow adding/removing/updating attributes
4 participants
@davidwengier @tmat @RikkiGibson @jcouv