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

Need an analyzer to warn about ignored Dispose method #32400

Open
jcouv opened this issue Jan 11, 2019 · 5 comments
Open

Need an analyzer to warn about ignored Dispose method #32400

jcouv opened this issue Jan 11, 2019 · 5 comments
Labels
Area-Compilers Bug Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. New Language Feature - enhanced using Using pattern and declaration
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Jan 11, 2019

In C# 8.0, we're adding support for pattern-based disposal. Due to compatibility concerns, the feature is restricted to ref structs for foreach and using. But it not restricted for await foreach and await using.

This unfortunate situation could cause user confusion. It would be useful for an analyzer to warn on foreach or using where the user might hope an instance Dispose() method to be called but it is in fact not called.

Umbrella for enhanced-using feature: #28588

@jcouv jcouv added Area-IDE New Language Feature - enhanced using Using pattern and declaration labels Jan 11, 2019
@jinujoseph jinujoseph added this to the 16.0 milestone Jan 15, 2019
@jinujoseph jinujoseph assigned jcouv and unassigned mavasani Jan 15, 2019
@jinujoseph jinujoseph modified the milestones: 16.0, 16.0.P3 Jan 15, 2019
@sharwell
Copy link
Member

@jcouv Why doesn't the compiler report a clear warning for this case (extension Dispose not available for use in foreach/using)?

@sharwell sharwell added Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. and removed Area-IDE Feature Request labels Jan 15, 2019
@jcouv
Copy link
Member Author

jcouv commented Jan 15, 2019

@sharwell We don't want to break existing code. You could have an existing Dispose method which was (and remains) ignored. Previously, no scenarios allowed such a Dispose method to contribute to a foreach, but now it could (in some limited scenarios).

To illustrate the purpose of this analyzer, consider someone who changes their ref struct enumerable/enumerator (with a Dispose method) to be a struct (Dispose will be ignored).
But the compiler cannot break someone who is already in the latter situation...

@sharwell
Copy link
Member

@jcouv My understanding is existing code will not be broken. Code will fall into three cases:

  1. The type supports pattern-based dispose, in which case this issue does not apply
  2. The type implements IDisposable, in which case pattern-based dispose methods are irrelevant because the IDisposable implementation would take precedence
  3. The type does not implement IDisposable in which case the code would not compile anyway so the error you report is at the discretion of the compiler (and could change)

This issue targets situation 3, reporting an error that a pattern-based dispose was not used instead of the current error saying the type must be disposable.

@jcouv
Copy link
Member Author

jcouv commented Jan 15, 2019

A foreach works on a pattern-based enumerable/enumerator without IDisposable today, so situation 3 actually compiles. The lowered code for foreach does if (enumerator is IDisposable) ....
The analyzer would produce a warning in that case (since the compiler cannot, for compat reasons).

@jcouv jcouv modified the milestones: 16.0.P3, 16.1.P1 Jan 23, 2019
@gafter gafter modified the milestones: 16.1.P1, 16.1.P3 Apr 9, 2019
@jcouv jcouv modified the milestones: 16.1.P3, 16.1, 16.2 Apr 18, 2019
@jcouv
Copy link
Member Author

jcouv commented May 6, 2019

@sharwell Did my reponse make sense? I still think this should be addressed by an analyzer warning rather than a compiler warning.

@jcouv jcouv modified the milestones: 16.2, 16.3 Jun 19, 2019
@jcouv jcouv removed their assignment Jun 19, 2019
@jcouv jcouv removed this from the 16.3 milestone Jul 17, 2019
@jcouv jcouv added this to the Compiler.Next milestone Jul 17, 2019
@gafter gafter added the Bug label Sep 17, 2019
@jaredpar jaredpar modified the milestones: Compiler.Next, Backlog Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. New Language Feature - enhanced using Using pattern and declaration
Projects
None yet
Development

No branches or pull requests

6 participants