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

IDE0052 fires on write-only properties #30894

Closed
davkean opened this issue Nov 1, 2018 · 6 comments
Closed

IDE0052 fires on write-only properties #30894

davkean opened this issue Nov 1, 2018 · 6 comments
Assignees
Milestone

Comments

@davkean
Copy link
Member

davkean commented Nov 1, 2018

While write-only properties are questionable, just because they don't have a getter doesn't mean they can be removed.

        ''' <summary>
        ''' Sets/Gets the Encoding used for text file resources, without undo support, and without causing the 
        '''  designer to be dirtied (because ComponentChangeService notifications won't get sent)
        ''' </summary>
        ''' <value></value>
        ''' <remarks></remarks>
        Private WriteOnly Property EncodingWithoutUndo() As Encoding
            Set(Value As Encoding)
                Debug.Assert(IsLink AndAlso ResourceTypeEditor.Equals(ResourceTypeEditors.TextFile))
                If _resXDataNode.FileRef IsNot Nothing Then
                    _resXDataNode = NewResXDataNode(Name, Comment, AbsoluteLinkPathAndFileName, ValueTypeName, Value)
                End If
            End Set
        End Property

Later:

EncodingWithoutUndo = GuessFileEncoding(AbsoluteLinkPathAndFileName)

Expected: No warning
Actual:

Severity	Code	Description	Project	Project Rank	File	Line	Category	Suppression State	Tool
Message	IDE0052	Private member 'Resource.EncodingWithoutUndo' can be removed as the value assigned to it is never read.	Microsoft.VisualStudio.Editors	3	E:\project-system2\src\Microsoft.VisualStudio.Editors\ResourceEditor\Resource.vb	958	Code Quality	Active	Compiler
@davkean davkean changed the title IDE00052 fires on write-only properties IDE0052 fires on write-only properties Nov 1, 2018
@davkean davkean transferred this issue from dotnet/project-system Nov 1, 2018
@sharwell sharwell added this to the 16.0 milestone Nov 1, 2018
@mavasani
Copy link
Contributor

mavasani commented Nov 1, 2018

This is by design. For majority of realistic code examples, a write-only property is very likely dead code and this violation will help them clean up their code. For questionable examples such as the one mentioned here, the write-only property should be converted to a method OR the violation should be suppressed if it is legacy code.

@mavasani mavasani closed this as completed Nov 1, 2018
@mavasani mavasani added the Resolution-By Design The behavior reported in the issue matches the current design label Nov 1, 2018
@davkean
Copy link
Member Author

davkean commented Nov 2, 2018

For majority of realistic code examples, a write-only property is very likely dead code

Can you show me exactly of what you claim as realistic write-only properties that are dead code? Every single write-only property in this code is noise.

@mavasani
Copy link
Contributor

mavasani commented Nov 2, 2018

Can you show me exactly of what you claim as realistic write-only properties that are dead code?

I don't think there is any modern code base that uses private write-only properties, except if they sneaked in by accident when code was getting refactored. Can you show me any other open source repo, except the legacy VB projects in project-system that uses one?

Regardless, given these are extremely rare, and even if they exist are a questionable API design choice, I would be fine with us detecting and bailing out on these. I am re-opening this issue and will confirm it in the next design meeting.

@mavasani mavasani reopened this Nov 2, 2018
@mavasani mavasani added Need Design Review The end user experience design needs to be reviewed and approved. and removed Resolution-By Design The behavior reported in the issue matches the current design labels Nov 2, 2018
@CyrusNajmabadi
Copy link
Member

@mavasani possibly worth having a sub-option for this? so people can say "actually, in my code we do use this pattern", but others can say "yeah... i would consider that strange, let me get rid of it"

Also, we do have a convert-property-to-method refactoring. But i'm not actually sure it works on write-only properties. :D

@mavasani
Copy link
Contributor

mavasani commented Nov 6, 2018

@CyrusNajmabadi Given these are so rare, for now I am going to submit a PR for this analyzer to bail out on write-only properties to remove the noise that @davkean is seeing.

We should likely have an option OR report a different diagnostic for write-only properties, suggesting replacing them with methods. I will file a separate issue and bring that up for design review next week.

@mavasani mavasani removed the Need Design Review The end user experience design needs to be reviewed and approved. label Nov 6, 2018
@mavasani
Copy link
Contributor

mavasani commented Nov 6, 2018

We should likely have an option OR report a different diagnostic for write-only properties, suggesting replacing them with methods. I will file a separate issue and bring that up for design review next week.

Filed #30983

@mavasani mavasani modified the milestones: 16.0, 16.0.P2 Nov 6, 2018
@mavasani mavasani added the 4 - In Review A fix for the issue is submitted for review. label Nov 6, 2018
@ghost ghost closed this as completed in f9f6143 Nov 8, 2018
@gafter gafter removed 4 - In Review A fix for the issue is submitted for review. labels Dec 17, 2018
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants