★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/8303Nouvelle recherche | Portail wanayoo
Skip to content

/PowerShell

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

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

Merged
merged 8 commits into from Nov 30, 2018

Conversation

Reviewers

@lzybkr lzybkr

@iSazonov iSazonov

@daxian-dbw daxian-dbw

@BrucePay BrucePay

Assignees

@iSazonov iSazonov

Labels
CL-General
Projects
None yet
Milestone
No milestone
3 participants
@IISResetMe @iSazonov @lzybkr
@IISResetMe
Copy link
Contributor

commented Nov 17, 2018 •
edited

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 
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.
Loading status checks…
0d9837a

@IISResetMe IISResetMe requested review from BrucePay and daxian-dbw as code owners Nov 17, 2018

@iSazonov iSazonov requested a review from lzybkr Nov 18, 2018

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 18, 2018 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

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 IISResetMe:patch/virtual-interface-properties branch to 4c6b908 Nov 21, 2018

@IISResetMe

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 21, 2018

We ignore "Complex Method".
@iSazonov

iSazonov reviewed Nov 21, 2018
View changes

Show resolved Hide resolved test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 Outdated
@iSazonov

iSazonov reviewed Nov 21, 2018
View changes

Show resolved Hide resolved test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 Outdated
@iSazonov

iSazonov reviewed Nov 21, 2018
View changes

Show resolved Hide resolved src/System.Management.Automation/engine/parser/PSType.cs Outdated
@iSazonov

iSazonov approved these changes Nov 21, 2018
View changes

@IISResetMe IISResetMe force-pushed the IISResetMe:patch/virtual-interface-properties branch Nov 21, 2018

@IISResetMe
Emit test class during interface inheritance test 
Build tests currently failing because the interface property test is missing
an instance of the test class to inspect
Loading status checks…
699f56c

@IISResetMe IISResetMe force-pushed the IISResetMe:patch/virtual-interface-properties branch to 699f56c Nov 21, 2018

@lzybkr

lzybkr approved these changes Nov 21, 2018
View changes

@iSazonov iSazonov closed this Nov 22, 2018

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

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 label Nov 30, 2018

@iSazonov iSazonov merged commit 950377f into PowerShell:master Nov 30, 2018
6 of 7 checks passed

6 of 7 checks passed

CodeFactor 1 issue found.
Details
PowerShell-CI-linux #PR-8303-20181122.01 succeeded
Details
PowerShell-CI-macos #PR-8303-20181122.01 succeeded
Details
PowerShell-CI-spelling #PR-8303-20181122.01 succeeded
Details
PowerShell-CI-windows #PR-8303-20181122.01 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 30, 2018

@IISResetMe Thanks for your contribution!

@IISResetMe IISResetMe referenced this pull request Dec 2, 2018

Merged

Update ShouldImplementProperty() to flatten interface hierarchy #8382

7 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.