Prevent member name collisions in PowerShell classes by IISResetMe · Pull Request #8372 · PowerShell/PowerShell

IISResetMe · 2018-11-30T13:44:42Z

PR Summary

PowerShell currently allows definition of class with overlapping property and method names without warning, like so:

class TestDuplicateMembers
{
  $A
  A(){}
}

Unlike the C# compiler (which throws a compilation error), we allow compilation of the above type definition, and ETS then implicitly hides the property behind the method at runtime - making the property implementation completely useless.

This change fixes that by making duplicate member name checks mutual between DefineMethod()/DefineProperty() - so that if a method with name X is already defined, an attempt to define property X will throw a ParserError (and vice versa) just like we already do for duplicate member names of the same member type

TODO:

~~Regression tests~~

PR Checklist

I'd consider this a bucket 3 breaking change (we'll start failing on defining types that are already broken but partially functional).

Change is obviously user-facing but I don't believe it warrants a documentation update since the previous documentation doesn't address the existing buggy behavior

Fix PowerShell#8235

@lzybkr

As per @lzybkr's advice, duplicate member names should not be a parser concern since it's not a syntax violation. This also avoids LINQ queries in the parser.

IISResetMe · 2018-11-30T15:27:07Z

Marked PR WIP to block merge since linked issue is still awaiting committee review

IISResetMe · 2018-12-07T12:34:43Z

@SteveL-MSFT can you include this (or the linked issue) in the next committee review?

SteveL-MSFT · 2018-12-20T01:53:56Z

@IISResetMe to set expectation, due to the holidays and unavailability of many committee members, this won't be reviewed until after the new year (and probably 2nd week of Jan at the earliest)

SteveL-MSFT · 2019-01-16T23:21:07Z

@PowerShell/powershell-committee reviewed this, although .Net allows this, no language supports it and PowerShell should not either. It is a technical breaking change although unlikely any customer is dependent on this since they would have defined a duplicate name member and not have used it. We agree PowerShell should prevent this and have a parse time error.

Fix CodeFactor issue

IISResetMe · 2019-01-17T11:00:45Z

Thanks @SteveL-MSFT, will add regression tests for this then and we can move ahead! :)

The following class definition should still be valid (as in any other .NET language): ``` class A { $A A(){} } ```

iSazonov · 2019-01-18T13:15:05Z

src/System.Management.Automation/engine/parser/PSType.cs

            private void DefineProperty(PropertyMemberAst propertyMemberAst)
            {
-                if (_definedProperties.ContainsKey(propertyMemberAst.Name))
+                if (_definedProperties.ContainsKey(propertyMemberAst.Name) || _definedMethods.ContainsKey(propertyMemberAst.Name) && !_definedMethods[propertyMemberAst.Name].Any(m => m.Item1.IsConstructor))


Suggested change

if (_definedProperties.ContainsKey(propertyMemberAst.Name) || _definedMethods.ContainsKey(propertyMemberAst.Name) && !_definedMethods[propertyMemberAst.Name].Any(m => m.Item1.IsConstructor))

if (_definedProperties.ContainsKey(propertyMemberAst.Name) || (_definedMethods.ContainsKey(propertyMemberAst.Name) && !_definedMethods[propertyMemberAst.Name].Any(m => m.Item1.IsConstructor)))

Already fixed 😅

Could we avoid LINQ?

We could move it into a separate method (like for the method overload check), but TBH I'm not so worried about the use of LINQ here since that code path is part of the compile-time routine, it's not triggered at parse-time

Actually we can avoid LINQ altogether - IsConstructor is name-bound, so if any m satisfies IsConstructor == true, they all will. We can simply inspect the first item in the list

src/System.Management.Automation/engine/parser/PSType.cs

Co-Authored-By: IISResetMe <[email protected]>

Since IsConstructor is already tied to the method name, we can safely assume that either all or none of them satisfy IsConstructor == true, so only checking the first list item is sufficient

IISResetMe · 2019-01-18T14:24:17Z

@iSazonov I think that was it! Do we need review from @daxian-dbw or @BrucePay still?

iSazonov · 2019-01-18T14:33:15Z

Yes, we are waiting.

src/System.Management.Automation/engine/parser/PSType.cs

IISResetMe · 2019-02-22T22:49:26Z

Can we turn off the "variable name looks like hungarian notation" rule (SA1305) flagged by CodeFactor? It's super noisy, and a cursory glance at S.M.A shows that we have over 900 parameter names alone that look like hungarian notation to StyleCop, but I went through the list of name and only ~50 of them are actually named using hungarian notation:

PS ~> $parameterNames = [psobject].Assembly.GetTypes().GetMethods().GetParameters().Name
PS ~> $looksHungarian = $parameterNames -cmatch '^[a-z]{2,3}[A-Z]+'
PS ~> $actuallyHungarian = $looksHungarian -notmatch '^(?:add|arg|ast|bad|do|eat|for|has|if|is|job|max|min|my|new|no|old|out|pre|ps|raw|run|set|sub|to|try|ui|use|yes)'
PS ~> $looksHungarian.Count
935
PS ~> $actuallyHungarian.Count
46

iSazonov · 2019-02-23T17:41:45Z

@IISResetMe We already have a PR to disable some CodeFactor rules.

src/System.Management.Automation/engine/parser/PSType.cs

IISResetMe · 2019-03-08T15:27:27Z

@iSazonov are we still waiting for the build pipeline to be fixed (CodeFactor issues)? Or do I need to address anything?

iSazonov · 2019-03-08T15:44:58Z

@IISResetMe Feel free to ignore CodeFactor issues if they is not in your changed code. Maintainers will ask you to fix style issues before merge if they deem it necessary.

stale · 2019-04-17T12:33:06Z

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.

IISResetMe · 2019-04-17T12:36:02Z

@daxian-dbw can you review this? :)

