Fix the parsing code for method generic arguments

[daxian-dbw]

PR Summary

Fix #16870

Update the parsing code to only support the syntax $var.Method[TypeName1 <, TypeName2 ...>] for method generic arguments, not the syntax $var.Method[[TypeName1] <, [TypeName2] ...>].

The latter syntax has been supported for type expression since the beginning, but it's ambiguous in this scenario because we could be looking at an indexing operation on a property like: $var.Property[<expression>] and the <expression> could start with a type expression like [TypeName]::Method(), or even just a single type expression acting as a key to a hashtable property. Such cases will cause ambiguities.

Even though it might be possible to write code that sorts out the ambiguity and continue to support the latter syntax for method generic arguments, I choose not to do so, because:

1. that will definitely increase the complexity of the parsing code and also make it fragile.
2. the latter syntax hurts readability a lot due to the number of opening/closing brackets.

The downside is that the assembly-qualified type names won't be supported for method generic arguments, but that's likely not a problem in practice, and we can revisit if it turns out otherwise.

---

Besides, there is no point to allow declaring generic arguments for a property. Even for method group, it makes no sense to allow generic arguments to be declared for it because we don't do anything with the generic arguments for a method group, so no validation will be done -- you will see the following script works in 7.3-preview.1 unexpectedly:

PS:1> [string]::Join[nonexisting]

OverloadDefinitions
-------------------
static string Join(char separator, Params string[] value)
static string Join(string separator, Params string[] value)
static string Join(char separator, string[] value, int startIndex, int count)
static string Join(string separator, string[] value, int startIndex, int count)
static string Join(string separator, System.Collections.Generic.IEnumerable[string] values)
static string Join(char separator, Params System.Object[] values)
static string Join(string separator, Params System.Object[] values)
static string Join[T](char separator, System.Collections.Generic.IEnumerable[T] values)
static string Join[T](string separator, System.Collections.Generic.IEnumerable[T] values)

So, the related code in AST and parser are removed, and declaring a generic argument on a property will generate the same parsing errors as before:

PS:1> [string]::Join[nonexisting]
ParserError:
Line |
   1 |  [string]::Join[nonexisting]
     |                 ~
     | Array index expression is missing or not valid.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.

[SeeminglyScience]

LGTM!

[vexx32]

This looks good to me overall!

I would ask that we add a test similar to this actual use case from the linked issue:

		[IPAddress]::Parse(
			$_.IPSubnet[
				([Array]::IndexOf($_.IPAddress, $_.IPAddress[0]))
			]
		)

It's a bit of an odd case but does look like a pretty solid test case where we should be recognising the index expression instead of a partial generic method syntax.

[PaulHigin]

LGTM

[kborowinski]

This looks good to me overall!

I would ask that we add a test similar to this actual use case from the linked issue:

		[IPAddress]::Parse(
			$_.IPSubnet[
				([Array]::IndexOf($_.IPAddress, $_.IPAddress[0]))
			]
		)

It's a bit of an odd case but does look like a pretty solid test case where we should be recognising the index expression instead of a partial generic method syntax.

It indeed looks a bit odd but it's just a simplified version of following code that selects IP subnet from IP subnets array based on IP address index in corresponding IP address array:

...
$IPAddress = $_.IPAddress | Where-Object {$_ -like $using:Filter} | Select-Object -First 1
$IPSubnet  = [IPAddress]::Parse(($_.IPSubnet[[Array]::IndexOf($_.IPAddress, $IPAddress)]))
...

[pull-request-quantifier-deprecated]

This PR has 124 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Medium
Size       : +71 -53
Percentile : 44.8%

Total files changed: 3

Change summary by file extension:
.cs : +17 -42
.ps1 : +54 -11

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detetcted.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

[daxian-dbw]

The Invoke-WebRequest failures are known test issues, not related to changes in this PR.

[ghost]

🎉v7.3.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes