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

Remove scope variance exceptions #76296

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Dec 6, 2024

Fixes #76100.
Alternative to #76261.
Speclet update: dotnet/csharplang#8777

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 6, 2024
@jjonescz jjonescz force-pushed the 76100-ScopeVariance-NoExceptions branch from 493ac3a to f0d3f46 Compare December 6, 2024 09:50
@jjonescz jjonescz marked this pull request as ready for review December 6, 2024 11:51
@jjonescz jjonescz requested a review from a team as a code owner December 6, 2024 11:51
@jjonescz jjonescz requested review from jaredpar and cston December 9, 2024 10:51
@@ -118,3 +118,22 @@ class C
}
```

## Variance of `scoped` and `[UnscopedRef]` is more strict
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I found these breaks when building the BCL with this change: dotnet/runtime@fae33d2e25a

Copy link
Member Author

Choose a reason for hiding this comment

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

@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Dec 10, 2024
@jjonescz jjonescz requested a review from jaredpar December 10, 2024 13:39
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

I much prefer the consistency here, thanks!

// We have removed exceptions to the scoped mismatch error reporting, but to avoid breaks
// we report the new scenarios (previously exempted) as warnings in C# 12 and earlier.
// https://github.com/dotnet/roslyn/issues/76100
(overrideMethod.DeclaringCompilation.LanguageVersion > LanguageVersion.CSharp12 || usedToBeReported(baseMethod));
Copy link
Member Author

Choose a reason for hiding this comment

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

Downgraded the new errors to warnings in C# 12 and lower to lessen the break per offline discussion with Jared. (We think breaking in C# 13 is fine since it's new.)

@jjonescz jjonescz requested a review from 333fred December 12, 2024 21:07
@jjonescz jjonescz enabled auto-merge (squash) December 13, 2024 09:51
@jjonescz jjonescz disabled auto-merge December 13, 2024 11:37
@jjonescz
Copy link
Member Author

@cston @dotnet/roslyn-compiler for a second review, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnscopedRef can be different in interface implementation, leading to ref safety holes
3 participants