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

Document ref enumerator disposal compat break #33367

Merged
merged 4 commits into from
Feb 19, 2019

Conversation

chsienki
Copy link
Member

Documents the breaking change of calling Dispose on a ref struct enumerator.

@chsienki chsienki added Area-Compilers Feature - enhanced using Using pattern and declaration labels Feb 13, 2019
@chsienki chsienki requested a review from jcouv February 13, 2019 22:56
@chsienki
Copy link
Member Author

@dotnet/roslyn-compiler for review please

@@ -81,3 +81,26 @@ Each entry should include a short description of the break, followed by either a

10. Previously, reference assemblies were emitted including embedded resources. In Visual Studio 2019, embedded resources are no longer emitted into ref assemblies.
See https://github.com/dotnet/roslyn/issues/31197

11. Ref structs now support disposal via pattern. A ref struct enumerator with an accessible `void Dispose()` method will now have it invoked at the end of enumeration, where it would not have been before:
Copy link
Member

Choose a reason for hiding this comment

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

method [](start = 110, length = 6)

"... instance method or static extension method"

Copy link
Member Author

Choose a reason for hiding this comment

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

LDM disallowed extension methods. (I really need to publish the updated spec)


In reply to: 256627330 [](ancestors = 256627330)

@@ -81,3 +81,26 @@ Each entry should include a short description of the break, followed by either a

10. Previously, reference assemblies were emitted including embedded resources. In Visual Studio 2019, embedded resources are no longer emitted into ref assemblies.
See https://github.com/dotnet/roslyn/issues/31197

11. Ref structs now support disposal via pattern. A ref struct enumerator with an accessible `void Dispose()` method will now have it invoked at the end of enumeration, where it would not have been before:
Copy link
Member

Choose a reason for hiding this comment

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

where it would not have been before [](start = 168, length = 36)

" ... regardless of whether the struct type implements IDisposable"

@jcouv
Copy link
Member

jcouv commented Feb 13, 2019

This should probably target the dev16.0 branch. Documentation changes don't need QB approval.

@jcouv jcouv added this to the 16.0.P4 milestone Feb 13, 2019
@jaredpar
Copy link
Member

Agree lets put this into dev16.0

@jcouv jcouv changed the base branch from master to dev16.0 February 14, 2019 23:32
@jcouv jcouv changed the base branch from dev16.0 to master February 14, 2019 23:33
@jcouv
Copy link
Member

jcouv commented Feb 14, 2019

@chsienki I tried re-targeting the PR to dev16.0, but that had too many commits. You'll have to do it with git.

@chsienki chsienki force-pushed the dispose_ref_enumerator_compat_break branch from 1e2510d to 7b57192 Compare February 15, 2019 18:32
@chsienki chsienki requested a review from a team as a code owner February 15, 2019 18:32
@chsienki chsienki changed the base branch from master to dev16.0 February 15, 2019 18:32
@chsienki
Copy link
Member Author

Rebased on dev16.0 and re-targeted.

foreach(var x in new C())
{
}
// RefEnumerator.Dispose() will be called here in C# 8.0
Copy link
Member

@jcouv jcouv Feb 15, 2019

Choose a reason for hiding this comment

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

in C# 8.0 [](start = 55, length = 9)

Just to confirm: Is this the case?
Is the LangVersion the determining factor, or the fact that you use a 3.0 (dev16) version of the compiler? If we don't already have a test for that, it would be good to add.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it should require only C# 8.0. Let me test, and add a test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we do correctly gate on langver

// Don't try and lookup if we're not enabled
if (MessageID.IDS_FeatureUsingDeclarations.RequiredVersion() > Compilation.LanguageVersion)
{
return null;
}

@jcouv jcouv self-assigned this Feb 15, 2019
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

- Check that we don't call Dispose on a ref struct enumerator in language versions lower than 8.0
- Assert.Null
- Add an execution test to check end to end
@chsienki chsienki merged commit fb6b898 into dotnet:dev16.0 Feb 19, 2019
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.

4 participants