★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/8382Nouvelle 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

Update ShouldImplementProperty() to flatten interface hierarchy #8382

Merged
merged 7 commits into from Feb 19, 2019
+32 −4

Conversation

Reviewers

@iSazonov iSazonov

@daxian-dbw daxian-dbw

@lzybkr lzybkr

@BrucePay BrucePay

Assignees

@daxian-dbw daxian-dbw

Labels
CL-Engine
Projects
None yet
Milestone
No milestone
5 participants
@IISResetMe @iSazonov @vexx32 @daxian-dbw @lzybkr
@IISResetMe
Copy link
Contributor

commented Dec 2, 2018 •
edited by iSazonov

PR Summary

TypeBuilder.GetInterfaces(), as introduced in #8303 returns only the interfaces that was explicitly passed to its constructor, so the current implementation doesn't work for inherited interfaces:

  Add-Type 'public interface ITestBase { string Base {get;} }
            public interface ITest : ITestBase { string Test {get;} }'
  class MyClass : ITest
  {
    [string]$Test
    [string]$Base
  }

In this example, ShouldImplementProperty() won't resolve ITestBase.Base and type definition fails because get_Base() isn't virtual.
During compilation the interface hierarchy is flattened, so we only need to resolve one level of ancestral interfaces.

PR Checklist

IISResetMe added some commits Dec 2, 2018

@IISResetMe
Update ShouldImplementProperty() to flatten interface hierarchy 
TypeBuilder.GetInterfaces() returns only the interfaces that was explicitly passed to its constructor, so the current implementation doesn't work for inherited interfaces:

  Add-Type 'public interface ITestBase { string Base {get;} }
            public interface ITest : ITestBase { string Test {get;} }'
  class : ITest
  {
    [string]$Test
    [string]$Base
  }

In this example, ShouldImplementProperty() won't resolve ITestBase.Base and type definition fails.
During compilation the interface hierarchy is flattened, so we only need to resolve one level of ancestral interfaces.
2bab38e
@IISResetMe
Add test for inherited interfaces
Loading status checks…
87191f2

@IISResetMe IISResetMe requested review from BrucePay and daxian-dbw as code owners Dec 2, 2018

@iSazonov iSazonov requested a review from lzybkr Dec 3, 2018

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Dec 3, 2018

@IISResetMe Please add a commit with [Feature] in title to run feature tests.

@IISResetMe IISResetMe changed the title Update ShouldImplementProperty() to flatten interface hierarchy [Feature] Update ShouldImplementProperty() to flatten interface hierarchy Dec 3, 2018

@IISResetMe

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 3, 2018

@iSazonov I think we'll need to push an empty commit (or close-reopen pr) to trigger the tests again
@vexx32

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 3, 2018

@IISResetMe push an empty commit that contains the tag [Feature] in the commit message itself, yep! 😄
@iSazonov

iSazonov reviewed Dec 4, 2018
View changes

Show resolved Hide resolved src/System.Management.Automation/engine/parser/PSType.cs Outdated
@daxian-dbw

daxian-dbw reviewed Dec 5, 2018
View changes

Show resolved Hide resolved src/System.Management.Automation/engine/parser/PSType.cs Outdated
Show resolved Hide resolved src/System.Management.Automation/engine/parser/PSType.cs Outdated
Show resolved Hide resolved test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 Outdated

IISResetMe added some commits Dec 5, 2018

@IISResetMe
Separate interface resolution from property enumeration in ShouldImpl… 
…ementProperty

+ use Tuple.Create() instead of explicit constructor
283eb55
@IISResetMe
Remove redundant instantiation of test class
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e003052

@adityapatwardhan adityapatwardhan changed the title [Feature] Update ShouldImplementProperty() to flatten interface hierarchy Update ShouldImplementProperty() to flatten interface hierarchy Dec 6, 2018

IISResetMe added some commits Dec 6, 2018

@IISResetMe
Merge branch 'patch/virtual-interface-properties-2' of https://github… 
….com/IISResetMe/PowerShell into patch/virtual-interface-properties-2
Loading status checks…
4a353ef
@IISResetMe
Fix CodeFactor issue (SA1513)
Loading status checks…
130beca
@stale

This comment has been minimized.

Sign in to view
Copy link

commented Jan 6, 2019

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Review - Abandoned label Jan 6, 2019

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Jan 6, 2019

@IISResetMe @daxian-dbw Could you please continue?

@stale stale bot removed the Review - Abandoned label Jan 6, 2019

@IISResetMe

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jan 12, 2019

@iSazonov I believe I've addressed all of @daxian-dbw's remarks, the only thing left was to run the feature tests.

Am I expected to remove the Invoke-Expression statement from the test? 😕

@iSazonov iSazonov assigned daxian-dbw Jan 13, 2019

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Jan 13, 2019 •
edited

@IISResetMe I think using Invoke-Expression haven't a security risk. The Codacy report is for custom scripts not tests. We have to align the settings.

@daxian-dbw Seems the PR is ready to merge. Please update your code review.
@IISResetMe

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jan 13, 2019

[...] The Codacy report is for custom scripts not tests. We have to align the settings.

Yeah, looks like we might want to include it only if .ps1 files that don't match *.tests?.ps1$ are changed
@stale

This comment has been minimized.

Sign in to view
Copy link

commented Feb 12, 2019

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Review - Abandoned label Feb 12, 2019

@IISResetMe

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Feb 15, 2019

@daxian-dbw had a chance to update this? :)

@stale stale bot removed the Review - Abandoned label Feb 15, 2019

@daxian-dbw

daxian-dbw approved these changes Feb 19, 2019
View changes

Copy link
Member

left a comment

Looks great. Sorry for being tardy in responding (was on a trip for the past 3 weeks).
@daxian-dbw

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 19, 2019

As for the Codacy report about Invoke-Expression, the Invoke-Expression command is needed as the interfaces are defined at runtime.

@daxian-dbw daxian-dbw merged commit 0a57021 into PowerShell:master Feb 19, 2019
6 of 7 checks passed

6 of 7 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
CodeFactor No issues found.
Details
PowerShell-CI-linux #PR-8382-20181206.02 succeeded
Details
PowerShell-CI-macos #PR-8382-20181206.02 succeeded
Details
PowerShell-CI-spelling #PR-8382-20181206.02 succeeded
Details
PowerShell-CI-windows #PR-8382-20181206.02 succeeded
Details
license/cla All CLA requirements met.
Details

@iSazonov iSazonov added the CL-Engine label Feb 20, 2019

RDIL pushed a commit to RDIL/PowerShell that referenced this pull request Mar 13, 2019

@IISResetMe --replace-all
Flatten interface hierarchy when generating properties that implement… 
… interface properties (PowerShell#8382)

`TypeBuilder.GetInterfaces()` returns only the interfaces that was explicitly passed to its constructor, so we need to flatten the interface hierarchy in order to properly support inherited interfaces.
a709eeb
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.