daxian-dbw · 2019-04-17T15:59:43Z

@IISResetMe Sure, will do the review today.

daxian-dbw · 2019-04-18T22:32:08Z

src/System.Management.Automation/engine/parser/PSType.cs

+                //       $C
+                //     }
+                //
+                if (_definedProperties.ContainsKey(propertyMemberAst.Name) || (_definedMethods.ContainsKey(propertyMemberAst.Name) && !_definedMethods[propertyMemberAst.Name][0].Item1.IsConstructor))


nit: Can you please define a local variable to hold propertyMemberAst.Name and replace propertyMemberAst.Name with that local variable in this method?
That will make this check less cumbersome and a bit more readable.

daxian-dbw · 2019-04-18T22:51:02Z

@IISResetMe Your change doesn't fix the case where class A derives from class B:

class Bar { $c }
class Foo : Bar { c() {} }
$s = [Foo]::new()
$s.c()

Method invocation failed because [Foo] does not contain a method named 'c'.
At line:1 char:1
+ $s.c()
+ ~~~~~~
+ CategoryInfo          : InvalidOperation: (:) [], RuntimeException
+ FullyQualifiedErrorId : MethodNotFound

But to be honest, I don't know if it's worth the effort to fix that ...

vexx32 · 2019-04-18T23:47:56Z

@IISResetMe are you gonna take that sitting down? 😄

That does look like a very interesting case to try to figure out, though. Kind of looks like it might be a bit of a maze of recursively checking inherited members... Fun times!

daxian-dbw · 2019-04-19T00:34:49Z

@IISResetMe worked on something similar before -- flatten all implemented interfaces.
I know we need to fix the inheritance scenario for the sake of completeness, but personally, I just don't feel it's worth to do it ... :) because it will impose a lot more checks on most of the usages that actually don't need those checks at all.

vexx32 · 2019-04-19T00:49:01Z

Oh, that's right, he did. Should be a walk in the park, then. 😉

But I get where you're coming from too, it might not be worth it... but then again, it might end up being necessary -- after all, one of those members you're accidentally hiding might come from up the inheritance chain quite a ways, and it could potentially be very confusing if you hit that by accident.

Could this happen if you attempted to inherit from two disparate classes? Do PS classes even support that? 🤔

IISResetMe · 2019-04-19T09:06:49Z

Could this happen if you attempted to inherit from two disparate classes? Do PS classes even support that?

No, and I can't think of a language that compiles against .NET that does... Multiple inheritance opens up a whole new can of worms wrt member resolution ambiguity, so maybe let's not try and solve that as part of this PR ^__^

@daxian-dbw we can indeed leverage the same approach as with the interface flattening code for this. The extra lookups would be dependent on the user specifying a base class so it doesn't have to be super expensive for types that just inherit from Object. Will take a stab at it over the weekend

rjmholt · 2019-04-19T16:16:31Z

Yeah, the CLR doesn't support multiple inheritance, even for C++

stale · 2019-05-19T16:57:29Z

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 · 2019-05-29T17:07:03Z

This PR has been automatically closed because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it.
Thanks again for your contribution.
Community members are welcome to grab these works.

microsoft-github-policy-service · 2025-11-12T15:49:51Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

IISResetMe added 2 commits November 24, 2018 21:03

Disallow class definitions with duplicate member names

bfd4be7

Fix PowerShell#8235

Move mutual check for duplicate class member names to PSType

5404f94

As per @lzybkr's advice, duplicate member names should not be a parser concern since it's not a syntax violation. This also avoids LINQ queries in the parser.

IISResetMe requested review from BrucePay and daxian-dbw as code owners November 30, 2018 13:44

iSazonov requested a review from lzybkr November 30, 2018 15:04

iSazonov self-assigned this Nov 30, 2018

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Dec 20, 2018

SteveL-MSFT added Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 16, 2019

Update PSType.cs

7a9e6ec

Fix CodeFactor issue

IISResetMe added 3 commits January 18, 2019 13:54

Exclude ctor definitions from member name collision checks

999bfcb

The following class definition should still be valid (as in any other .NET language): ``` class A { $A A(){} } ```

Add negative parsing tests for member name collisions

bca5cbd

Fix CodeFactor issue (SA1408)

cd680a7

iSazonov reviewed Jan 18, 2019

View reviewed changes

iSazonov and others added 2 commits January 18, 2019 14:17

Exit early on property name collision checks for constructor definitions

4eecf75

Co-Authored-By: IISResetMe <[email protected]>

Avoid unnecessary use of LINQ (.Any(this IEnumerable<T>)) in PSType

1323dc9

Since IsConstructor is already tied to the method name, we can safely assume that either all or none of them satisfy IsConstructor == true, so only checking the first list item is sufficient

iSazonov added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Jan 18, 2019

[noop] re-run failed tests

94cb873

SteveL-MSFT requested a review from rjmholt January 22, 2019 18:26

rjmholt reviewed Jan 22, 2019

View reviewed changes

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

SteveL-MSFT mentioned this pull request Jan 23, 2019

Class : Methods and properties can't have the same name #8235

Closed

Add comment regarding property name collision check logic

fbc76a3

Expand previous commit

ab38d4d

Trigger tests

8bfff0f

rjmholt approved these changes Mar 6, 2019

View reviewed changes

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

stale bot added the Stale label Apr 17, 2019

stale bot removed the Stale label Apr 17, 2019

daxian-dbw reviewed Apr 18, 2019

View reviewed changes

stale bot added the Stale label May 19, 2019

stale bot closed this May 29, 2019

IISResetMe mentioned this pull request Jan 21, 2022

Prevent member naming collision during type creation IISResetMe/PSType-vNext#2

Open

3 tasks

powercode reopened this Nov 5, 2025

powercode requested a review from SeeminglyScience as a code owner November 5, 2025 11:55

powercode added KeepOpen The bot will ignore these and not auto-close and removed Stale labels Nov 5, 2025

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Nov 12, 2025

	if (_definedProperties.ContainsKey(propertyMemberAst.Name) \|\| _definedMethods.ContainsKey(propertyMemberAst.Name) && !_definedMethods[propertyMemberAst.Name].Any(m => m.Item1.IsConstructor))
	if (_definedProperties.ContainsKey(propertyMemberAst.Name) \|\| (_definedMethods.ContainsKey(propertyMemberAst.Name) && !_definedMethods[propertyMemberAst.Name].Any(m => m.Item1.IsConstructor)))

Conversation

IISResetMe commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

IISResetMe commented Nov 30, 2018

Uh oh!

IISResetMe commented Dec 7, 2018

Uh oh!

SteveL-MSFT commented Dec 20, 2018

Uh oh!

SteveL-MSFT commented Jan 16, 2019

Uh oh!

IISResetMe commented Jan 17, 2019

Uh oh!

iSazonov Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

IISResetMe Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

iSazonov Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

IISResetMe Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

IISResetMe Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IISResetMe commented Jan 18, 2019

Uh oh!

iSazonov commented Jan 18, 2019

Uh oh!

Uh oh!

IISResetMe commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Feb 23, 2019

Uh oh!

Uh oh!

IISResetMe commented Mar 8, 2019

Uh oh!

iSazonov commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Apr 17, 2019

Uh oh!

IISResetMe commented Apr 17, 2019

Uh oh!

daxian-dbw commented Apr 17, 2019

Uh oh!

daxian-dbw Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Apr 18, 2019

Uh oh!

vexx32 commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vexx32 commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IISResetMe commented Apr 19, 2019

Uh oh!

rjmholt commented Apr 19, 2019

Uh oh!

stale bot commented May 19, 2019

Uh oh!

stale bot commented May 29, 2019

Uh oh!

microsoft-github-policy-service bot commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

IISResetMe commented Nov 30, 2018 •

edited

Loading

IISResetMe commented Feb 22, 2019 •

edited

Loading

iSazonov commented Mar 8, 2019 •

edited

Loading

vexx32 commented Apr 18, 2019 •

edited

Loading

daxian-dbw commented Apr 19, 2019 •

edited

Loading

vexx32 commented Apr 19, 2019 •

edited

Loading