Update ShouldImplementProperty() to flatten interface hierarchy#8382
Conversation
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 Please add a commit with |
|
@iSazonov I think we'll need to push an empty commit (or close-reopen pr) to trigger the tests again |
|
@IISResetMe push an empty commit that contains the tag |
test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1
Outdated
Show resolved
Hide resolved
…ementProperty + use Tuple.Create() instead of explicit constructor
….com/IISResetMe/PowerShell into patch/virtual-interface-properties-2
|
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. |
|
@IISResetMe @daxian-dbw Could you please continue? |
|
@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 |
|
@IISResetMe I think using @daxian-dbw Seems the PR is ready to merge. Please update your code review. |
Yeah, looks like we might want to include it only if |
|
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. |
|
@daxian-dbw had a chance to update this? :) |
daxian-dbw
left a comment
There was a problem hiding this comment.
Looks great. Sorry for being tardy in responding (was on a trip for the past 3 weeks).
|
As for the Codacy report about |
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:
In this example, ShouldImplementProperty() won't resolve ITestBase.Base and type definition fails because
get_Base()isn'tvirtual.During compilation the interface hierarchy is flattened, so we only need to resolve one level of ancestral interfaces.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests