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: Stop reporting CS7101 on runtime that supports generic updates #68601

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

tmat
Copy link
Member

@tmat tmat commented Jun 13, 2023

Related: #68293

@tmat tmat requested a review from a team as a code owner June 13, 2023 22:48
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Interactive untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 13, 2023
@tmat
Copy link
Member Author

tmat commented Jun 13, 2023

@davidwengier PTAL

Comment on lines +993 to +1000
// The compiler only uses this predicate to determine if CS7101: "Member 'X' added during the current debug session
// can only be accessed from within its declaring assembly 'Lib'" should be reported.
// Prior to .NET 8 Preview 4 the runtime failed to apply such edits.
// This was fixed in Preview 4 along with support for generics. If we see a generic capability we can disable reporting
// this compiler error. Otherwise, we leave the check as is in order to detect at least some runtime failures on .NET Framework.
// Note that the analysis in the compiler detecting the circumstances under which the runtime fails
// to apply the change has both false positives (flagged generic updates that shouldn't be flagged) and negatives
// (didn't flag cases like https://github.com/dotnet/roslyn/issues/68293).
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this sort of thing is why we have capabilities. If this bit of logic is permanent, then I think it makes more sense to add a capability for this scenario specifically, that this code can check, and then in put the logic in the Parse method so that if it sees the GenericAddMethodToExistingType capability, it can also set the new capability. Hopefully the runtime can also report the capability at some point, so the parse method doesn't have any special logic.

But arguably that is just moving deck chairs on the titanic, so up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add a new capability. Not sure how we would call it though. AddPublicMemberAndItsInvocationAtTheSameTime is a bit mouthful. Since the issue was fixed at the same time as GenericAddMethodToExistingType and it doesn't apply to Mono, I'm leaning towards keeping it part of GenericAddMethodToExistingType.

@tmat tmat merged commit 419ee23 into dotnet:main Jun 26, 2023
@tmat tmat deleted the CS7101 branch June 26, 2023 16:02
@ghost ghost added this to the Next milestone Jun 26, 2023
@RikkiGibson RikkiGibson modified the milestones: Next, 17.7 P3 Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interactive untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants