Skip to content

Update ShouldImplementProperty() to flatten interface hierarchy #8382

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.

Sign up for GitHub

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

Jump to bottom
Merged
daxian-dbw merged 7 commits into from
Feb 19, 2019
Merged

Update ShouldImplementProperty() to flatten interface hierarchy #8382

daxian-dbw merged 7 commits into from
Feb 19, 2019
+32 −4

Conversation

@IISResetMe
Copy link
Collaborator

@IISResetMe IISResetMe commented Dec 2, 2018 •
edited by iSazonov
Loading

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
Update ShouldImplementProperty() to flatten interface hierarchy
2bab38e 
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.
@IISResetMe
Add test for inherited interfaces
87191f2
@iSazonov iSazonov requested a review from lzybkr December 3, 2018 04:15
@iSazonov
Copy link
Collaborator

iSazonov 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
Copy link
Collaborator Author

IISResetMe 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
Copy link
Collaborator

vexx32 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 reviewed changes
daxian-dbw
daxian-dbw reviewed Dec 5, 2018
View reviewed changes
@adityapatwardhan adityapatwardhan changed the title [Feature] Update ShouldImplementProperty() to flatten interface hierarchy Update ShouldImplementProperty() to flatten interface hierarchy Dec 6, 2018
@stale
Copy link

stale bot 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 Stale label Jan 6, 2019
@iSazonov
Copy link
Collaborator

iSazonov commented Jan 6, 2019

@IISResetMe @daxian-dbw Could you please continue?

@stale stale bot removed the Stale label Jan 6, 2019
@IISResetMe
Copy link
Collaborator Author

IISResetMe 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
Copy link
Collaborator

iSazonov commented Jan 13, 2019 •
edited
Loading

@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
Copy link
Collaborator Author

IISResetMe 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
Copy link

stale bot 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 Stale label Feb 12, 2019
@IISResetMe
Copy link
Collaborator Author

IISResetMe commented Feb 15, 2019

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

@stale stale bot removed the Stale label Feb 15, 2019
daxian-dbw
daxian-dbw approved these changes Feb 19, 2019
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@daxian-dbw
Copy link
Member

daxian-dbw 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
@iSazonov iSazonov added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@BrucePay BrucePay Awaiting requested review from BrucePay

@lzybkr lzybkr Awaiting requested review from lzybkr

Assignees

@daxian-dbw daxian-dbw

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@IISResetMe @iSazonov @vexx32 @daxian-dbw @lzybkr