Skip to content

Conditionally mark getter/setter implementations virtual in generated classes#8303

Merged
iSazonov merged 8 commits into
PowerShell:masterfrom
IISResetMe:patch/virtual-interface-properties
Nov 30, 2018
Merged

Conditionally mark getter/setter implementations virtual in generated classes#8303
iSazonov merged 8 commits into
PowerShell:masterfrom
IISResetMe:patch/virtual-interface-properties

Conversation

@IISResetMe
Copy link
Copy Markdown
Collaborator

@IISResetMe IISResetMe commented Nov 17, 2018 •
edited
Loading

Fix #8302

When implementing interfaces, PowerShell incorrectly produces non-virtual get/set methods for interface-defined properties.
This commit adds a lookup method for interface-defined properties and marks get/set methods for properties with matching signatures virtual.

PR Summary

PR Checklist

I'm unsure how to meaningfully add tests to prevent regression for this

@IISResetMe
Conditionally mark getter/setter implementations virtual
0d9837a 
When implementing interfaces, PowerShell incorrectly produces non-virtual get/set methods for interface-defined properties.
This commit adds a lookup method for interface-defined properties and marks get/set methods for properties with matching signatures virtual.
@iSazonov iSazonov requested a review from lzybkr November 18, 2018 13:15
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 18, 2018 •
edited
Loading

I have a two-year-old commit that supports getters/setters (MSFT team had a plan to enhance class support before 6.1 release but it was postponed and I'm already tired to rebase the commit every month :-) ) and I could add it there since this code has been significantly modified.

@IISResetMe Please add tests and fix CodeFactor issues.

@IISResetMe
Copy link
Copy Markdown
Collaborator Author

IISResetMe commented Nov 19, 2018

@iSazonov if you have anything that's either more correct, complete or aesthetically pleasing, please feel free to update the PR :)

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 19, 2018

@IISResetMe My post was informational. If you commit will approved I can grab it to my branch or rebase my branch.

@IISResetMe IISResetMe force-pushed the patch/virtual-interface-properties branch from 94deaef to 4c6b908 Compare November 21, 2018 13:04
@IISResetMe
Copy link
Copy Markdown
Collaborator Author

IISResetMe commented Nov 21, 2018

@iSazonov Fixed the CodeFactor issues in ShouldImplementProperty() but I'm unsure what to do with the "Complex Method" finding for EmitPropertyIL()

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 21, 2018

We ignore "Complex Method".

iSazonov
iSazonov reviewed Nov 21, 2018
View reviewed changes
Comment thread test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 Outdated
iSazonov
iSazonov reviewed Nov 21, 2018
View reviewed changes
Comment thread test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 Outdated
iSazonov
iSazonov reviewed Nov 21, 2018
View reviewed changes
Comment thread src/System.Management.Automation/engine/parser/PSType.cs Outdated
@IISResetMe IISResetMe force-pushed the patch/virtual-interface-properties branch from c544123 to f1b54df Compare November 21, 2018 14:33
@IISResetMe
Emit test class during interface inheritance test
699f56c 
Build tests currently failing because the interface property test is missing
an instance of the test class to inspect
@IISResetMe IISResetMe force-pushed the patch/virtual-interface-properties branch from f1b54df to 699f56c Compare November 21, 2018 15:15
@iSazonov iSazonov closed this Nov 22, 2018
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 22, 2018

Reopen to restart Appveyor CI.,

@iSazonov iSazonov reopened this Nov 22, 2018
@iSazonov iSazonov self-assigned this Nov 30, 2018
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 30, 2018
@iSazonov iSazonov merged commit 950377f into PowerShell:master Nov 30, 2018
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Nov 30, 2018

@IISResetMe Thanks for your contribution!

@IISResetMe IISResetMe mentioned this pull request Dec 2, 2018
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

+1 more reviewer

@lzybkr lzybkr lzybkr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@iSazonov iSazonov

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Generated get/set methods incorrectly marked non-virtual

3 participants

@IISResetMe @iSazonov @lzybkr