Skip to content

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

Merged
Ilya (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
Ilya (iSazonov) merged 8 commits into
PowerShell:masterfrom
IISResetMe:patch/virtual-interface-properties

Conversation

@IISResetMe

@IISResetMe Mathias R. Jessen (IISResetMe) commented Nov 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

Ilya (iSazonov) commented Nov 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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.

Mathias R. Jessen (@IISResetMe) Please add tests and fix CodeFactor issues.

@IISResetMe

Mathias R. Jessen (IISResetMe) commented Nov 19, 2018

Copy link
Copy Markdown
Collaborator Author

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

@iSazonov

Ilya (iSazonov) commented Nov 19, 2018

Copy link
Copy Markdown
Collaborator

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

@IISResetMe
Mathias R. Jessen (IISResetMe) force-pushed the patch/virtual-interface-properties branch from 94deaef to 4c6b908 Compare November 21, 2018 13:04
@IISResetMe

Mathias R. Jessen (IISResetMe) commented Nov 21, 2018

Copy link
Copy Markdown
Collaborator Author

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

@iSazonov

Ilya (iSazonov) commented Nov 21, 2018

Copy link
Copy Markdown
Collaborator

We ignore "Complex Method".

iSazonov
Ilya (iSazonov) reviewed Nov 21, 2018
View reviewed changes
Comment thread test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 Outdated
iSazonov
Ilya (iSazonov) reviewed Nov 21, 2018
View reviewed changes
Comment thread test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 Outdated
iSazonov
Ilya (iSazonov) reviewed Nov 21, 2018
View reviewed changes
Comment thread src/System.Management.Automation/engine/parser/PSType.cs Outdated
@IISResetMe
Mathias R. Jessen (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
Mathias R. Jessen (IISResetMe) force-pushed the patch/virtual-interface-properties branch from f1b54df to 699f56c Compare November 21, 2018 15:15
@iSazonov

Ilya (iSazonov) commented Nov 22, 2018

Copy link
Copy Markdown
Collaborator

Reopen to restart Appveyor CI.,

@iSazonov Ilya (iSazonov) self-assigned this Nov 30, 2018
@iSazonov Ilya (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
Ilya (iSazonov) merged commit 950377f into PowerShell:master Nov 30, 2018
@iSazonov

Ilya (iSazonov) commented Nov 30, 2018

Copy link
Copy Markdown
Collaborator

Mathias R. Jessen (@IISResetMe) Thanks for your contribution!

@IISResetMe Mathias R. Jessen (IISResetMe) mentioned this pull request Dec 2, 2018
Merged
11 tasks
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
@IISResetMe @iSazonov
Conditionally mark getter/setter implementations virtual in generated…
41e3bfc 
… classes (PowerShell#8303)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw daxian-dbw is a code owner
+1 more reviewer
@lzybkr Jason Shirk (lzybkr) lzybkr approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

See More

@iSazonov Ilya (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