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

EnC - Runtime capabilities support #52566

Merged
merged 33 commits into from
Apr 26, 2021

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Apr 12, 2021

Fixes #49010

This just has a dummy extension method implementation, will be removed in a vs-deps branch which has a real implementation from the debugger.

@davidwengier davidwengier marked this pull request as ready for review April 14, 2021 03:59
@davidwengier davidwengier requested a review from a team as a code owner April 14, 2021 03:59
@davidwengier davidwengier requested a review from tmat April 14, 2021 03:59
Comment on lines 17 to 18
Baseline = 1 << 0,
AddDefinitionToExistingType = 1 << 1,
Copy link
Member

Choose a reason for hiding this comment

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

Unclear on what "Baseline" here means.

AddDefinitionToExistingType may be too broad for Mono. I think the following will present different challenges to Mono:

  • adding (non-virtual) static and instance methods
  • adding static fields
  • adding instance fields

Copy link
Member

Choose a reason for hiding this comment

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

Baseline means capabilities that Mono 6, .NET FX and .NET 5 have in common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've expanded out that into these constituent parts, hopefully that at least puts you in control @lambdageek

Let me know what you think.

@davidwengier
Copy link
Contributor Author

Thanks for the feedback! A bunch of security work came up today and I ran out of time, will address it on Monday.

"NewTypeDefinition" => ManagedEditAndContinueCapability.NewTypeDefinition,

// To make it eaiser for runtimes to specify more broad capabilities
"AddDefinitionToExistingType" => ManagedEditAndContinueCapability.AddMethodToExistingType | ManagedEditAndContinueCapability.AddStaticFieldToExistingType | ManagedEditAndContinueCapability.AddInstanceFieldToExistingType,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is being too clever, but also I imagine it would be a pain to try to get Mono and .NET runtimes to keep pace with the same breakdown of capabilities.

This could go further too, for example AddFieldToExistingType = AddStaticFieldToExistingType | AddInstanceFieldToExistingType, or breaking up AddMethodToExistingType. Not sure what the sweet spot is now, or if we just revisit later?

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) April 26, 2021 20:21
@davidwengier davidwengier merged commit dcf1fa0 into dotnet:main Apr 26, 2021
@ghost ghost added this to the Next milestone Apr 26, 2021
@davidwengier davidwengier deleted the EnCRuntimeCapabilities branch April 26, 2021 22:05
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
/// <summary>
/// Creating a new type definition.
/// </summary>
NewTypeDefinition = 1 << 2
Copy link
Member

Choose a reason for hiding this comment

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

@davidwengier oops! this is the same bit value as AddStaticFieldToExistingType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops is what I said too :)

The fix missed the 4pm snap unfortunately, but has been merged to 16.10 already: #52935

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.

EnC: Runtime-specific Rude Edits
4 participants